Do not skip code launching application in application dashboard
ClosedPublic

Authored by luc4 on Jul 7 2019, 12:57 PM.

Details

Summary

Since Plasma 5.16, clicks over the icons are frequently ignored. See https://bugs.kde.org/show_bug.cgi?id=408748 for more info. This patch is an attempt to fix that seems to work for me.

BUG: 408748

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
luc4 created this revision.Jul 7 2019, 12:57 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 7 2019, 12:57 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
luc4 requested review of this revision.Jul 7 2019, 12:57 PM
luc4 added a reviewer: trmdi.Jul 7 2019, 1:11 PM
ngraham edited the summary of this revision. (Show Details)Jul 7 2019, 6:45 PM
ngraham accepted this revision.Jul 7 2019, 6:50 PM
ngraham added a reviewer: Plasma.
ngraham added a subscriber: hein.

I tested this with both mouse and touch. Though I could not reproduce the original issue, this patch does not introduce any regressions for me; in all cases, clicking/tapping and dragging both work correctly 100% of the time. The code change looks sensible as well.

I'd like at least one Plasma review before we land this. Preferably @hein since he's most familiar with the code, but IIRC he's on vacation right now so I think anyone else would be fine too.

This revision is now accepted and ready to land.Jul 7 2019, 6:50 PM
hein added a comment.Jul 17 2019, 7:01 AM

I see what this is trying to do, but some of the details seem a bit wrong.

E.g. you're only setting dragging to false in a code branch that's only executed when it already is false, which means it's not going to be set to false on a drag release, and pressX/Y also won't be unset on a release that doesn't happen above an iten anymore. You can keep the code flow change (the main thing this is probably fixing is that wrong check for the return value of updatePositionProperties), but please rework the patch a bit to make sure that the state of these variables still gets updated in the way they should on a release.

luc4 added a comment.Jul 17 2019, 11:19 AM

Unfortunately I'm not able to get logs from plasma-desktop. Can someone explain how (assuming this is possible) to write logs in this portion of code and read? Or can you point me to docs? I tried to add console.log calls and read using journalctl but I don't see the logs. Can someone help so I can check the values of those variables? Thanks.

hein added a comment.EditedJul 18 2019, 4:28 AM

First of, run kdebugsettings and make sure that debug output isn't disabled on your system.

Then you can e.g. stop the shell (panels, desktop background) from a terminal with: kquitapp5 plasmashell

And restart it with plasmashell, and you'll see console.log output on the terminal.

If you don't want to muck with your shell, you can also run a seperate instance of just Dashboard, which will also cut down on unrelated debug output noise: plasmawindowed org.kde.plasma.kickerdash

(Whether the debug output shows in journalctl normally depends on the distro, but otherwise you can usually find it by tailing the xsession-errors or wayland-errors files, modulo the kdebugsettings status.)

luc4 updated this revision to Diff 61994.Jul 18 2019, 6:02 PM
This comment was removed by luc4.
luc4 added a comment.Jul 18 2019, 11:01 PM

Hello! Thank you for your help! I can see the logs now.
Unfortunately I cannot update my patch according to your notes. According to the logs I added, I do not see values that I wouldn't expect. Can you describe somehow how to reproduce the behavior you reported?
Thanks.

luc4 updated this revision to Diff 62170.EditedJul 21 2019, 9:12 AM

This is another implementation that follows the same principle of the previous ones, but moves the responsibility of maintaining the state to the dragHelper object. This requires to postpone the reset of the state after the onRelease event.

luc4 updated this revision to Diff 62338.Jul 22 2019, 6:19 PM

Removed volatile modifier, not probably useful in that context.

hein added a comment.Jul 24 2019, 9:36 AM

Thanks, this looks a lot better and cleaner. Minor nitpick to clean up.

applets/kicker/package/contents/ui/ItemGridView.qml
440

Coding style: Even for single-line blocks we require braces.

luc4 updated this revision to Diff 62488.Jul 24 2019, 4:31 PM

Updated to respect code conventions.

ngraham accepted this revision.Jul 24 2019, 5:05 PM
hein accepted this revision.Jul 24 2019, 5:06 PM
luc4 marked an inline comment as done.Jul 24 2019, 5:23 PM

I do not have commit access, can someone do it for me? Thank you guys for your help!

This revision was automatically updated to reflect the committed changes.