Fix 'Loading...' action not showing
ClosedPublic

Authored by faridb on Jun 3 2018, 7:12 PM.

Details

Summary

When the 'Share' button is clicked and the Kipi plugins are not loaded yet the menu is supposed to show Loading... but D13289 disabled that behaviour.

Diff Detail

Repository
R260 Gwenview
Branch
bug/share-menu-first-use
Lint
No Linters Available
Unit
No Unit Test Coverage
faridb requested review of this revision.Jun 3 2018, 7:12 PM
faridb created this revision.
faridb edited the summary of this revision. (Show Details)Jun 3 2018, 7:17 PM
faridb added reviewers: Gwenview, rkflx.
faridb added a comment.Jun 3 2018, 7:24 PM

Calling d->updateMenu() twice seems to fix both D13289 and D13312. If not the menu is updated correctly but it's invisible while still working (i.e. you can click invisible menu actions and they would work even if it doesn't show on screen).

rkflx added a comment.EditedJun 3 2018, 10:15 PM
  • At the start of updateMenu() call menu->hide(), then call menu->show() at the end. However, this causes the menu to flicker because it's shown and hidden quickly. It causes the menu to be shown when it shouldn't.

Yeah, it causes the button menu to show, even when you only clicked on Plugins in the menu bar ;)

  • Call d->updateMenu() twice when the loading is finished (i.e. both inside and outside if (d->mKIPIInterface->isLoadingFinished()) or twice outside).

This causes some flickering for me, and conceptually sounds a bit odd. Why is it working the second time, but not the first? It might be worth investigating what parts of the second call will do the trick.

The second solution seems to work fine but I couldn't find the real cause of the problem. What do you think?

I think we should at least try to find the real cause and not blindly try things.

the menu is updated correctly but it's invisible while still working (i.e. you can click invisible menu actions and they would work even if it doesn't show on screen).

That's really interesting, maybe we can make it work based on that.

I'd like to add two observations:

  • When I remove clear(), everything will work just fine (of course there will be one superfluous Loading... entry).
  • If I turn off compositing via +Alt+F12, the menu will also show every time.

What might be happening here is that clear() gets the "window" of the menu in such a state that KWin's compositor won't update it properly afterwards, or there is some kind of race. Technically it's there, but we don't see it. Note that this only affects compositing on X11, on Wayland I don't see the problem in the first place. Also, it's independent of the chosen widget style or the window decoration.

BTW, I'm mostly testing in a VM. Are you testing on real hardware?

Thoughts?

BTW, I'm mostly testing in a VM. Are you testing on real hardware?

Yes, I am doing the tests on real hardware (with Arch Linux | KDE Plasma 5.12.5 | KDE Frameworks 5.46.0 | Qt 5.11.0 | Linux 4.16.13)

Here is a video showing that the menu still works even if it's invisible:

I also noticed that it's not always broken but It works sometimes.

I found that someone had a similar problem with QMenu (on this thread) and I noticed that they were using Kubuntu (hence, most likely KDE) and the original implementation of KIPIExportAction::init() is logically correct. It seems like the problem is not related to the implementation of the Share menu but it looks like some sort of bug related to QMenu on KDE and this also points to that direction:

If I turn off compositing via ⇧+Alt+F12, the menu will also show every time.
[...]
Note that this only affects compositing on X11, on Wayland I don't see the problem in the first place

I think that D13289 and D13312 don't really solve the problem but they're just hacks which hide the actual problem.

faridb added a comment.EditedJun 4 2018, 11:25 AM

I kept digging and I found that it might be related to this code (in KIPIInterface::loadOnePlugin() which is in app/kipiinterface.cpp):

QMetaObject::invokeMethod(this, "loadOnePlugin", Qt::QueuedConnection);

If loadOnePlugin() is called synchronously the Share menu works fine (without D13289 or D13312). Could it be some sort of thread issue? But then why does the problem disappear when compositing is disabled?

rkflx added a comment.Jun 4 2018, 7:10 PM

For me it seem Gwenview is not at fault, and until we fix the issue elsewhere, here we only can work around the problem.

I found that someone had a similar problem with QMenu (on this thread)

At least it's somewhat comforting to know that others have the problem too. BTW, I already tried setUpdatesEnabled(false) before you posted the link, does not help.

I kept digging and I found that it might be related to this code (in KIPIInterface::loadOnePlugin() which is in app/kipiinterface.cpp):

QMetaObject::invokeMethod(this, "loadOnePlugin", Qt::QueuedConnection);

If loadOnePlugin() is called synchronously the Share menu works fine (without D13289 or D13312).

Using git blame on that line, I end up in a7fdfb42fad0, which reads "Do not block the UI while loading KIPI plugins". And indeed, if I follow your suggestion to use recursion (i.e. the direct message call) instead of iterating and coming back to the event loop (which invokeMethod achieves), the UI blocks, which results in Loading... not showing. (Add qDebug() before and after the call to see what I mean.)

Therefore I don't think we should go in that direction.

Could it be some sort of thread issue? But then why does the problem disappear when compositing is disabled?

Sounds like there is some kind of race, which does not work out in our favour in some situations.


For the moment, I think showing the menu after the first click is most important. Now we have to decide between flickering and not showing Loading.... Since machines are plenty fast nowadays (see your remark above), I'd say we can go with the latter until we have a proper fix to also improve it for slower machines.

I'd suggest to do the following:

  • Write a small test case app, similar to what's in the forum post.
  • Submit that as a bug to KWin (it might also be a Qt bug, though).
  • Change your patch to add a TODO to the code, with a short explanation and a reference to the bug number as well as the discussion over here.
faridb updated this revision to Diff 35565.Jun 4 2018, 10:05 PM

Include a TODO note to flag this fix as temporary

Write a small test case app, similar to what's in the forum post.

I actually tried to reproduce the bug by clearing a QMenu multiple times in a row but no luck, until I followed your suggestion and I used a QTimer to clear the menu which caused the exact same bug. It seems like it only happens when the menu is cleared through a slot triggered outside of the main event loop.

Submit that as a bug to KWin (it might also be a Qt bug, though).

I filed this bug in which I attached videos of the test program which reproduces the bug.

rkflx added a comment.Jun 5 2018, 1:12 PM

I filed this bug in which I attached videos of the test program which reproduces the bug.

Great, thank you so much for your investigations and filing the bug!

Include a TODO note to flag this fix as temporary

The patch looks a bit odd after your latest update: You only seem to add the comment, but the addition of d->updateMenu(); is not shown (while it should be, as currently that line is not in the repo).

Normally arc diff should do the right thing, i.e. upload the diff between HEAD and master, but maybe you did something different?

faridb updated this revision to Diff 35616.Jun 5 2018, 1:21 PM
  • Add TODO note to flag this as a temporary fix
rkflx accepted this revision.Jun 6 2018, 9:48 PM

Nice, LGTM now. I'll wait a couple of days whether we get any more updates on the bug, and then commit on your behalf.

This revision is now accepted and ready to land.Jun 6 2018, 9:48 PM
This revision was automatically updated to reflect the committed changes.