D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi updated this revision to Diff 77644.
trmdi added a comment.


  Nate's comment

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27998?vs=77643=77644

BRANCH
  add-dndToMove (branched from master)

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

AFFECTED FILES
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/workspaceoptions_kdeglobalssettings.kcfg

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added inline comments.

INLINE COMMENTS

> trmdi wrote in main.qml:204
> If there is a new stuff below this, when you make it visible, the below stuff 
> will jump up.

That already happens with the other labels though. Implementing this behavior 
here would make it inconsistent with the other ones. If we don't want the UI 
jumping around, we should change that in another patch to that this one isn't 
inconsistent with the current state.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi added inline comments.

INLINE COMMENTS

> ngraham wrote in main.qml:204
> I would make this label non-visible when `dndToMoveEnabler.checked` is false, 
> rather than changing its text.

If there is a new stuff below this, when you make it visible, the below stuff 
will jump up.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added a comment.


  Hmm, that's true. How awkward.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi added a comment.


  In D27998#627623 , @ngraham wrote:
  
  > Where? Being wrong in multiple places doesn't make it right. ;)
  
  
  On the left panel.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added inline comments.

INLINE COMMENTS

> main.qml:204
> +
> +QQC2.Label {
> +Layout.fillWidth: true

I would make this label non-visible when `dndToMoveEnabler.checked` is false, 
rather than changing its text.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi updated this revision to Diff 77643.
trmdi added a comment.


  Nate's comment

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27998?vs=77509=77643

BRANCH
  add-dndToMove (branched from master)

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

AFFECTED FILES
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/workspaceoptions_kdeglobalssettings.kcfg

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added a comment.


  Where? Being wrong in multiple places doesn't make it right. ;)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi added a comment.


  In D27998#627605 , @ngraham wrote:
  
  > Right.
  >
  > @bugseforuns's objection is that you used the word "device" to mean 
"partition" in the UI.
  
  
  Both Dolphin and Qt use this word.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added a comment.


  Right.
  
  @bugseforuns's objection is that you used the word "device" to mean 
"partition" in the UI.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi added a comment.


  In D27998#627603 , @ngraham wrote:
  
  > What does?
  
  
  dnd from /dev/sda1 -> /dev/sda1 : move
  dnd from /dev/sda1 -> /dev/sda2 : show menu

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added a comment.


  What does?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Tranter Madi 
trmdi added a comment.


  Yes, it means "partition".

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Nathaniel Graham 
ngraham added a comment.


  In D27998#627401 , @bugseforuns 
wrote:
  
  > Will popup menu show up if "Move files if on the same device" is selected 
and files are dragged to another partition on the same device?
  >  If so, "Move files if on the same device" label is not accurate.
  
  
  Good point. Maybe it needs to say "Move files if on the same partition or 
device" (yes, "device" is a bit redundant if you're already saying "partition", 
but "device" is a less technical term).

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-14 Thread Patrick Silva 
bugseforuns added a comment.


  Will popup menu show up if "Move files if on the same device" is selected and 
files are dragged to another partition on the same device?
  If so, "Move files if on the same device" label is not accurate.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: bugseforuns, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-13 Thread Nathaniel Graham 
ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  LGTM, modulo some small string change requests. Obviously this can't go in 
until and unless the dependent patch lands. :)

INLINE COMMENTS

> main.qml:206
> +Layout.fillWidth: true
> +text: dndToMoveEnabler.checked ? i18n("Hold Shift when dropping 
> to show the menu") : i18n("")
> +elide: Text.ElideRight

Maybe "Hold Shift when dropping to show drop options"

> workspaceoptions_kdeglobalssettings.kcfg:13
> +
> +  Drag and drop files will move files
> +  false

No reason to be terse here; this should be as descriptive as needed. For 
example:

For local files on the same device, whether dragging and dropping will perform 
a move operation instead of showing the options menu

REPOSITORY
  R119 Plasma Desktop

BRANCH
  add-dndToMove (branched from master)

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

To: trmdi, #vdg, #plasma, ngraham
Cc: meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27998: [KCMs/Workspace] Add option for dndToMove

2020-03-13 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> main.qml:189
> +text: i18n("Always ask what to do")
> +enabled: !kcm.globalsSettings.isImmutable("dndToMove")
> +checked: !kcm.globalsSettings.dndToMove

My comment "you can nowadays write `!kcm.globalsSettings.isDndToMoveImmutable`" 
was about this line that you moved before seeing my comment I guess.
Same applies line 198.

REPOSITORY
  R119 Plasma Desktop

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

To: trmdi, #vdg, #plasma, ngraham
Cc: meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart