Skip to content
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

FIX(theme): Adapt Qt behavior change regarding padding in QMenu #5771

Merged
merged 2 commits into from
Aug 6, 2022

Conversation

Hartmnt
Copy link
Member

@Hartmnt Hartmnt commented Aug 3, 2022

Fixes #5462

This merge request contains two commits:

  1. The padding for QMenu items is adjusted according to the Qt behavior change described here. It has the side effect that menus without either icon or check-mark are now not reserving space on the left (see new help menu).
before after
screen1 screen1b
screen2 screen2b

This needs additional testing for Qt >= 5.14.1, because of the fix for QTBUG-80506 described here
If QMenus without icons, but WITH checkable items are too far right with Qt >= 5.14.1 (and also I understand all the padding bug reports at Qt correctly), the right margin of indicator needs to be removed

I think this change is good to go for Qt >= 5.12. For Qt 6 I do not know.

  1. The second commit enables icons in the Menu Bar menus, if they are not check able and an appropriate icon already exists.
    I consider the merge of the second commit optional.
    screen3

Qt >= 5.12.6 has changed the behavior of the padding value in style sheets
for QMenus. Previously the padding was counted from the start of the QMenu::item,
ignoring any decorations. The new behavior starts counting the padding after any
decoration item.

Without this change, Mumble compiled against Qt versions >= 5.12.6 will have
effectively double the padding on all QMenus containing icons or check-marks.
However, there are multiple issues with padding in QMenus pending on the Qt side
so this change may regress later on.

Fixes mumble-voip#5462
Since the introduction of the menu bar, action icons were hidden in it.

This commit enables the display of existing icons for actions which are not
checkable (otherwise it would clash with the check-mark). Additionally, the
"heart" icon is enabled for "Add Friend..." and the "i" icon is used for user
"Information..."
@Hartmnt
Copy link
Member Author

Hartmnt commented Aug 3, 2022

This whole problem is quite possibly the source of the alignment trouble in #4792 as it also affects user and channel QMenus in the TreeView

@Hartmnt
Copy link
Member Author

Hartmnt commented Aug 4, 2022

Tested on a fresh Ubuntu 22.10 beta VM with Qt 5.15.4:
screen3

1.3.4 vs 1.5.0:
screen4

Seems to work flawlessly.

@Hartmnt
Copy link
Member Author

Hartmnt commented Aug 5, 2022

Are you ok with menus without checkmark or icon to not reserve space on the left? If you want it consistent with the Qt internal theme, this would need a workaround.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Aug 5, 2022

Yes that's fine for me

@Krzmbrzl Krzmbrzl added client ui theme bug A bug (error) in the software labels Aug 6, 2022
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Aug 6, 2022

Note that in 1.4 we are still using git submodules for managing the theme, so backporting this fix is not as easy as it might appear. And given that the issue is not really deal-breaking, I'd vote for not actually bothering to backport it. Thus, it would only ship in Mumble 1.5.

@Krzmbrzl Krzmbrzl merged commit eea6eac into mumble-voip:master Aug 6, 2022
@Hartmnt
Copy link
Member Author

Hartmnt commented Aug 7, 2022

Note that in 1.4 we are still using git submodules for managing the theme, so backporting this fix is not as easy as it might appear. And given that the issue is not really deal-breaking, I'd vote for not actually bothering to backport it. Thus, it would only ship in Mumble 1.5.

I tend to agree, especially considering that running/compiling Mumble against a lower 5.x version of Qt would result in the opposite problem. And on Linux there might be some distros still shipping or depending on older versions of Qt.
looks at Debian still packaging Mumble 1.3.4 in sid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software client theme ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.4.230] [Windows 10] Too much padding in option menus
2 participants