-
Notifications
You must be signed in to change notification settings - Fork 271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve preferences layout for long translations #1856
Conversation
<height>16777215</height> | ||
</size> | ||
</property> | ||
<property name="layoutDirection"> | ||
<enum>Qt::LeftToRight</enum> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the size hints to work properly. Since this is essentially an entirely vertical widget, this only really affects the position of the scrollbar in rtl languages. Given that the dialog is now by default large enough to show all items without scrolling, that shouldn’t be a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s odd, I’m definitely not seeing that on Linux. On Windows, Qt 5 has that issue but not Qt 6, maybe that’s the issue? I’ll see if I can figure out what’s going on there.
You mean the left-alignment? That’s what the size hint is supposed to deal with. It’s not technically new though. It’s not as noticeable in the English version because of the relatively short length of the labels, but in other languages it can be painfully obvious (see my “before” screenshot in the PR description). |
Ah good point, I was testing with Qt 5 but even with my Qt 6 build the issue remains. I've tested with 6.6.2 and 6.7.0.
Ah right.. forget that part then. |
c8b5ea6
to
5ffd283
Compare
I added some changes that, at least on Windows, seem to make the size hint behave as intended, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did the trick, both qt 5 and qt 6 works now.
Good job 👍
btw. shouldn't the PR point to the release branch or are we simply not applying more bug fixes to the release? |
Target the master branch since the change was made based on the master branch. And then I will cherry-pick this to the release branch. |
Since the change is for both branches I figured I might as well target master so it can then be cherry-picked to the release branch. But ofc I could also do it the other way around for any future changes if that’s more convenient. |
Fixes #1850 by simply removing the size restriction altogether. I couldn’t think of a good reason to keep it. I also made the dialog a little bigger by default and made some changes to the “tab bar” to avoid cutting off long translations.
Before:
After: