Iconloader scaling enhancemets

palinek commented 2 years ago

While debugging the lxqt/libfm-qt#788 I dived "deeply" into some aspects of the iconloader logic and here are probable enhancements.

palinek commented 2 years ago

@tsujan Can you, please, have a look into this commits?

tsujan commented 2 years ago

I assure you that the problem isn't in libqtxdg. LXQt's icon engine works self-consistently and without problem with scale factors; any change could damage that.

Anyway, this patch causes a crash with a scale factor:

#0  0x00007f9f8bef267a in QIconEngine::isNull() const () from /usr/lib/libQt5Gui.so.5
#1  0x00007f9f8c6e7586 in QToolButton::initStyleOption(QStyleOptionToolButton*) const ()
   from /usr/lib/libQt5Widgets.so.5
#2  0x00007f9f8c6ea3d0 in QToolButton::sizeHint() const () from /usr/lib/libQt5Widgets.so.5
#3  0x00007f9f8c52705b in qSmartMaxSize(QWidgetItem const*, QFlags<Qt::AlignmentFlag>) ()
   from /usr/lib/libQt5Widgets.so.5
#4  0x00007f9f8c52751e in QWidgetItem::maximumSize() const () from /usr/lib/libQt5Widgets.so.5
#5  0x00007f9f8c6df3cb in ?? () from /usr/lib/libQt5Widgets.so.5
#6  0x00007f9f8c6e201d in ?? () from /usr/lib/libQt5Widgets.so.5
#7  0x00007f9f8c5221ce in QLayout::totalMinimumSize() const () from /usr/lib/libQt5Widgets.so.5
#8  0x00007f9f8c526e45 in qSmartMinSize(QWidgetItem const*) () from /usr/lib/libQt5Widgets.so.5
#9  0x00007f9f8c6dd3b0 in ?? () from /usr/lib/libQt5Widgets.so.5
#10 0x00007f9f8c6dd548 in ?? () from /usr/lib/libQt5Widgets.so.5
#11 0x00007f9f8c655f94 in ?? () from /usr/lib/libQt5Widgets.so.5
#12 0x00007f9f8c656071 in ?? () from /usr/lib/libQt5Widgets.so.5
#13 0x00007f9f8c5221ce in QLayout::totalMinimumSize() const () from /usr/lib/libQt5Widgets.so.5
#14 0x00007f9f8c5231c8 in QLayout::activate() () from /usr/lib/libQt5Widgets.so.5
#15 0x00007f9f8c53e7aa in QWidgetPrivate::setVisible(bool) () from /usr/lib/libQt5Widgets.so.5
...

tsujan commented 2 years ago

