Differential D17596

[KDirOperator] Allow renaming files from the context menu
ClosedPublic
Actions

Authored by ngraham on Dec 15 2018, 2:56 AM.

Details

Reviewers
meven
Group Reviewers
Frameworks
Dolphin
Commits
R241:5ee3e7bb4797: [KDirOperator] Allow renaming files from the context menu
Summary

BUG: 189482
FIXED-IN: 5.67

Depends on D17595

Test Plan

  • Using the menu item works to rename one or more items
  • The menu item is disables when there's no selection
  • This menu item is inappropriately enabled for items that cannot be deleted, just like the trash/delete menu item (pre-existing bug, will fix in another patch)

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6043
Build 6061: arc lint + arc unit
ngraham created this revision.Dec 15 2018, 2:56 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 15 2018, 2:57 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 15 2018, 2:57 AM
ngraham edited the summary of this revision. (Show Details)Dec 15 2018, 2:57 AM
ngraham edited the test plan for this revision. (Show Details)
ngraham added a dependency: D17595: Upstream Dolphin's file rename dialog.
dhaumann added a subscriber: dhaumann.EditedDec 15 2018, 8:54 AM

Not sure about binary compatibility, since this is an exported class / public API.

src/filewidgets/kdiroperator.h
774

Adding a virtual function changes the vtable, so this is BIC (binary incompatible). Does the function need to be virtual?

markuss added a subscriber: markuss.EditedDec 15 2018, 1:05 PM

Before the feature was removed, the window had all file operations, incl. move to trash and IIRC even the ability to cut/copy/paste and move (via D'n'D) files.

elvisangelaccio added a subscriber: elvisangelaccio.Dec 15 2018, 4:17 PM
elvisangelaccio added inline comments.
src/filewidgets/kdiroperator.cpp
907–909

I think we should just use dialog->open() here. I see you've copied these 3 lines from Dolphin, but we should also fix it there (the rename dialog should really be modal).

ngraham updated this revision to Diff 47624.Dec 15 2018, 4:32 PM
ngraham marked 2 inline comments as done.

Address review comments

ngraham added a comment.Dec 15 2018, 4:33 PM
In D17596#377423, @markuss wrote:

Before the feature was removed, the window had all file operations, incl. move to trash and IIRC even the ability to cut/copy/paste and move (via D'n'D) files.

Move to Trash is already there. Cut/Copy/Paste shouldn't be too hard to add--in another patch of course. :)

dhaumann added inline comments.Dec 15 2018, 5:00 PM
src/filewidgets/kdiroperator.cpp
907–909

Does dialog->open() block? If so, do we have to new the dialog at all? (Only reason I can think of is some DBus crash, see https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0 but that would require a better fix).

elvisangelaccio added inline comments.Dec 15 2018, 5:22 PM
src/filewidgets/kdiroperator.cpp
907–909

Nope, dialog->open() does not block.

emateli added a subscriber: emateli.Dec 15 2018, 9:41 PM

IMO it would be great if this would just rename inline rather than opening a dialog.

ngraham added a comment.Dec 15 2018, 9:43 PM

That's currently an option in Dolphin. Maybe we should just read that setting to see if we should do it here too?

Either way I'd prefer to do that in a separate patch.

markuss added a comment.Dec 15 2018, 10:40 PM
In D17596#377668, @ngraham wrote:

That's currently an option in Dolphin. Maybe we should just read that setting to see if we should do it here too?

Sounds like a good idea.

cfeck added a subscriber: cfeck.Jan 9 2019, 2:47 AM

What is the status of this patch? It missed the 5.54 deadline, so the version would need to be adjusted.

ngraham added a comment.EditedJan 10 2019, 5:15 PM

Sorry, I lost track of this after large changes were requested for the dependent patch D17595. I'll try to get this in for 5.55.

ngraham edited the summary of this revision. (Show Details)Jan 10 2019, 5:15 PM
ngraham updated this revision to Diff 72641.Jan 2 2020, 7:11 PM

Update @since

meven added a subscriber: meven.Jan 6 2020, 1:09 PM
meven added inline comments.
src/filewidgets/kdiroperator.h
772

Update to @since 5.67

meven requested changes to this revision.Jan 6 2020, 1:10 PM
meven edited the summary of this revision. (Show Details)

Don't forget to amend commit message (I have update the FIXED-IN: value).

This revision now requires changes to proceed.Jan 6 2020, 1:11 PM
ngraham updated this revision to Diff 72897.Jan 6 2020, 3:24 PM
ngraham marked an inline comment as done.

Fix @since value

ngraham added a comment.Jan 6 2020, 3:24 PM

@since value needs to be updated.

ngraham marked 2 inline comments as done.Jan 6 2020, 3:24 PM
meven accepted this revision as: meven.Jan 6 2020, 3:40 PM
meven added a comment.Jan 6 2020, 7:48 PM

D17595 just landed, just tested this patch

This revision was not accepted when it landed; it landed in state Needs Review.Jan 6 2020, 9:24 PM
Closed by commit R241:5ee3e7bb4797: [KDirOperator] Allow renaming files from the context menu (authored by ngraham). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 47607

View Options

src/filewidgets/kdiroperator.h

Loading...
View Options

src/filewidgets/kdiroperator.cpp

Loading...