D22306: Do not skip code launching application in application dashboard

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:fba194e56273: Do not skip code launching application in 
application dashboard (authored by luc4, committed by ngraham).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22306?vs=62488=62499

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml
  applets/kicker/plugin/draghelper.cpp
  applets/kicker/plugin/draghelper.h

To: luc4, ngraham, trmdi, #plasma, hein
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 marked an inline comment as done.
luc4 added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma, hein
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 updated this revision to Diff 62488.
luc4 added a comment.


  Updated to respect code conventions.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22306?vs=62338=62488

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml
  applets/kicker/plugin/draghelper.cpp
  applets/kicker/plugin/draghelper.h

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Eike Hein]

hein added a comment.


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

INLINE COMMENTS

> ItemGridView.qml:440
> +itemGrid.itemActivated(pressedItem.itemIndex, "", null);
> +} else if (!dragHelper.dragging && !pressedItem && 
> mouse.button == Qt.LeftButton)
> +root.toggle();

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 updated this revision to Diff 62338.
luc4 added a comment.


  Removed volatile modifier, not probably useful in that context.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22306?vs=62170=62338

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml
  applets/kicker/plugin/draghelper.cpp
  applets/kicker/plugin/draghelper.h

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 updated this revision to Diff 62170.
luc4 added a comment.


  This is another implementation that follows the same principale 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.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22306?vs=61994=62170

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml
  applets/kicker/plugin/draghelper.cpp
  applets/kicker/plugin/draghelper.h

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 updated this revision to Diff 61994.
luc4 added a comment.


  Also reset the state variable during onPress.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22306?vs=61271=61994

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Eike Hein]

hein added a comment.


  First of, run `kdebugsettings` and make sure that debug output isn't disabled 
on your system.
  
  Then you can e.g. stop plasmashell 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 plasmashell, 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`

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Nathaniel Graham]

ngraham added a comment.


  #plasma  ping

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Nathaniel Graham]

ngraham added a reviewer: Plasma.
ngraham accepted this revision.
ngraham added a subscriber: hein.
ngraham added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

To: luc4, ngraham, trmdi, #plasma
Cc: hein, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22306: Do not skip code launching application in application dashboard

[Luca Carlon]

luc4 created this revision.
luc4 added a reviewer: ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
luc4 requested review of this revision.

REVISION 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D22306

AFFECTED FILES
  applets/kicker/package/contents/ui/ItemGridView.qml

To: luc4, ngraham
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart