D29006: Allow to copy or move selection to the other split view

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:a291c5999035: Allow to copy or move selection to the 
other split view (authored by aprcela, committed by ngraham).

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=82883=82884

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Thank you!

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

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


  Done :)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 82883.
aprcela marked 2 inline comments as done.
aprcela added a comment.


  - Renaming of functions

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=82880=82883

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Nathaniel Graham]

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


  Actually sorry, I have a few more comments before I think this can land:

INLINE COMMENTS

> dolphintabwidget.h:196
> +/** Moves all selected items to the other view. */
> +void moveToOtherView();
> +

"the other view" is not very informative. How about 
`{move,copy}ToInactiveSplitView()`, to mirror the name of the action itself?

> dolphinui.rc:2
>  
> -
> +
>  

bump to 31, not 32

> dolphinview.h:377
> + */
> +void copySelectedItems(const KFileItemList , const QUrl 
> );
> +

ditto for the names here

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Thanks!

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#671172 , @ngraham wrote:
  
  > Nice feature. I would rename the menu items a bit:
  >  "Move to inactive split view"
  >  "Copy to inactive split view"
  >
  > No need to mention "the selection" because it's implicit. And I would 
mention that this is about the split view, or else when not using the split 
view feature, it's not clear how to enable the menu items. If you mention 
splits in the name though, it's advertising the split view feature for whose 
who haven't found it yet.
  
  
  Did the renaming. And sorry @meven just had a hard time rebasing now.. :/

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 82880.
aprcela added a comment.


  - Minor renaming of text and the according variables
  - Merge remote-tracking branch 'origin' into arcpatch-D29006

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=82628=82880

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Nathaniel Graham]

ngraham added a comment.


  Nice feature. I would rename the menu items a bit:
  "Move to inactive split view"
  "Copy to inactive split view"
  
  No need to mention "the selection" because it's implicit. And I would mention 
that this is about the split view, or else when not using the split view 
feature, it's not clear how to enable the menu items. If you mention splits in 
the name though, it's advertising the split view feature for whose who haven't 
found it yet.

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#670660 , @meven wrote:
  
  > Rather than merging master into your branches, I would recommend you 
rebasing ;)
  
  
  Ok, thanks for the information! :)

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Méven Car]

meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.


  Rather than merging master into your branches, I would recommend you rebasing 
;)

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked 2 inline comments as done.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked 3 inline comments as done.
aprcela added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in dolphinview.h:377
> Let's just call it `destinationUrl`. This can be any URL, not necessarily the 
> URL of a split pane.

Yeah, makes sense. Done

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 82628.
aprcela marked an inline comment as done.
aprcela added a comment.


  - Renamed to destinationUrl

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=82609=82628

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> dolphinview.h:377
> + */
> +void copySelectedItems(const KFileItemList , const QUrl 
> );
> +

Let's just call it `destinationUrl`. This can be any URL, not necessarily the 
URL of a split pane.

> dolphinview.h:383
> + */
> +void moveSelectedItems(const KFileItemList , const QUrl 
> );
> +

Same here.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked an inline comment as done.
aprcela added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in dolphinview.h:367-377
> Not anymore since d1a70c0b62 
>  ;)

Thanks. I renamed those now :)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 82609.
aprcela added a comment.


  - Merge branch 'master' of https://anongit.kde.org/dolphin into 
arcpatch-D29006
  - Merge branch 'master' of https://anongit.kde.org/dolphin into 
arcpatch-D29006
  - Rename to (copy|move)SelectedItems

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81872=82609

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> aprcela wrote in dolphinview.h:367-377
> `copySelectedItems()` is already used to copy to clipboard. See L365.

Not anymore since d1a70c0b62 
 ;)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#667654 , @meven wrote:
  
  > In D29006#667588 , @dfaure wrote:
  >
  > > It sounds to me like you found bugs in some kioslaves. Fixing that is out 
of scope for this change request. Certainly a good idea to do that, but IMHO 
not a blocker for this to go in.
  >
  >
  > I don't think this is a bug in any ioslave : dolphin view doesn't have this 
issue when copy/pasting files in , and for instance recentlyused:/ has 
"writing=false" and "moving=false" in its json file.
  >  So I would guess this is not a bug there but somewhere else along the call 
chain. One can compare this code to `DolphinContextMenu::createPasteAction` 
that uses `KIO::pasteActionText`
  >
  > I agree this should not keep this bug from landing.
  >  But it would be great to have a look as to where the issue comes from...
  >
  > One approval from @ngraham or @elvisangelaccio and we should land this.
  
  
  I'm gonna check this one again later on to see if I can find anything.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Méven Car]

meven added a comment.


  In D29006#667588 , @dfaure wrote:
  
  > It sounds to me like you found bugs in some kioslaves. Fixing that is out 
of scope for this change request. Certainly a good idea to do that, but IMHO 
not a blocker for this to go in.
  
  
  I don't think this is a bug in any ioslave : dolphin view doesn't have this 
issue when copy/pasting files in , and for instance recentlyused:/ has 
"writing=false" and "moving=false" in its json file.
  So I would guess this is not a bug there but somewhere else along the call 
chain. One can compare this code to `DolphinContextMenu::createPasteAction` 
that uses `KIO::pasteActionText`
  
  I agree this should not keep this bug from landing.
  But it would be great to have a look as to where the issue comes from...
  
  One approval from @ngraham or @elvisangelaccio and we should land this.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Good from a KIO point of view, but this needs approval from Dolphin people.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure added a comment.


  It sounds to me like you found bugs in some kioslaves. Fixing that is out of 
scope for this change request. Certainly a good idea to do that, but IMHO not a 
blocker for this to go in.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, 
Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, 
skadinna, emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  @meven 
  These seem to be 'special-cases' locations. 
  The call to `capabilitiesDestination.isWritable()` :
  
  Does not work for these. An error message pops up when trying to move/copy 
here):
  remote:/
  recentlyused:/
  search:/
  
  Does work as expected, following destination is writable:
  trash:/
  
  Can't test:
  /media/nfs
  /media/floppy0
  /media/XO-Y4
  /media/cdrom
  
  What would be a good approach to solve this?

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81872.
aprcela added a comment.


  - Rewrite so it's obvious that it is copying/moving to the inactive view

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81854=81872

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela edited the summary of this revision.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela edited the summary of this revision.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81854.
aprcela added a comment.


  - Merge branch 'master' of https://anongit.kde.org/dolphin into 
arcpatch-D29006

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81850=81854

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked an inline comment as done.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked an inline comment as done.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked an inline comment as done.
aprcela added inline comments.

INLINE COMMENTS

> aprcela wrote in dolphinview.cpp:1222
> Ok. I'm gonna make a separate patch via phabricator.

https://phabricator.kde.org/D29399

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81850.
aprcela marked an inline comment as done.
aprcela added a comment.


  - Revert "Instead of append, change the value to a simplifiedUrlList"

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81826=81850

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked an inline comment as done.
aprcela added inline comments.

INLINE COMMENTS

> meven wrote in dolphinview.cpp:1222
> No your individual commits are supposed to get squashed upon merging.

Ok. I'm gonna make a separate patch via phabricator.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> aprcela wrote in dolphinview.cpp:1222
> It's in the commit 5359352b0ef1. Is that enough?

No your individual commits are supposed to get squashed upon merging.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked 2 inline comments as done.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela marked 2 inline comments as done.
aprcela added a comment.


  @elvisangelaccio wrote:
  
  copySelectedItems() is already used to copy to clipboard. See L365.
  
  @meven I uploadaded the currently working code (to check if destination is 
writable for the copy function). But I can't get it to work for the location 
you provided: `recentlyused:/files/`

