New plugin to open the selected file path
Needs ReviewPublic

Authored by nononux on Jul 1 2019, 7:30 PM.

Details

Reviewers
dhaumann
Summary

The plugin allow the user to open the selected file path in the current document. If there is no selection, the plugin try to detect the path under the cursor.
I've see that functionnality in the editor called "nedit" and find it great as I often manipulate "address files" containing a list of paths.

Diff Detail

Repository
R40 Kate
Branch
openselection (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13531
Build 13549: arc lint + arc unit
nononux created this revision.Jul 1 2019, 7:30 PM
Restricted Application added projects: Kate, Documentation. · View Herald TranscriptJul 1 2019, 7:30 PM
Restricted Application added subscribers: kde-doc-english, kwrite-devel. · View Herald Transcript
nononux requested review of this revision.Jul 1 2019, 7:30 PM
yurchor added a subscriber: yurchor.Jul 2 2019, 4:25 AM

Thanks in advance for fixing the issue with docs.

addons/openselection/plugin_kateopenselection.cpp
60

This seems to be a non-standard way to define a menu item. The other items are just verbs "Open", "Save, not "Opens" or "Saves".

doc/kate/plugins.docbook
1538

Should be

<keycombo>&Ctrl;&Shift;<keycap>O</keycap></keycombo>

pino added a subscriber: pino.Jul 2 2019, 5:16 AM

In addition to the assumption done about a path being a local file: what if the selection contains more than one path/URL?

addons/openselection/kateopenselectionplugin.desktop
7–8

Please do not add translations manually, there is a KDE-wide system to handle them.

10

ditto

addons/openselection/plugin_kateopenselection.cpp
48

I guess there is no need for commented code in new code.

60

In addition to what Yuri said:a better action text, more in line with our HIG [1] is IMHO "Open Selected Path".
"Opens the selected path" can be a good tooltip or whatsthis text.

[1] https://hig.kde.org/style/writing/index.html

101

selection.chop(1) is easier. Even better, directly trim the string to remove all whitespaces at the beginning and at the end.

115

This condition (and the same below for end) is hard to read, as it mixes checks and an assignment mid-way.
A suggestion to make it simpler, and also avoid the duplicated checks is to put the character checks in a small helper:

static bool isValidChar(QChar c)
{
  return c != QLatin1Char(' ') && c != QLatin1Char('\t') && c != QLatin1Char('"') && c != QLatin1Char('\'');
}

and thus using it:

while (start > 0 && isValidChar(line.at(start - 1))

Way more readable IMHO.

Also, most probably other characters can be excluded from what is a file path -- for example word boundaries, newlines, etc. Check the QChar API documentation to see whether there are character classes that can help here.

135

Excluding the newline in the code above (see my isValidChar() suggestion) can avoid the need to check for newline here.

136

This assumes the string is a local file -- what if under the cursor there is a remote URL?

addons/openselection/ui.rc
4

Since it is a new plugin, I'd start with version=1.

dhaumann added inline comments.
addons/openselection/plugin_kateopenselection.cpp
92

A view always has a valid document, no need to check editView->document()

108

QString is implicitly shared. Please remove the '&' here.

113

In general: Please use spaces around operators and adhere to the standard KDE coding guidelines (also used in Kate everywhere else).

nononux updated this revision to Diff 61022.Jul 2 2019, 6:14 PM

Taking into account the proposed changes

nononux marked 11 inline comments as done.Jul 2 2019, 6:36 PM

Thanks for your advices for my first KDE dev. I hope I've taken them into account in a good way.

addons/openselection/kateopenselectionplugin.desktop
7–8

I removed all the translations. Is it a dedicated team who do the translations ?

addons/openselection/plugin_kateopenselection.cpp
60

I've put "Open selected path" as it seems the others texts in my menu doesn't have all first letters capitalized.

115

Done, I use isSpace() to capture more characters (\n, \r, ...). Maybe it is better to put the function in the class as a private function ?

135

I think the test should be kept : if a multi-line text is selected, it can contain a newline (not removed by the trimmed function).

136

The name of the function 'fromLocalFile' is a bit ambiguous. The QUrl doc (https://doc.qt.io/qt-5/qurl.html#fromLocalFile) says this function work with remote files, with a path starting with //.

yurchor added inline comments.Jul 2 2019, 6:46 PM
addons/openselection/kateopenselectionplugin.desktop
7–8

Sure. We (translation team) do.

https://l10n.kde.org/teams-list.php

Then scripty (our friendly translation script) put the translations where they should be automatically.

nononux updated this revision to Diff 61026.Jul 2 2019, 7:17 PM

Improve coding guidelines respect

nononux updated this revision to Diff 61027.Jul 2 2019, 7:21 PM

Correction, forgot to test before send

nononux marked 9 inline comments as done.Jul 5 2019, 7:21 PM
nononux updated this revision to Diff 61239.Jul 5 2019, 7:22 PM

Change the shortcut to Alt+O as Ctrl+Shift+O was already used for the spelling correction

pino added inline comments.Jul 6 2019, 3:31 AM
addons/openselection/plugin_kateopenselection.cpp
59

This should be "Open Selected Paths"; see https://hig.kde.org/style/writing/capitalization.html

135

This is more general thing to take into account, as I mentioned in another comment: what if the selection contains more than two paths? A space or a newline between them does not make much difference.

136

No, QUrl::fromLocalFile() always assumes it is a local file, using a local "file" protocol. The "remote files" mentioned there are simply Qt things, not a general stuff -- it will not handle http://www.kde.org/file.txt at all.

nononux updated this revision to Diff 61266.Jul 7 2019, 8:04 AM

Add support of remote paths (http://) + change action name

nononux marked 2 inline comments as done.Jul 7 2019, 8:12 AM
nononux added inline comments.
addons/openselection/plugin_kateopenselection.cpp
59

Ok, changed. In fact, menus are not capitalized in my native language, but they are with "LANG=en_US.UTF-8 kate", hence the misunderstanding.

135

It won't open anything if there are more than one path in the selection (the file existence test will fail). I don't think it's possible to manage multiple paths without doing important assumptions on separators. Maybe you have a good idea to manage them ?

136

I changed the code to support http://-like paths.

Hi, can the code be pushed ? I don't think there are still changes to do.
(I don't have a dev account)

dhaumann requested changes to this revision.Jul 21 2019, 7:09 PM

The KTextEditor::Command implementation is wrong, see my other comments. Please fix this first :-)

addons/openselection/plugin_kateopenselection.cpp
81

A PluginViewKateOpenSelection instance is created here for every KTextEditor::MainWindow - this is ok and works as designed / intended.

However, PluginViewKateOpenSelection inherits KTextEditor::Command, which itself registers in its constructor with the command "open-selection" in the Kate's command line. Now when you create multiple MainWindows in Kate (View > New Window), then you will add *the same* "open-selection" command twice, which is wrong.

In short: PluginViewKateOpenSelection must not inherit KTextEditor::Command. Instead, the PluginKateOpenSelection should inherit it.

108

Please factor this code part out into a separate function: QString findPathAtCursor(...).

Reasoning: Currently, you are mixing different levers of logic, i.e. slotOpenSelection() should open the selection, but not itself do the work to find the path under a cursor.

This revision now requires changes to proceed.Jul 21 2019, 7:09 PM
nononux updated this revision to Diff 62250.Jul 21 2019, 10:08 PM

Fix the required parts, I hope it's ok this time :)

nononux marked 2 inline comments as done.Jul 21 2019, 10:11 PM
nononux added inline comments.
addons/openselection/plugin_kateopenselection.cpp
81

As a class cannot inherit from 2 QObjects, I've made another class included in the plugin as a member.