D26484: Add KIO::DropJobFlag to allow manually showing the menu
kossebau added a comment. And now reported the challenge of drop objects lifetime vs user input on decisions by https://bugreports.qt.io/browse/QTBUG-125229 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D26484 To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure Cc: kossebau, ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, ahmadsamir, bruns, vkrause
D26484: Add KIO::DropJobFlag to allow manually showing the menu
kossebau added a comment. Hi. I just came across dropping random data (in my case an image) into file views not working, i.e. not creating any new files due to no more data in the QMimeData instance referred to in the DropJob. Looking into the details, it seems this is due to the flaw of KIO::DropJob being async, but the QDropEvent (which got a respective comment) and even more the QMimeData it exposes (which has no comment, but just a helpless QPointer wrapper) is not defined to outlive the QWidget drop event handler call. And both at least X11 and Wayland protocol seems to negotiate things by the drop serving instance only offering a list of MIME types, next to the copy or move action, and only on the reply of the drop consumer actually deliver the data of the selected MIME type. My naive reaction to this would be to use some QEventLoop inside KIO::drop() for the user interaction to query "Copy or Move" and "Which format?", then extract the very data from the QMimeData, before returning to the Qt drop event handler call from which KIO::drop() would be called. But the method added here, DropJob::showMenu() and their usage would not be fixed by this. Anyone around still involved with this who could work on this from their use case POV? Or at least give some thoughts on this? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D26484 To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure Cc: kossebau, ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, ahmadsamir, bruns, vkrause
D26484: Add KIO::DropJobFlag to allow manually showing the menu
trmdi added a dependent revision: D26514: Delay popup for containments. 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: Add KIO::DropJobFlag to allow manually showing the menu
This revision was automatically updated to reflect the committed changes. Closed by commit R241:fbb2be9aad19: Add KIO::DropJobFlag to allow manually showing the menu (authored by trmdi). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26484?vs=73399=73400 REVISION DETAIL https://phabricator.kde.org/D26484 AFFECTED FILES 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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi updated this revision to Diff 73399. trmdi added a comment. - Comment REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26484?vs=73364=73399 BRANCH master REVISION DETAIL https://phabricator.kde.org/D26484 AFFECTED FILES 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: Add KIO::DropJobFlag to allow manually showing the menu
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. You can land after making these last minute adjustments, no need for another round of review :) INLINE COMMENTS > dropjob.h:83 > + * @since 5.67 > + **/ > +void showMenu(const QPoint , QAction *atAction = nullptr); Nitpick: normally it's a single star on the last line. Dunno if it matters for doxygen. > dropjob.h:163 > + * @since 5.67 > + **/ > +KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl > , DropJobFlags dropjobFlags, JobFlags flags = DefaultFlags); // BCI: > merge with DropJobFlags dropjobFlag = DropJobDefaultFlags (same) > dropjob.h:164 > + **/ > +KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl > , DropJobFlags dropjobFlags, JobFlags flags = DefaultFlags); // BCI: > merge with DropJobFlags dropjobFlag = DropJobDefaultFlags > + BIC = Binary Incompatible Change. What's BCI? In any case better add "TODO KF6" to the comment, I'm sure we'll be grepping for that when the time comes (rather than BCI/BIC). REPOSITORY R241 KIO BRANCH master 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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi updated this revision to Diff 73364. trmdi added a comment. - Typo REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26484?vs=73363=73364 BRANCH master REVISION DETAIL https://phabricator.kde.org/D26484 AFFECTED FILES 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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi marked 3 inline comments as done. 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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi updated this revision to Diff 73363. trmdi added a comment. - Improve comment/doc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26484?vs=73337=73363 BRANCH master REVISION DETAIL https://phabricator.kde.org/D26484 AFFECTED FILES 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: Add KIO::DropJobFlag to allow manually showing the menu
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Almost there.
Please add @since 5.67 to the new enum and the new methods.
INLINE COMMENTS
> dfaure wrote in dropjob.cpp:416
> This '{' goes on its own line.
Not fixed.
> dropjob.h:147
>
> +KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl
> , DropJobFlags dropjobFlags, JobFlags flags = DefaultFlags); // BCI:
> merge wih DropJobFlags dropjobFlag = DropJobDefaultFlag
> +
Please document this method (copy/pasting is OK)
And add @since 5.67
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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi edited the summary of this revision. 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: Add KIO::DropJobFlag to allow manually showing the menu
trmdi retitled this revision from "Add KIO::DropJobFlag flag and DropJob::menuPopup()" to "Add KIO::DropJobFlag to allow manually showing the menu". 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
