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.
Details
- Reviewers
dhaumann
Diff Detail
- Repository
- R40 Kate
- Branch
- openselection (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 13670 Build 13688: arc lint + arc unit
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> |
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". | |
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. 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. |
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). |
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 //. |
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. |
Change the shortcut to Alt+O as Ctrl+Shift+O was already used for the spelling correction
addons/openselection/plugin_kateopenselection.cpp | ||
---|---|---|
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. | |
58 ↗ | (On Diff #61239) | This should be "Open Selected Paths"; see https://hig.kde.org/style/writing/capitalization.html |
addons/openselection/plugin_kateopenselection.cpp | ||
---|---|---|
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. | |
58 ↗ | (On Diff #61239) | 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. |
Hi, can the code be pushed ? I don't think there are still changes to do.
(I don't have a dev account)
The KTextEditor::Command implementation is wrong, see my other comments. Please fix this first :-)
addons/openselection/plugin_kateopenselection.cpp | ||
---|---|---|
80 | 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. | |
107 | 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. |
addons/openselection/plugin_kateopenselection.cpp | ||
---|---|---|
80 | As a class cannot inherit from 2 QObjects, I've made another class included in the plugin as a member. |