Bug 387539 - QMenuBar doesn't render icons properly
Summary: QMenuBar doesn't render icons properly
Status: RESOLVED FIXED
Alias: None
Product: Breeze
Classification: Plasma
Component: QStyle (show other bugs)
Version: 5.10.5
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Hugo Pereira Da Costa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-02 21:04 UTC by arsenarsentmc
Modified: 2017-12-08 13:54 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
POC code (694 bytes, text/plain)
2017-12-02 21:04 UTC, arsenarsentmc
Details

Note You need to log in before you can comment on or make changes to this bug.
Description arsenarsentmc 2017-12-02 21:04:13 UTC
Created attachment 109180 [details]
POC code

While making my software I noticed that with the Breeze styles fail to render QMenuBar icons properly, while it works with Fusion etc. even through qt5ct. Attached below are previews with some POC code. I could reproduce this on more systems than just Fedora by the way. They include, among others, Arch and derivatives, and Debian and derivatives.

Previews: https://imgur.com/a/soN5B
Comment 1 Hugo Pereira Da Costa 2017-12-03 07:42:20 UTC
yep. Breeze never attempted to render icons in menubar items. (I didn't know this was possible/needed).
For my understanding: what happens (with eg fusion), if you set both text and icon to the qaction ?
Comment 2 arsenarsentmc 2017-12-08 11:33:40 UTC
With both Fusion and GTK2 the icon gets rendered and while the action is being held pressed the text is rendered.
Comment 3 Hugo Pereira Da Costa 2017-12-08 12:36:11 UTC
(In reply to arsenarsentmc from comment #2)
> With both Fusion and GTK2 the icon gets rendered and while the action is
> being held pressed the text is rendered.

Thanks for the input. 
Interesting.
Feels like a bug to me ... 
I guess I'll try to implement rendering both text and icon in all cases as soon as both are set, in a way that is similar to what is done in menus ...
Comment 4 Hugo Pereira Da Costa 2017-12-08 12:59:20 UTC
Running your test program here with Qt 5.9.2, I can indeed see the red icon with widget styles gtk and windows, but not with fusion. What is the Qt version you are using ? 
In any case it seems that the feature you are trying to use is quite fragile and buggy. 
Anyway, I'll go ahead with the implementation which I think is the most rational.
Comment 5 Hugo Pereira Da Costa 2017-12-08 13:41:10 UTC
(In reply to Hugo Pereira Da Costa from comment #4)
> Running your test program here with Qt 5.9.2, I can indeed see the red icon
> with widget styles gtk and windows, but not with fusion. What is the Qt
> version you are using ? 
> In any case it seems that the feature you are trying to use is quite fragile
> and buggy. 
> Anyway, I'll go ahead with the implementation which I think is the most
> rational.

Forget about it. I can reproduce the behavior you describe for fusion too. 
I'm about to submit a patch to breeze and oxygen for rendering the icon (and only the icon) as soon as it is set. 
After checking the qmenubar code, text, if present, should be ignored in such cases (in both pressed and unpressed mode), as this is what qmenubar expects
Comment 6 Hugo Pereira Da Costa 2017-12-08 13:46:16 UTC
Git commit be09a708f5759792607aee45e553406d5a9024e5 by Hugo Pereira Da Costa.
Committed on 08/12/2017 at 13:41.
Pushed by hpereiradacosta into branch 'master'.

When an icon is set to a QMenuBar Item, render the icon only, and ignore the text, when set, as this is what qmenubar expects.

M  +67   -16   kstyle/breezestyle.cpp

https://commits.kde.org/breeze/be09a708f5759792607aee45e553406d5a9024e5
Comment 7 Hugo Pereira Da Costa 2017-12-08 13:53:31 UTC
Git commit 8a38d240cf7fbf34902edf99db1c880c4ee5eb1b by Hugo Pereira Da Costa.
Committed on 08/12/2017 at 13:53.
Pushed by hpereiradacosta into branch 'master'.

When an icon is set to a QMenuBar Item, render the icon only, and ignore the text, when set, as this is what qmenubar expects.

M  +48   -9    kstyle/oxygenstyle.cpp

https://commits.kde.org/oxygen/8a38d240cf7fbf34902edf99db1c880c4ee5eb1b
Comment 8 Hugo Pereira Da Costa 2017-12-08 13:54:10 UTC
Git commit 9f4fd9f43638527b719578f546d700d2f17b68bc by Hugo Pereira Da Costa.
Committed on 08/12/2017 at 13:53.
Pushed by hpereiradacosta into branch 'Plasma/5.11'.

When an icon is set to a QMenuBar Item, render the icon only, and ignore the text, when set, as this is what qmenubar expects.

M  +48   -9    kstyle/oxygenstyle.cpp

https://commits.kde.org/oxygen/9f4fd9f43638527b719578f546d700d2f17b68bc
Comment 9 Hugo Pereira Da Costa 2017-12-08 13:54:52 UTC
Git commit 3fcebd4e627a0255631ec81d39ed9eac158ac374 by Hugo Pereira Da Costa.
Committed on 08/12/2017 at 13:54.
Pushed by hpereiradacosta into branch 'Plasma/5.11'.

When an icon is set to a QMenuBar Item, render the icon only, and ignore the text, when set, as this is what qmenubar expects.

M  +67   -16   kstyle/breezestyle.cpp

https://commits.kde.org/breeze/3fcebd4e627a0255631ec81d39ed9eac158ac374