INLINE COMMENTS

> elvisangelaccio wrote in dolphinview.cpp:1222
> I see.
> 
> @aprcela Can you add this information in the commit message? Otherwise it 
> will get lost.

It's in the commit 5359352b0ef1. Is that enough?

> elvisangelaccio wrote in dolphinview.h:367-377
> Nitpick: `DolphinView` doesn't know what a "split view" is. I'd rather call 
> these methods `copySelectedItems()` and `moveSelectedItems()`, since that's 
> what they are actually doing.

`copySelectedItems()` is already used to copy to clipboard. See L365.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81826.
aprcela marked 2 inline comments as done.
aprcela added a comment.


  - Add check if destination is writable for copy
  - Move copy/move logic to DolphinTabWidget
  - Set Shift+F5 /F6 instead of CTRL+F5 
/F6 as keyboard shortcut

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81765=81826

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphintabwidget.cpp
  src/dolphintabwidget.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.


  The feature looks reasonable to me.

INLINE COMMENTS

> dolphinmainwindow.cpp:670-684
> +void DolphinMainWindow::copyToOtherView()
> +{
> +const DolphinTabPage* tabPage = m_tabWidget->currentTabPage();
> +if (!tabPage->splitViewEnabled() || 
> m_activeViewContainer->view()->selectedItems().isEmpty()) {
> +return;
> +}
> +

Can you try to add a `DolphinTabWidget::copyToOtherView()` function and move 
this logic there?

> dolphinmainwindow.cpp:686-700
> +void DolphinMainWindow::moveToOtherView()
> +{
> +const DolphinTabPage* tabPage = m_tabWidget->currentTabPage();
> +if (!tabPage->splitViewEnabled() || 
> m_activeViewContainer->view()->selectedItems().isEmpty()) {
> +return;
> +}
> +

Can you try to add a `DolphinTabWidget::moveToOtherView()` function and move 
this logic there?

> dfaure wrote in dolphinview.cpp:1222
> This line of code (from me, it turns out) is bogus, it should say `=` instead 
> of `<<`.
> We want to replace the list with the simplified list, not concatenate the two.
> 
> And m_selectedUrls needs to contain the new URLs, just like we do when 
> pasting.
> 
> The solution I have in mind for that is to connect to 
> CopyJob::slotCopyingDone and grab the "to" argument.
> 
> [discussed on IRC, but writing here for the record, and in case someone 
> wonders why this << gets modified]

I see.

@aprcela Can you add this information in the commit message? Otherwise it will 
get lost.

> dolphinview.h:367-377
> +/**
> + * Copies all selected items to the other view.
> + * Only available in Split View.
> + */
> +void copySelectedItemsToOtherSplitView(const KFileItemList , 
> const QUrl );
> +
> +/**

Nitpick: `DolphinView` doesn't know what a "split view" is. I'd rather call 
these methods `copySelectedItems()` and `moveSelectedItems()`, since that's 
what they are actually doing.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#662058 , @meven wrote:
  
  > Just a check to add.
  
  
  @meven See inline comment
  
  In D29006#662094 , @dfaure wrote:
  
  > Actually, wait, I vote against Ctrl+F5 
/Ctrl+F6 because these shortcuts, by default, 
in Plasma, are bound to "Switch to Desktop 5" and "Switch to Desktop 6".
  >
  > Shift+F5 /F6 would be much better, if it's 
available.
  >
  > Meven: drag-n-drop is 'easy' but it requires using the mouse. For 
accessibility, or for people stuck on a plane with a bad touchpad and no room 
for a real mouse, or simply for maximum performance in optimized workflows, I 
can see the benefit of keyboard shortcuts.
  >
  > Just not if they switch desktops inadvertently, once you configure 6+ 
virtual desktops :-)
  
  
  `Shift+F5/F6` seems to bee free. So we could go with that.

INLINE COMMENTS

> meven wrote in dolphinmainwindow.cpp:1993
> This is not sufficient, you can copy or move to `recentlyused:/files/` for 
> instance, which fails.
> So test the other view url to see if it is writable as well.

I'm trying to add a check and it only doesn't work only on this kind of 
folders. Works fine with local and samba share. See diff:

  diff --git a/src/dolphinmainwindow.cpp b/src/dolphinmainwindow.cpp
  index 428c61952..5e7c56385 100644
  --- a/src/dolphinmainwindow.cpp
  +++ b/src/dolphinmainwindow.cpp
  @@ -1959,7 +1959,7 @@ void DolphinMainWindow::updateFileAndEditActions()
   {
   const KFileItemList list = 
m_activeViewContainer->view()->selectedItems();
   const KActionCollection* col = actionCollection();
  -KFileItemListProperties capabilities(list);
  +KFileItemListProperties capabilitiesSource(list);
   
   QAction* addToPlacesAction = 
col->action(QStringLiteral("add_to_places"));
   QAction* copyToOtherViewAction   = 
col->action(QStringLiteral("copy_to_other_split_view"));
  @@ -1989,22 +1989,31 @@ void DolphinMainWindow::updateFileAndEditActions()
   }
   
   if (m_tabWidget->currentTabPage()->splitViewEnabled()) {
  -copyToOtherViewAction->setEnabled(true);
  -moveToOtherViewAction->setEnabled(capabilities.supportsMoving());
  +DolphinTabPage* tabPage = m_tabWidget->currentTabPage();
  +KFileItem capabilitiesDestination;
  +
  +if (tabPage->primaryViewActive()) {
  +capabilitiesDestination = 
tabPage->secondaryViewContainer()->url();
  +} else {
  +capabilitiesDestination = 
tabPage->primaryViewContainer()->url();
  +}
  +
  +
copyToOtherViewAction->setEnabled(capabilitiesDestination.isWritable());
  +
moveToOtherViewAction->setEnabled(capabilitiesSource.supportsMoving() && 
capabilitiesDestination.isWritable());
   } else {
   copyToOtherViewAction->setEnabled(false);
   moveToOtherViewAction->setEnabled(false);
   }
   
  -const bool enableMoveToTrash = capabilities.isLocal() && 
capabilities.supportsMoving();
  +const bool enableMoveToTrash = capabilitiesSource.isLocal() && 
capabilitiesSource.supportsMoving();
   
  -renameAction->setEnabled(capabilities.supportsMoving());
  +renameAction->setEnabled(capabilitiesSource.supportsMoving());
   moveToTrashAction->setEnabled(enableMoveToTrash);
  -deleteAction->setEnabled(capabilities.supportsDeleting());
  -deleteWithTrashShortcut->setEnabled(capabilities.supportsDeleting() 
&& !enableMoveToTrash);
  -cutAction->setEnabled(capabilities.supportsMoving());
  +deleteAction->setEnabled(capabilitiesSource.supportsDeleting());
  +
deleteWithTrashShortcut->setEnabled(capabilitiesSource.supportsDeleting() && 
!enableMoveToTrash);
  +cutAction->setEnabled(capabilitiesSource.supportsMoving());
   showTarget->setEnabled(list.length() == 1 && list.at(0).isLink());
  -duplicateAction->setEnabled(capabilities.supportsWriting());
  +duplicateAction->setEnabled(capabilitiesSource.supportsWriting());
   }
   }

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.


  Actually, wait, I vote against Ctrl+F5 
/Ctrl+F6 because these shortcuts, by default, 
in Plasma, are bound to "Switch to Desktop 5" and "Switch to Desktop 6".
  
  Shift+F5 /F6 would be much better, if it's 
available.
  
  Meven: drag-n-drop is 'easy' but it requires using the mouse. For 
accessibility, or for people stuck on a plane with a bad touchpad and no room 
for a real mouse, or simply for maximum performance in optimized workflows, I 
can see the benefit of keyboard shortcuts.
  
  Just not if they switch desktops inadvertently, once you configure 6+ virtual 
desktops :-)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Méven Car]

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


  I tested the patch, works nicely.
  Just a check to add.
  
  Drag'n drop is quite easy in the use case it covers, so wait for @ngraham 
and/or @elvisangelaccio feedback before merging.

INLINE COMMENTS

> dolphinmainwindow.cpp:1993
> +copyToOtherViewAction->setEnabled(true);
> +moveToOtherViewAction->setEnabled(capabilities.supportsMoving());
> +} else {

This is not sufficient, you can copy or move to `recentlyused:/files/` for 
instance, which fails.
So test the other view url to see if it is writable as well.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#661964 , @dfaure wrote:
  
  > Looks good to me.
  >
  > Did you test copying and moving directories as well? Should work, but just 
to make sure.
  >
  > I'll let the Dolphin developers/maintainers decide on whether the shortcuts 
are OK, and land this.
  
  
  Yes, tested:
  copy/move file & multiple files
  copy/move folder & multiple folder
  copy/move both folder and file & both multiple folders and multiple files
  
  Shall we keep the keyboard shortcuts as `CTRL+F5` for copy and `CTRL+F6` for 
move. Krusader uses `F5` for copy and `F6` for move. So it would be almost same 
(`F5` is refresh in dolphin).
  
  https://kde.org/applications/utilities/org.kde.krusader

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Looks good to me.
  
  Did you test copying and moving directories as well? Should work, but just to 
make sure.
  
  I'll let the Dolphin developers/maintainers decide on whether the shortcuts 
are OK, and land this.

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29006

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81765.
aprcela added a comment.


  - Connect to copyingDone to update m_selectedUrls

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81757=81765

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphinmainwindow.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81757.
aprcela marked 2 inline comments as done.
aprcela added a comment.


  - Instead of append, change the value to a simplifiedUrlList
  - Remove 'selected' from text

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81708=81757

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphinmainwindow.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dolphinview.cpp:1222
>  if (!m_selectedUrls.isEmpty()) {
>  m_selectedUrls << KDirModel::simplifiedUrlList(m_selectedUrls);
>  }

This line of code (from me, it turns out) is bogus, it should say `=` instead 
of `<<`.
We want to replace the list with the simplified list, not concatenate the two.

And m_selectedUrls needs to contain the new URLs, just like we do when pasting.

The solution I have in mind for that is to connect to CopyJob::slotCopyingDone 
and grab the "to" argument.

[discussed on IRC, but writing here for the record, and in case someone wonders 
why this << gets modified]

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dolphinmainwindow.cpp:1414
>  paste->setIconText(i18nc("@action:inmenu Edit", "Paste"));
> -paste->setWhatsThis(xi18nc("@info:whatsthis paste", "This copies the 
> items from "
> +paste->setWhatsThis(xi18nc("@info:whatsthis paste", "This copies the 
> selected items from "
>  "your clipboard to the currently viewed 
> folder."

"selected " is not relevant here : pasting something does not care about your 
selection

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81708.
aprcela marked 2 inline comments as done.
aprcela added a comment.


  - Rename slotPasteJobResult to more generic slotJobResult
  - Added error check when copying to other view

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81573=81708

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphinmainwindow.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  Done :)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dolphinview.cpp:697
> +{
> +KIO::CopyJob* job = KIO::copy(selection.urlList(), destinationPanelUrl, 
> KIO::DefaultFlags);
> +KJobWidgets::setWindow(job, this);

Why is there no error handling for the copy job? It can fail too. (Try having / 
as a destination...)

> dolphinview.cpp:708
> +
> +connect(job, ::DropJob::result, this, 
> ::slotPasteJobResult);
> +

The slot should probably be renamed to something more generic, this isn't about 
pasting anymore.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  @dfaure is it good now? Anyone found anything else to fix and/or improve?

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Yuri Chornoivan]

yurchor added a comment.


  Thanks for updating the docbook. +1

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: yurchor, kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, 
feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, 
emmanuelp, rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81573.
aprcela marked 7 inline comments as done.
aprcela added a comment.


  - Update docbook

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81509=81573

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphinmainwindow.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  In D29006#660096 , @dfaure wrote:
  
  > You can compare with what happens when doing the same invalid move using 
Cut / Paste for instance.
  >
  > The job gives the error "A folder cannot be moved into itself" and dolphin 
displays this in the view.
  >
  > You want to connect to the job's result() signal and emit errorMessage from 
the slot, like DolphinView::slotPasteJobResult does.
  
  
  Done all of that. Tested and it's working as expected with the error showing 
up.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dolphinmainwindow.cpp:1973
> +copyToOtherViewAction->setEnabled(false);
> +moveToOtherViewAction->setEnabled(capabilities.supportsMoving());
>  } else {

What will you move if there are no selected items?

> dolphinmainwindow.cpp:1996
> +copyToOtherViewAction->setEnabled(false);
> +moveToOtherViewAction->setEnabled(capabilities.supportsMoving());
> +}

same here. No split view, the action should be disabled, no?

> dolphinview.cpp:701
> +QList newSelection;
> +if (job) {
> +newSelection << destinationPanelUrl;

job can never be null, this if() can be removed

> dolphinview.cpp:702
> +if (job) {
> +newSelection << destinationPanelUrl;
> +KIO::FileUndoManager::self()->recordCopyJob(job);

unused variable `newSelection`

> dolphinview.cpp:713
> +QList newSelection;
> +if (job) {
> +newSelection << destinationPanelUrl;

same

> dolphinview.cpp:714
> +if (job) {
> +newSelection << destinationPanelUrl;
> +KIO::FileUndoManager::self()->recordCopyJob(job);

same

> dolphinview.h:371
> + */
> +void copySelectedItemsToOtherSplitView(KFileItemList selection, QUrl 
> destinationPanelUrl);
> +

const &for both arguments

(repeats)

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven, dfaure
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[David Faure]

dfaure added a comment.


  You can compare with what happens when doing the same invalid move using Cut 
/ Paste for instance.
  
  The job gives the error "A folder cannot be moved into itself" and dolphin 
displays this in the view.
  
  You want to connect to the job's result() signal and emit errorMessage from 
the slot, like DolphinView::slotPasteJobResult does.

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela updated this revision to Diff 81509.
aprcela added a comment.
Herald added a project: Documentation.
Herald added a subscriber: kde-doc-english.


  - Make moving only available if file is movable.
  - Update docbook

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29006?vs=81505=81509

BRANCH
  arcpatch-D29006

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

AFFECTED FILES
  doc/index.docbook
  src/dolphinmainwindow.cpp
  src/dolphinmainwindow.h
  src/dolphinui.rc
  src/views/dolphinview.cpp
  src/views/dolphinview.h

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov

D29006: Allow to copy or move selection to the other split view

[Antonio Prcela]

aprcela added a comment.


  One more fix and update.
  Is it ok that nothing happens if a user wants to move a folder into one of 
it's subfolders?
  If dolphin is launched from bash, one can see `"Could not rename file 
/tmp/subfolder copy."` if one tries to move `/tmp/subfolder` to 
`/tmp/subfolder/subsub/`

REPOSITORY
  R318 Dolphin

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

To: aprcela, #dolphin, elvisangelaccio, ngraham, meven
Cc: kde-doc-english, dfaure, meven, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, gennad, fbampaloukas, alexde, Codezela, feverfew, 
spoorun, navarromorales, firef, ngraham, andrebarros, skadinna, emmanuelp, 
rdieter, mikesomov