D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi updated this revision to Diff 73273.
trmdi added a comment.


  - use the flag approach and add DropJob::menuPopup()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26484?vs=73175=73273

BRANCH
  master

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

AFFECTED FILES
  src/core/job_base.h
  src/widgets/dropjob.cpp
  src/widgets/dropjob.h

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in dropjob.cpp:415
> `QCursor::pos()` should be avoided as it's not reliable on wayland.
> 
> The whole "popup() again" here looks like a workaround to me. Can't this be 
> fixed on the plasma side?

> Can't this be fixed on the plasma side?

I think it can't, because KIO::DropJob popup the menu, only afterward Plasma 
adds new items into it.

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in dropjob.cpp:415
> `QCursor::pos()` should be avoided as it's not reliable on wayland.
> 
> The whole "popup() again" here looks like a workaround to me. Can't this be 
> fixed on the plasma side?

I'm having a plan to make it popup only one time.

> QCursor::pos() should be avoided as it's not reliable on wayland.

this is how it does currently, I just copy the original code. This could need 
to be resolved in another patch.

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> dropjob.cpp:415
> +// popup again to update position. BUG: 415917
> +menu->popup(window ? window->mapToGlobal(m_relativePos) : 
> QCursor::pos());
>  }

`QCursor::pos()` should be avoided as it's not reliable on wayland.

The whole "popup() again" here looks like a workaround to me. Can't this be 
fixed on the plasma side?

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi added a comment.


  In D26484#591782 , @dfaure wrote:
  
  > Sorry I have no idea about the calling side in plasma. It sounds buggy 
indeed, if those are all calls on the same DropJob instance.
  
  
  Yes, you could check it here: 
https://phabricator.kde.org/source/plasma-framework/browse/master/src/scriptengines/qml/plasmoid/containmentinterface.cpp
  So do you mean this should be fixed to call 
`DropJob::setApplicationActions()` only one time?
  
  And do you @mart agree with that?

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Sorry I have no idea about the calling side in plasma. It sounds buggy 
indeed, if those are all calls on the same DropJob instance.

INLINE COMMENTS

> dropjob.cpp:409
>  d->m_appActions = actions;
> +auto m_relativePos = d->m_relativePos;
> +auto window = KJobWidgets::window(this);

The m_ prefix is for member variables. Don't use it for a local stack variable.

> dropjob.cpp:414
>  menu->addExtraActions(d->m_appActions, d->m_pluginActions);
> +// popup again to update position. BUG: 415917
> +menu->popup(window ? window->mapToGlobal(m_relativePos) : 
> QCursor::pos());

Doesn't this flicker?

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi added a comment.


  @mart @dfaure
  
  I have some question.
  1, Should `DropJob::setApplicationActions()` be called only one time?
  2, I see this when I drop multiple file types at once to the desktop (e.g. 2 
jpg and 1 zip), the `DropJob::setApplicationActions()` is called 3 times as 
follow:
  
Mimetype Job returns. "image/jpeg"
Received a suitable dropEvent at QPoint(1199,237)
Creating menu for: "image/jpeg" QPoint(1199,237)

Mimetype Job returns. "application/zip"
Received a suitable dropEvent at QPoint(1199,237)
Creating menu for: "application/zip" QPoint(1199,237)

Mimetype Job returns. "image/jpeg"
Received a suitable dropEvent at QPoint(1199,237)
Creating menu for: "image/jpeg" QPoint(1199,237)
  
  But finally only the last one's appActions are added to the menu. This is a 
bit weird.
  I want to understand your intention to make this patch as good as possible.

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26484: Popup menu again to reposition it

[Tranter Madi]

trmdi retitled this revision from "Add a new parameter for delaying showing 
menu" to "Popup menu again to reposition it".

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns