[Development] What to expect from QIcon/the icon engine on screen changes

[Elvis Stansvik]

Den mån 8 apr. 2019 kl 15:40 skrev Elvis Stansvik <elvstone at gmail.com>:
>
> Den sön 7 apr. 2019 kl 20:21 skrev Elvis Stansvik <elvstone at gmail.com>:
> >
> > Den sön 7 apr. 2019 kl 18:45 skrev Olivier Goffart <olivier at woboq.com>:
> > >
> > > On 06.04.19 10:40, Elvis Stansvik wrote:
> > > > Hi all,
> > > >
> > > > I'm looking for someone who knows the inner workings of QIcon and the
> > > > icon engines.
> > > >
> > > > In our application, we almost exclusively use SVG icons, and we use a
> > > > single SVG file for each icon (no @2x versions) that we try to design
> > > > to work reasonably well at all sizes, since we do not have the
> > > > resources to make custom variations for each target size.
> > > >
> > > > We put these SVG icons into an XDG icon theme, that we ship inside the
> > > > application resources (.qrc) in the expected XDG layout and with an
> > > > icon theme index file. We can then use
> > > > QIcon::fromTheme("our-app-some-icon") to reference an icon (either
> > > > through the .ui file or in code).
> > > >
> > > > The problem we've run into is that when the application is launched in
> > > > a mixed-DPI setup, for example a retina Mac laptop with an external
> > > > lower-DPI monitor, the icons appear too large and get cropped. In
> > > > effect, it seems to always use the DPI of the primary screen (the
> > > > built-in retina screen) when calculating the size of the pixmaps it
> > > > generates for the icons.
> > >
> > >
> > > Not sure if this is your problem, but it seems that
> > > QSvgIconEngine::virtual_hook does not handle the QIconEngine::ScaledPixmapHook
> > > call, which is needed for the QIcon::pixmap(QWindow *window, ...) call.
> >
> > Thanks Olivier,
> >
> > I'm not familiar with the code, but it sounds like that could be it.
> >
> > I'll try to make a minimal test case tomorrow.
>
> I've now created a minimal test case:
>
> dirac:~ insight$ find testcase
> testcase
> testcase/testcase.cpp
> testcase/icons
> testcase/icons/mytheme
> testcase/icons/mytheme/index.theme
> testcase/icons/mytheme/scalable
> testcase/icons/mytheme/scalable/actions
> testcase/icons/mytheme/scalable/actions/test-icon.svg
> testcase/testcase.pro
> testcase/testcase.qrc
> dirac:~ insight$
>
> testcase.cpp:
>
> #include <QApplication>
> #include <QIcon>
> #include <QToolButton>
>
> int main(int argc, char *argv[]) {
>     QApplication app(argc, argv);
>
>     app.setAttribute(Qt::AA_UseHighDpiPixmaps);
>
>     QIcon::setThemeName("mytheme");
>
>     // Create icon from file name -> Size of pixmap is correct on both screens
>     QToolButton button1;
>     button1.setIcon(QIcon("icons/mytheme/scalable/actions/test-icon.svg"));
>     button1.show();
>
>     // Create icon from theme -> Size of pixmap is correct only on primary screen
>     QToolButton button2;
>     button2.setIcon(QIcon::fromTheme("test-icon"));
>     button2.show();
>
>     return app.exec();
> }
>
> This is the result when running on the external lower-DPI monitor:
>
>
> Note how the right button icon (button2), which is created using QIcon::fromTheme has the wrong pixmap size for that monitor (it's appropriate for the internal retina screen).
>
> And it really does seem like the Qt::AA_UseHighDpiPixmaps application attribute has a finger in the game here, because if I remove setting of that attribute, the problem goes away.
>
> Attaching the full test case as a ZIP.
>
> Should I report this as a bug?

I couldn't find an issue quite like this in JIRA, so I went ahead and filed:

    https://bugreports.qt.io/browse/QTBUG-75039

Please continue any discussion there.

Elvis

>
> Elvis
>
> >
> > Elvis
> >
> > >
> > >
> > >
> > >
> > > >
> > > > To work around this, we've had to put in special code in all of our
> > > > widgets that use icons. The code reacts to screen change events (or
> > > > changes to the underlying QWindow in some cases), and in that code, go
> > > > through each and every one of our icons and do this monkey dance:
> > > >
> > > >          auto pixmap = someButton->icon().pixmap(someButton->iconSize());
> > > >          pixmap.setDevicePixelRatio(window()->windowHandle()->screen()->devicePixelRatio());
> > > >          someButton->setIcon(QIcon(pixmap));
> > > >
> > > > So essentially taking the pixmap out of the icon, set its DPR to that
> > > > of the current screen, and then set that pixmap back on the icon.
> > > >
> > > > This "works", the icons now look correct on both the retina screen and
> > > > the external one, and adjust themselves when the application is moved
> > > > back and forth, or when the external monitor is activated/deactivated.
> > > >
> > > > But surely this kludge should not be necessary? We've provided Qt with
> > > > an SVG, so it should be able to work out on its own that the screen
> > > > has changed, and regenerate an appropriate pixmap in response to that?
> > > >
> > > > Some more details:
> > > >
> > > > - We are running with the Qt::AA_UseHighDpiPixmaps application
> > > > attribute turned on. I'm not sure this is relevant for this issue
> > > > though, because AFAIK that attribute is only about picking 2x versions
> > > > of icons (we have a couple of those, where we have PNGs with 2x
> > > > versions).
> > > >
> > > > - We are not running with Qt's built-in scaling activated, but relying
> > > > on the Mac platform scaling (I'm not even sure Qt's built-in scaling
> > > > is applicable on Mac). The application runs with
> > > > NSHighResolutionCapable set to True in its Info.plist (which I also
> > > > think is the default nowadays).
> > > >
> > > > - I have not investigated yet whether this problem also occurs on a
> > > > mixed-DPI Linux setup, with Qt's high-dpi scaling activated. Nor have
> > > > I checked if it happens on Windows using it's artificial "screen
> > > > scaling" (we do not use Qt's built-in scaling on Windows either,
> > > > trying to follow the advise in the docs to avoid that for new
> > > > applications). So for now this is only about Mac retina + external
> > > > monitor.
> > > >
> > > > Any advise on this would be highly appreciated, because the code
> > > > required to re-trigger pixmap generation on screen changes is a real
> > > > kludge all over our code base, and often it happens that we add
> > > > buttons et.c. with icons, but forget to update this machinery.
> > > >
> > > > I'm not at the office at the moment, but can provide a little test
> > > > program that mimics what we're doing on Monday.
> > > >
> > > > Thanks in advance,
> > > > Elvis
> > > > _______________________________________________
> > > > Development mailing list
> > > > Development at qt-project.org
> > > > https://lists.qt-project.org/listinfo/development
> > > >
> > >
> > > _______________________________________________
> > > Development mailing list
> > > Development at qt-project.org
> > > https://lists.qt-project.org/listinfo/development