Bug 390737 - App icons look pixelated when I increase display scale
Summary: App icons look pixelated when I increase display scale
Status: RESOLVED FIXED
Alias: None
Product: kmenuedit
Classification: Applications
Component: general (show other bugs)
Version: 5.12.1
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: Unassigned bugs mailing-list
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2018-02-19 15:17 UTC by Patrick Silva
Modified: 2018-03-24 11:00 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
screenshot (111.74 KB, image/png)
2018-02-19 15:17 UTC, Patrick Silva
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Silva 2018-02-19 15:17:39 UTC
Created attachment 110814 [details]
screenshot

See the screenshot.
My display scale is 1.2 under X11.
Comment 1 Nate Graham 2018-02-19 16:09:46 UTC
Most likely needs `QGuiApplication::setAttribute(Qt::AA_UseHighDpiPixmaps);` set in main.cpp - the same fix as Bug 388633.

Dr Chapatin, would you like to tackle this fix? It looks pretty simple, and I can walk you through it.
Comment 2 Patrick Silva 2018-02-19 17:24:57 UTC
No, thanks.
I undertand nothing about software development.
Comment 3 Nate Graham 2018-02-19 17:40:21 UTC
If you'd like to learn, I'm happy to teach you!

Otherwise, I'll fix it.
Comment 4 Patrick Silva 2018-02-19 17:55:11 UTC
Thank you for your offer.
But I have no interest.
Comment 5 Andrew Crouthamel 2018-02-19 20:58:11 UTC
I confirm the pixelation, I run 2x scaling and see it. Nate is getting me setup to do dev work, so I'll take a whack at this in the next 24 hours.
Comment 6 Andrew Crouthamel 2018-02-21 16:48:32 UTC
I'm currently working on this. I have this fixed in main.cpp, but it's still an issue in the tree menu on the left. I'm trying to figure out where I need to make a change for that.
Comment 7 null 2018-02-21 17:13:31 UTC
Easiest if you just upload an in-progress version of your patch, e.g. via "arc diff --plan-changes" or "git diff | arc paste".

Or is it something like https://phabricator.kde.org/D10532?
Comment 8 null 2018-02-21 17:17:54 UTC
And don't forget about GammaRay, where you can just start the app, use a tool in the "Widgets" tab to point at the element you are interested in, and instantly you know more about it (you can even open your editor at the line the object was created).
Comment 9 Andrew Crouthamel 2018-02-21 17:36:04 UTC
(In reply to Henrik Fehlauer from comment #8)
> And don't forget about GammaRay, where you can just start the app, use a
> tool in the "Widgets" tab to point at the element you are interested in, and
> instantly you know more about it (you can even open your editor at the line
> the object was created).

Well that's cool. Thanks for that!
Comment 10 null 2018-02-22 22:19:11 UTC
Poked at this some more. In this case GammaRay only tells us to look at the "TreeView" class. There I grepped for "icon" and ended up in the static function "appIcon".

My next idea would be to try whether something like the link I mentioned before would help, because this one was also about "KIconLoader" (which is only used there and in "startDrag"), but I'll leave that for you to test.

Let me know if that helps.
Comment 11 Andrew Crouthamel 2018-02-23 05:39:48 UTC
(In reply to Henrik Fehlauer from comment #10)
> Poked at this some more. In this case GammaRay only tells us to look at the
> "TreeView" class. There I grepped for "icon" and ended up in the static
> function "appIcon".
> 
> My next idea would be to try whether something like the link I mentioned
> before would help, because this one was also about "KIconLoader" (which is
> only used there and in "startDrag"), but I'll leave that for you to test.
> 
> Let me know if that helps.

Thanks, I tried fiddling with it to convert it to QIcon::fromTheme. I got stuck and asked #kde-devel for help, they said to fix KIconLoader. Interestingly bug 390639 was linked to me, where David E mentions Qt 5.11 should fix some icon issues. I'm awaiting more info.
Comment 12 Andrew Crouthamel 2018-02-26 19:23:59 UTC
Interestingly, I've discovered that the same treemenu which appears when clicking "Add Program" in the Autostart configuration app, is not pixelated. So that gives me something to look at code-wise.
Comment 13 null 2018-03-01 23:56:14 UTC
The bug you linked is probably unrelated, and I doubt changes in Qt 5.11 will help in our case (I only know about https://phabricator.kde.org/D5109#162592).

IMO it's quite unrealistic to expect you to fix KIconLoader if even more experienced developers are stuck (see https://phabricator.kde.org/D6313). Also, https://phabricator.kde.org/D10532 clearly applied a similar fix and it was allowed to go in.

Looking at the "Open With" dialog you get for "Add Program", you'll observe the problem with QIcon::fromTheme we've talked about elsewhere (which I believe should possibly be fixed in Breeze): Going from 1x scaling to 1.5x scaling, the Akregator icon changes colour. Now, given for this dialog it's somewhat broken, I'd suggest to just go with QIcon::fromTheme for your patch too.

Let me know where you are stuck, or upload a WIP version. I tried it myself, and the patch really is not all that complicated. Just try to understand what the code is doing in both cases, and you should be able to adapt it.
Comment 14 Kai Uwe Broulik 2018-03-02 10:45:39 UTC
@Andrew: QTreeWidgetItem used by the tree has a setIcon(QIcon) method so you could just pass it a QIcon::fromTheme directly (ie. change appIcon to return QIcon instead of QPixmap), then the tree view can choose the appropriate icon on the fly. Don't forget to set AA_UseHighDpiPixmaps in main :)
Comment 15 null 2018-03-02 10:49:58 UTC
We might even remove setIcon(...) completely once it is so simple (no SC/BC guarantees, I assume?).
Comment 16 null 2018-03-02 10:59:00 UTC
> remove setIcon(...)
Meant appIcon(...) of course, sadly no editing of comments on Bugzilla.
Comment 17 Kai Uwe Broulik 2018-03-02 11:00:18 UTC
Sure, you could do that.
Comment 18 null 2018-03-24 11:00:51 UTC
Git commit e8e3c0f8e4a122ffc13386ab55b408aba2d29e14 by Henrik Fehlauer, on behalf of Andrew Crouthamel.
Committed on 24/03/2018 at 10:48.
Pushed by rkflx into branch 'Plasma/5.12'.

Fix pixelated icon scaling for HiDPI screens

Summary:
This enables HiDPI pixmaps in the main window. This commit also replaces QPixmap and KIconLoader with QIcon to load properly scaled icons in the tree menu for HiDPI screens.

Before:
{F5764274}

After:
{F5764275}

Test Plan:
Open icon selection in main window.
Expand tree menu and ensure selection works.

Reviewers: rkflx, broulik, davidedmundson

Reviewed By: rkflx, davidedmundson

Subscribers: davidedmundson, cfeck, ngraham, plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D11631

M  +1    -0    main.cpp
M  +5    -12   treeview.cpp

https://commits.kde.org/kmenuedit/e8e3c0f8e4a122ffc13386ab55b408aba2d29e14