Differential D27998

[KCMs/Workspace] Add option for dndToMove
AbandonedPublic
Actions

Authored by trmdi on Mar 12 2020, 4:44 AM.

Details

Reviewers
ngraham
Group Reviewers
VDG
Plasma
Summary

Add GUI for the new dndToMove option.
Depends on D27951

Test Plan

It writes to ~/.config/kdeglobals properly.

Diff Detail

Repository
R119 Plasma Desktop
Branch
add-dndToMove (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23708
Build 23726: arc lint + arc unit
trmdi created this revision.Mar 12 2020, 4:44 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 12 2020, 4:44 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
trmdi requested review of this revision.Mar 12 2020, 4:44 AM
trmdi edited the test plan for this revision. (Show Details)Mar 12 2020, 4:47 AM
meven added a subscriber: meven.Mar 12 2020, 8:15 AM
meven added inline comments.
kcms/workspaceoptions/package/contents/ui/main.qml
150

You can nowadays write !kcm.globalsSettings.isDndToMoveImmutable
That was added recently. D26368

trmdi updated this revision to Diff 77509.Mar 12 2020, 3:23 PM

Move to bottom, add hint

trmdi edited the test plan for this revision. (Show Details)Mar 12 2020, 3:26 PM
trmdi added inline comments.
kcms/workspaceoptions/package/contents/ui/main.qml
150

Thanks, but maybe this need another patch. :)

ngraham retitled this revision from Add workspace kcm option for dndToMove to [KCMs/Workspace] Add option for dndToMove.Mar 12 2020, 3:36 PM
ngraham edited the summary of this revision. (Show Details)
ngraham added a dependency: D27951: Allow users to change dropAction to MoveAction through kdeglobals.
meven added inline comments.Mar 13 2020, 7:55 AM
kcms/workspaceoptions/package/contents/ui/main.qml
189

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.

ngraham accepted this revision.Mar 13 2020, 9:19 PM

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

kcms/workspaceoptions/package/contents/ui/main.qml
206

Maybe "Hold Shift when dropping to show drop options"

kcms/workspaceoptions/workspaceoptions_kdeglobalssettings.kcfg
13

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

This revision is now accepted and ready to land.Mar 13 2020, 9:19 PM
bugseforuns added a subscriber: bugseforuns.Mar 14 2020, 11:58 AM

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.

ngraham added a comment.Mar 14 2020, 5:03 PM
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).

trmdi added a comment.Mar 15 2020, 12:51 AM

Yes, it means "partition".

trmdi added a comment.Mar 15 2020, 12:54 AM
In D27998#627603, @ngraham wrote:

What does?

dnd from /dev/sda1 -> /dev/sda1 : move
dnd from /dev/sda1 -> /dev/sda2 : show menu

ngraham added a comment.Mar 15 2020, 12:55 AM

Right.

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

trmdi added a comment.Mar 15 2020, 2:52 AM
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.

ngraham added a comment.Mar 15 2020, 2:53 AM

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

trmdi updated this revision to Diff 77643.Mar 15 2020, 2:54 AM

Nate's comment

trmdi marked 2 inline comments as done.Mar 15 2020, 2:54 AM
ngraham added inline comments.Mar 15 2020, 2:55 AM
kcms/workspaceoptions/package/contents/ui/main.qml
204

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

trmdi added a comment.Mar 15 2020, 2:55 AM
In D27998#627623, @ngraham wrote:

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

On the left panel.

ngraham added a comment.Mar 15 2020, 2:56 AM

Hmm, that's true. How awkward.

trmdi added inline comments.Mar 15 2020, 2:58 AM
kcms/workspaceoptions/package/contents/ui/main.qml
204

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

ngraham added inline comments.Mar 15 2020, 3:03 AM
kcms/workspaceoptions/package/contents/ui/main.qml
204

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.

trmdi updated this revision to Diff 77644.Mar 15 2020, 3:10 AM

Nate's comment

trmdi marked 3 inline comments as done.Mar 15 2020, 3:11 AM
trmdi abandoned this revision.Apr 5 2020, 3:27 AM

Revision Contents
Changeset List

Diff 77644

View Options

kcms/workspaceoptions/package/contents/ui/main.qml

Loading...
View Options

kcms/workspaceoptions/workspaceoptions_kdeglobalssettings.kcfg

Loading...