BTW, what you see (with your eyes) partly depends on the widget style too (→ second paragraph of https://github.com/lxqt/libfm-qt/issues/788#issuecomment-1057899821); don't trust your eyes too much ;)

palinek commented 2 years ago

Anyway, this patch causes a crash with a scale factor:

Did you rebuild the lxqt-qtplugin also?

tsujan commented 2 years ago

Oh, you're right; I forgot that. But, still, I think it's important to stick with how Qt does it, in order to guarantee that our code is as standard as possible.

palinek commented 2 years ago

I think it's important to stick with how Qt does it

If we don't fix spotted bugs/problems, then there is no point for us to have our own implementation.

tsujan commented 2 years ago

If we don't fix spotted bugs/problems, then there is no point for us to have our own implementation.

Yes, but I'm not sure to which bug you refer. https://github.com/lxqt/libfm-qt/pull/792 isn't a bug in libqtxdg and the situation can become very confusing because of problems that are really elsewhere (like in the Breeze icon set: https://github.com/lxqt/libfm-qt/pull/792#issuecomment-1057986354).

palinek commented 2 years ago

I'm not sure to which bug you refer

There are 3 commits here:

iconloader: Build/use map for finding entries [current] the resolved entries for one particular XdgIconLoaderEngine object are crawled and tried to find the best match every time some information is requested (available size/paint/pixmap/...) [new] by adding a map the best suited entry for particular size&scale is searched only the first time; each next time a "cached pointer" is used (which I believe is more performant)
iconloader: Handle non-integer scaling more precise * [current] Qt (or any caller) calls the QIconEngine API requesting the <scaledSize> (<size> <floating scale>); In XdgIconLoaderEngine for getting the most valid entry it uses <requestedSize> (<scaledSize> / qCeil(<floating scale>)), which is obviously not correct as the original <size> != <requestedSize> for non-integer scales * [new] consider the <floating scale> as is for gathering the <requestedSize>, but do round the <floating scale> only for evaluation of the scaled (`@Sx) entries (also prefer the entries where scale == qCeil(`))
iconloader: Consider scale in actualSize() & pixmap() [current] in actualSize() & pixmap() for searching the most suitable entry the scale is not considered at all (i.e. it is (wrongly) assumed to be 1.0) [new] use application wide scale (the same as it is done in Qt methods, when the window is not known)

Don't you think, it is worth addressing?

EDIT: forget the "iconloader: Handle non-integer scaling more precise" commit, I was reading the logic wrong and I've substituted it with more suitable one

tsujan commented 2 years ago

Don't you think, it is worth addressing?

I'll need some time to read them.

Just one thing caught my eyes by a glance:

const qreal scale = qApp->devicePixelRatio();// Don't know which window to target

As far as I remember, we didn't have that elsewhere and I didn't find it in Qt's icon loader either. The problem is that a window of an app can be inside another screen with scaling. Therefore, qApp->devicePixelRatio() should be avoided, especially here.

My general point is that such details could easily get out of hand after a while if we don't stick with Qt's way of doing things as far as possible. Moreover, if our code gets too much different from that of Qt, we'll have trouble in importing Qt's fixes (like https://github.com/lxqt/libqtxdg/pull/225).

palinek commented 2 years ago

My general point is that such details could easily get out of hand after a while if we don't stick with Qt's way of doing things as far as possible.

I believe all these commits can be ported to Qt straight away. But this probably won't be backported to Qt v5.15 (I don't know if Qt would accept this binary incompatible change).

palinek commented 2 years ago

Therefore, qApp->devicePixelRatio() should be avoided, especially here.

It is used in Qt for similar purposes also:

tsujan commented 2 years ago

It is used in Qt for similar purposes also:

It's the last resort and only where really needed. The pixel ratio of window, pixmap or paint device has priority over it.

Just one possible scenario among who knows how many?

I have an app with a devicePixelRatio of 2 on an HDPI screen. One of its windows opens in an ordinary screen. QIcon::pixmap(QWindow *window,...) calls qt_effective_device_pixel_ratio(), sets devicePixelRatio to 1, and then calls d->engine->pixmap(). With your change, it may get a wrong pixmap. It's only in QIcon::pixmap(QWindow *window,...) that we can know about a window; we shouldn't deal with the pixel ratio prematurely in XdgIconLoaderEngine::pixmap().

palinek commented 2 years ago

One of its windows opens in an ordinary screen. QIcon::pixmap(QWindow *window,...) calls qt_effective_device_pixel_ratio(), sets devicePixelRatio to 1, and then calls d->engine->pixmap(). With your change, it may get a wrong pixmap.

...and we can say the same for the former "HDPI windows" -> without the change the windows on all HDPI screens may get wrong pixmaps (because we assume the scale == 1). There is no silver bullet for this, but I would say, that using the application wide scale will cover more situations correctly than assuming scale == 1.

tsujan commented 2 years ago

...and we can say the same for the former "HDPI windows" -> without the change the windows on all HDPI screens may get wrong pixmaps (because we assume the scale == 1).

No. It's correctly handled by QIcon::pixmap(QWindow *window,...).

In old times, we relied on qApp's pixel ratio. Now things are different: qApp's pixel ratio shouldn't take priority and, especially, it shouldn't be considered prematurely.

One of our (or my) users warned me about this (don't remember where). My first reaction was like yours but then I saw its logic; made changes here (following Qt) as well as in Kvantum.

There is no silver bullet for this

Following Qt is the safest way.

palinek commented 2 years ago

...and we can say the same for the former "HDPI windows" -> without the change the windows on all HDPI screens may get wrong pixmaps (because we assume the scale == 1).

...and we can say the same for the former "HDPI windows" -> without the change the windows on all HDPI screens may get wrong pixmaps (because we assume the scale == 1).

No. It's correctly handled by QIcon::pixmap(QWindow *window,...).

Then this exact answer is for you initial issue. We're not using/guessing the scale in PixmapEntry/ScalableEntry... but in the XdgIconLoaderEngine. Here I don't consider the usage pre-mature.

tsujan commented 2 years ago

Qt6 has gone further and has removed the scaling from QIconEngine::virtual_hook() too. Now the caller is responsible for providing QIcon::pixmap(const QSize &size, qreal devicePixelRatio, Mode mode, State state) with the correct dpr.

palinek commented 2 years ago

Qt6 has gone further and has removed the scaling from QIconEngine::virtual_hook() too.

It dropped the whole virtual_hook concept, but this doesn't make any difference to our discussed topic here.

tsujan commented 2 years ago

QIconLoaderEngine::virtual_hook() has lost its role, QIconEngine::virtual_hook() might be removed at some point, and QIcon::pixmap(QWindow *window,...) has been deprecated. That's a big step further away from consulting qApp's dpr at the level of QIconLoaderEngine.

I've kept scaling of virtual_hook() in https://github.com/lxqt/libqtxdg/tree/wip_qt6 though, as it seemed harmless.

palinek commented 2 years ago

and QIcon::pixmap(QWindow *window,...) has been deprecated. That's a big step further away from consulting qApp's dpr at the level of QIconLoaderEngine.

There are still the QIcon::pixmap(size...) overloads, which just uses the QIcon::pixmap(size, -1, ...) and this handles the scaling:

  QPixmap QIcon::pixmap(const QSize &size, qreal devicePixelRatio, Mode mode, State state) const
  {
      if (!d)
          return QPixmap();

      // Use the global devicePixelRatio if the caller does not know the target dpr
      if (devicePixelRatio == -1)
          devicePixelRatio = qApp->devicePixelRatio();

      // Handle the simple normal-dpi case
      if (!(devicePixelRatio > 1.0)) {
          QPixmap pixmap = d->engine->pixmap(size, mode, state);
          pixmap.setDevicePixelRatio(1.0);
          return pixmap;
      }

      // Try get a pixmap that is big enough to be displayed at device pixel resolution.
      QPixmap pixmap = d->engine->scaledPixmap(size * devicePixelRatio, mode, state, devicePixelRatio);
      pixmap.setDevicePixelRatio(d->pixmapDevicePixelRatio(devicePixelRatio, size, pixmap.size()));
      return pixmap;
  }

But reading the code of Qt (5.15 & 6.dev) and how the QIcon calls engine->pixmap()... yes you're right the QIconEngine::pixmap(..) should no use scaling.

But the QIconEngine::actualSize(..) is not aware of the scaling at all. So I still think, we should check tha app wide scale there.

tsujan commented 2 years ago

But the QIconEngine::actualSize(..) is not aware of the scaling at all.

In Qt5, because of the way QIcon::actualSize(QWindow *window,...) deals with it, QIconEngine::actualSize() shouldn't depend on qApp's dpr.

In Qt6, QIcon::actualSize(QWindow *window,...) is deprecated. Instead, QIcon::actualSize(size) takes qApp's dpr into account (and not that of window — there should be a good reason for it) and, again, because of that, QIconEngine::actualSize() shouldn't depend on qApp's dpr.

lxqt / libqtxdg

Iconloader scaling enhancemets #275