Differential D16830

Add file related actions to Tabbar context menu and File menu: Rename, Delete, Compare (new), and some more
ClosedPublic
Actions

Authored by gregormi on Nov 11 2018, 8:23 PM.

Details

Reviewers
ngraham
cullmann
Group Reviewers
Kate
VDG
Commits
R40:e8496c605592: Add file related actions to Tabbar context menu and File menu: Rename, Delete…
Summary

Add some actions to Tabbar context menu and File menu

New items in Tabbar context menu:

  • 'Rename file'
  • 'Delete file'
  • 'Properties'
  • 'Compare'

'Compare' does the following: compare two files with an external diff tool. It works like this: right click on a tab which is NOT active and select the appropriate menu item (see screenshot). Then, the external tool opens and compares the active document with the one which was clicked on. Currently, three diff tools are supported: kdiff3, kompare and meld.

New items in File main menu:

  • 'Rename file'
  • 'Delete file'
  • 'Compare'
  • 'Copy File Path'
  • 'Open Containing Folder'
  • 'Properties'

Screenshots:

Tab context menu:

Tab context menu when no file is associated:

Tab context menu with the compare feature:

Diff tools not found:

Actions are also available in the main File menu:

NOTE: Some code of kfileactions.cpp was copied and adapted from katefiletree.cpp. TODO: reuse code there. Probably, this means kfileactions.h must be moved to KTextEditor.

TODO: in a later change the 'Open with...' menu item which is currently available in the Projects and FileTree plugin could also be extracted and be added to the Tabbar context menu and File menu (Gwenview has it, too).

Diff Detail

Repository
R40 Kate
Branch
arcpatch-D16830
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5100
Build 5118: arc lint + arc unit
gregormi created this revision.Nov 11 2018, 8:23 PM
Restricted Application added a project: Kate. · View Herald TranscriptNov 11 2018, 8:23 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
gregormi requested review of this revision.Nov 11 2018, 8:23 PM
gregormi retitled this revision from Tabbar: add 'File Properties' menu item to WIP: Tabbar: Add 'File Properties' menu item and two more.Nov 11 2018, 8:27 PM
gregormi edited the summary of this revision. (Show Details)
gregormi added a reviewer: Kate.
gregormi edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.Nov 11 2018, 8:32 PM

Ah, I have wanted these for years!!! Can you make sure these are also in the File menu? We can't have functionality that's only accessible via context menu.

gregormi added a comment.Nov 11 2018, 8:45 PM
In D16830#358084, @ngraham wrote:

Ah, I have wanted these for years!!! Can you make sure these are also in the File menu?

I first would like to wait for consent if the action code is on the right place.

Can you propose the exact place in the File menu where those items should be located? E.g. below "Print Preview"? Maybe in a submenu to avoid cluttering the top level? Maybe remove the separator above "Export as HTML..."?

We can't have functionality that's only accessible via context menu.

What about adding a little menu icon for each tab to make the context menu to be accessed from there? Maybe even replace the Close icon by the new icon and add the shortcut Ctrl+W to the "Close document" menu item?

gregormi updated this revision to Diff 45330.Nov 11 2018, 9:19 PM
gregormi edited the summary of this revision. (Show Details)
  • Tabbar: Add menu item to compare two files with KDiff3
gregormi edited the summary of this revision. (Show Details)Nov 11 2018, 9:21 PM
gregormi edited the summary of this revision. (Show Details)
gregormi retitled this revision from WIP: Tabbar: Add 'File Properties' menu item and two more to WIP: Tabbar: Add 'File Properties' menu item and three more (Rename, Delete and Compare).Nov 11 2018, 9:25 PM
ngraham added a comment.Nov 12 2018, 4:14 AM
In D16830#358090, @gregormi wrote:

Can you propose the exact place in the File menu where those items should be located? E.g. below "Print Preview"? Maybe in a submenu to avoid cluttering the top level? Maybe remove the separator above "Export as HTML..."?

I'd put "Delete" towards the bottom of the menu with the other destructive actions with red icons. The other ones, yeah, could maybe go below Print Preview.

In D16830#358090, @gregormi wrote:

What about adding a little menu icon for each tab to make the context menu to be accessed from there? Maybe even replace the Close icon by the new icon and add the shortcut Ctrl+W to the "Close document" menu item?

That's an interesting idea, but we already have a much more standard way of ensuring that functionality is available: the main menu bar. :)

mmustac added a subscriber: mmustac.Nov 12 2018, 10:02 AM

I would love to see features like this. Specially the compare function would be a great benefit and improve workflows.

TODO: possibility to choose another program (like Kompare or Meld)

I would be one of those persons who use kompare instead of kdiff 3 :-)

dhaumann added a subscriber: dhaumann.Nov 12 2018, 9:38 PM

Unfortunately, someone started with v2 only many years ago, and other reused this license simply due to copy & paste. Nowadays, we have a hard time getting rid of v2 only. Please use v2+.

kate/katefileactions.h
6

Please LGPL v2+

gregormi updated this revision to Diff 45748.Nov 18 2018, 8:56 PM
gregormi edited the summary of this revision. (Show Details)
  • Change license to GPL v2+
  • Add support for three diff tools: kdiff3, kompare and meld
  • Remove KIO_VERSION check because it is no longer needed
gregormi updated this revision to Diff 45749.Nov 18 2018, 9:00 PM
gregormi marked an inline comment as done.
  • LGPLv2+ or later or KDE approved
gregormi edited the summary of this revision. (Show Details)Nov 18 2018, 9:01 PM
gregormi edited the summary of this revision. (Show Details)Nov 18 2018, 9:03 PM
ngraham added a comment.Nov 19 2018, 3:28 AM

Lovely stuff. These items still need to also be in the File menu though. :)

dhaumann added inline comments.Nov 20 2018, 9:08 PM
kate/katefileactions.cpp
51

RAII: here we request a resource without initialization. Correct is:

bool ok = false;

Even if the bool is set in the next line via out parameter. Over time the code may change and then we have an uninitialized variable. :)

93

Here, I would prefer

const auto && url = ...

Reasoning: && uses a reference if the returned value is a reference, otherwise a copy. In this case, it's more what we want in long term (aka it's always correct, even if the API changes).

138

Can't you use QProcess? :) Wasn't KProcess close to deprecated? I think KProcess is only required in rare corner cases nowadays.

gregormi added a comment.Nov 20 2018, 10:39 PM
In D16830#361985, @ngraham wrote:

Lovely stuff. These items still need to also be in the File menu though. :)

Yes, I forgot to mention that this is on the todo list.

gregormi added inline comments.Nov 20 2018, 10:49 PM
kate/katefileactions.cpp
51

Thanks for the review! I didn't look at the copied code very closely so far. My bad.

Though "bool ok;" should be corrected, I woudn't call it RAII: https://en.cppreference.com/w/cpp/language/raii says RAII is about acquiring and releasing resources which exist in limited supply like heap memory, file handles etc. Creating a mere stack variable does not fit this description.

gregormi updated this revision to Diff 45924.Nov 20 2018, 11:01 PM
  • rebase on master
  • Consistent naming
  • apply reviewer's comments pending...
gregormi edited the summary of this revision. (Show Details)Nov 20 2018, 11:06 PM
gregormi updated this revision to Diff 46164.EditedNov 24 2018, 11:25 PM
gregormi edited the summary of this revision. (Show Details)
  • Make actions available from the File menu
  • Move all relevant functions to KateFileActions and reuse them in KateMainWindow and KateViewSpace
  • Use QProcess (todo: error handling)
  • Compare in File menu: show a message box with a hint
  • Cleanups
gregormi marked 2 inline comments as done.Nov 24 2018, 11:27 PM
gregormi edited the summary of this revision. (Show Details)Nov 24 2018, 11:30 PM
gregormi edited the summary of this revision. (Show Details)Nov 24 2018, 11:46 PM
ngraham added a comment.Nov 24 2018, 11:50 PM

TODO: Instead of "System" the new menu could be named "Document File"?

The whole file menu has actions that act on the current file; instead of putting all the new actions in a submenu, I'd recommend putting them all in the base level of the menu itself in appropriate places, and then we can work on consolidating and simplifying in your other patch. You might look at Gwenview's File menu for inspiration, since it already has most of these actions in it.

anthonyfieroni added a subscriber: anthonyfieroni.Nov 25 2018, 7:04 AM
anthonyfieroni added inline comments.
kate/data/kateui.rc
3 ↗(On Diff #46164)

Increase version

dhaumann added a comment.Nov 26 2018, 9:52 PM

In general I have no objections to this patch. I'd welcone another review, though.

kate/katemainwindow.cpp
734 ↗(On Diff #46164)

move down:

const bool hasUrl = view && !view->document()->url().isEmpty();

This way you don't need an if at all, thr code is more compact and more readable.

gregormi updated this revision to Diff 46682.Dec 2 2018, 11:14 AM
gregormi edited the summary of this revision. (Show Details)
  • Put new File items at base level similar to Gwenview
  • rc file: increase version
  • simplify code
gregormi edited the summary of this revision. (Show Details)Dec 2 2018, 11:19 AM
gregormi updated this revision to Diff 46690.Dec 2 2018, 12:01 PM
  • Simplify code; error handling
gregormi updated this revision to Diff 46691.Dec 2 2018, 12:15 PM
  • assertions and comments
gregormi retitled this revision from WIP: Tabbar: Add 'File Properties' menu item and three more (Rename, Delete and Compare) to Add file related actions to Tabbar context menu and File menu: Rename, Delete, Compare (new), and some more.Dec 2 2018, 12:22 PM
gregormi edited the summary of this revision. (Show Details)
gregormi edited the summary of this revision. (Show Details)
gregormi updated this revision to Diff 46694.Dec 2 2018, 12:30 PM
  • add comments
gregormi edited the summary of this revision. (Show Details)Dec 2 2018, 12:35 PM
ngraham accepted this revision.Dec 2 2018, 6:29 PM
ngraham added a reviewer: VDG.

I'll let the Kate devs do the code review and have the last word, but looks great to me UI/UX-wise! I can't wait to have these features in Kate.

This revision is now accepted and ready to land.Dec 2 2018, 6:30 PM
gregormi added a subscriber: cullmann.Dec 2 2018, 8:20 PM
gregormi added inline comments.
kate/katefileactions.cpp
43–51 ↗(On Diff #45749)

Hello @dhaumann @cullmann,
I wonder if it would be better for the functions in this file to use a Document reference instead of a pointer. Currently, these functions will crash on a nullptr. In a function farther down below I used assertions which could be removed if I use references instead of pointers. This makes the code more readable and safe (and seems the recommended way to pass parameters: http://www.modernescpp.com/index.php/c-core-guidelines-how-to-pass-function-parameters).
What do you think?

gregormi marked 3 inline comments as done.Dec 2 2018, 8:22 PM
cullmann accepted this revision.Dec 3 2018, 6:44 PM

For the pointer vs. ref: as we use close to everywhere pointers ATM and nearly all places need non-nullptr ones I am not sure one should introduce refs in other places.

Please commit as is,the feature is nice, thanks for the work!

Closed by commit R40:e8496c605592: Add file related actions to Tabbar context menu and File menu: Rename, Delete… (authored by gregormi). · Explain WhyDec 3 2018, 8:37 PM
This revision was automatically updated to reflect the committed changes.

Showing Only Differences

This revision modifies 2 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

PathPackages
AMkate/katefileactions.h (80 lines)
AMkate/katefileactions.cpp (141 lines)

Diff 45752

View Options

kate/katefileactions.h

Loading...
View Options

kate/katefileactions.cpp

Loading...