D22199: New plugin to open the selected file path
nononux marked 2 inline comments as done. nononux added inline comments. INLINE COMMENTS > dhaumann wrote in 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. As a class cannot inherit from 2 QObjects, I've made another class included in the plugin as a member. REPOSITORY R40 Kate REVISION DETAIL https://phabricator.kde.org/D22199 To: nononux, dhaumann Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux updated this revision to Diff 62250. nononux added a comment. Fix the required parts, I hope it's ok this time :) REPOSITORY R40 Kate CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22199?vs=61266=62250 BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux, dhaumann Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
dhaumann requested changes to this revision.
dhaumann added a comment.
This revision now requires changes to proceed.
The KTextEditor::Command implementation is wrong, see my other comments.
Please fix this first :-)
INLINE COMMENTS
> plugin_kateopenselection.cpp:80
> +{
> +return new PluginViewKateOpenSelection(this,mainWindow);
> +}
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.
> plugin_kateopenselection.cpp:107
> +if (selection.isEmpty()) {
> +const KTextEditor::Cursor = editView->cursorPosition();
> +const QString line = editView->document()->line(cursor.line());
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.
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux, dhaumann
Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad,
fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux added a comment. Hi, can the code be pushed ? I don't think there are still changes to do. (I don't have a dev account) REPOSITORY R40 Kate REVISION DETAIL https://phabricator.kde.org/D22199 To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux marked 2 inline comments as done. nononux added inline comments. INLINE COMMENTS > pino wrote in plugin_kateopenselection.cpp:58 > This should be "Open Selected Paths"; see > https://hig.kde.org/style/writing/capitalization.html 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. > pino wrote in plugin_kateopenselection.cpp:134 > 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. 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 ? > pino wrote in plugin_kateopenselection.cpp:135 > 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. I changed the code to support http://-like paths. REPOSITORY R40 Kate REVISION DETAIL https://phabricator.kde.org/D22199 To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux updated this revision to Diff 61266. nononux added a comment. Add support of remote paths (http://) + change action name REPOSITORY R40 Kate CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22199?vs=61239=61266 BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
pino added inline comments.
INLINE COMMENTS
> plugin_kateopenselection.cpp:58
> +QAction *a =
> actionCollection()->addAction(QStringLiteral("file_openselection"));
> +a->setText(i18n("Open selected path"));
> +actionCollection()->setDefaultShortcut(a, QKeySequence(Qt::ALT +
> Qt::Key_O));
This should be "Open Selected Paths"; see
https://hig.kde.org/style/writing/capitalization.html
> nononux wrote in plugin_kateopenselection.cpp:134
> 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).
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.
> nononux wrote in plugin_kateopenselection.cpp:135
> 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 //.
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.
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux
Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad,
fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux updated this revision to Diff 61239. nononux added a comment. Change the shortcut to Alt+O as Ctrl+Shift+O was already used for the spelling correction REPOSITORY R40 Kate CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22199?vs=61027=61239 BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux marked 9 inline comments as done. REPOSITORY R40 Kate REVISION DETAIL https://phabricator.kde.org/D22199 To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux updated this revision to Diff 61026. nononux added a comment. Improve coding guidelines respect REPOSITORY R40 Kate CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22199?vs=61022=61026 BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
yurchor added inline comments. INLINE COMMENTS > nononux wrote in kateopenselectionplugin.desktop:6-7 > I removed all the translations. Is it a dedicated team who do the > translations ? 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. REPOSITORY R40 Kate REVISION DETAIL https://phabricator.kde.org/D22199 To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux marked 11 inline comments as done.
nononux added a comment.
Thanks for your advices for my first KDE dev. I hope I've taken them into
account in a good way.
INLINE COMMENTS
> pino wrote in kateopenselectionplugin.desktop:6-7
> Please do not add translations manually, there is a KDE-wide system to handle
> them.
I removed all the translations. Is it a dedicated team who do the translations ?
> pino wrote in plugin_kateopenselection.cpp:59
> 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
I've put "Open selected path" as it seems the others texts in my menu doesn't
have all first letters capitalized.
> pino wrote in plugin_kateopenselection.cpp:114
> 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.
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 ?
> pino wrote in plugin_kateopenselection.cpp:134
> Excluding the newline in the code above (see my `isValidChar()` suggestion)
> can avoid the need to check for newline here.
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).
> pino wrote in plugin_kateopenselection.cpp:135
> This assumes the string is a local file -- what if under the cursor there is
> a remote URL?
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 //.
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux
Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad,
fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
nononux updated this revision to Diff 61022. nononux added a comment. Taking into account the proposed changes REPOSITORY R40 Kate CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22199?vs=60958=61022 BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
dhaumann added inline comments.
INLINE COMMENTS
> plugin_kateopenselection.cpp:91
> +KTextEditor::View* editView =
> application->activeMainWindow()->activeView();
> +if (editView && editView->document()) {
> +QString selection;
A view always has a valid document, no need to check editView->document()
> plugin_kateopenselection.cpp:107
> +const KTextEditor::Cursor = editView->cursorPosition();
> +const QString = editView->document()->line(cursor.line());
> +const int lineLength = line.size();
QString is implicitly shared. Please remove the '&' here.
> plugin_kateopenselection.cpp:112
> +const int col = cursor.column();
> +int start=col, end=col;
> +// Stop at first white space or quote (for path with space,
> user should select the text)
In general: Please use spaces around operators and adhere to the standard KDE
coding guidelines (also used in Kate everywhere else).
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux
Cc: dhaumann, pino, yurchor, kwrite-devel, kde-doc-english, gennad,
fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars
D22199: New plugin to open the selected file path
pino added a comment.
In addition to the assumption done about a path being a local file: what if
the selection contains more than one path/URL?
INLINE COMMENTS
> kateopenselectionplugin.desktop:6-7
> +Name=Open Selection
> +Name[en_GB]=Open Selection
> +Name[fr]=Ouvrir la sélection
> +Comment=Opens the selected path
Please do not add translations manually, there is a KDE-wide system to handle
them.
> kateopenselectionplugin.desktop:9
> +Comment=Opens the selected path
> +Comment[fr]=Ouvre le chemin sélectionné
ditto
> plugin_kateopenselection.cpp:47
> +K_PLUGIN_FACTORY_WITH_JSON(KateOpenSelectionFactory,"kateopenselectionplugin.json",
> registerPlugin();)
> +//K_EXPORT_PLUGIN(KateOpenSelectionFactory(KAboutData("kateopenselection","kateopenselection",ki18n("Open
> Selection"), "0.1", ki18n("Open selection for a source file"),
> KAboutData::License_LGPL_V2)) )
> +
I guess there is no need for commented code in new code.
> yurchor wrote in plugin_kateopenselection.cpp:59
> 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".
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
> plugin_kateopenselection.cpp:100
> +if (selection.endsWith(QLatin1Char('\n'))) {
> +selection = selection.left(selection.size() -1);
> +}
`selection.chop(1)` is easier. Even better, directly trim the string to remove
all whitespaces at the beginning and at the end.
> plugin_kateopenselection.cpp:114
> +// Stop at first white space or quote (for path with space,
> user should select the text)
> +while(start > 0 && (c = line.at(start-1)) != QLatin1Char('
> ') && c != QLatin1Char('\t') && c != QLatin1Char('"') && c !=
> QLatin1Char('\'')) {
> +--start;
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.
> plugin_kateopenselection.cpp:134
> +// Open the file
> +if (!selection.isEmpty() && !selection.contains(QLatin1Char('\n'))) {
> +const QUrl url = QUrl::fromLocalFile(selection);
Excluding the newline in the code above (see my `isValidChar()` suggestion) can
avoid the need to check for newline here.
> plugin_kateopenselection.cpp:135
> +if (!selection.isEmpty() && !selection.contains(QLatin1Char('\n'))) {
> +const QUrl url = QUrl::fromLocalFile(selection);
> +if (fileExists(url)) {
This assumes the string is a local file -- what if under the cursor there is a
remote URL?
> ui.rc:3
> +
> + version="5" translationDomain="kateopenselection">
> +
Since it is a new plugin, I'd start with version=1.
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux
Cc: pino, yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson,
michaelh, ngraham, demsking, skadinna, cullmann, sars, dhaumann
D22199: New plugin to open the selected file path
yurchor added a comment.
Thanks in advance for fixing the issue with docs.
INLINE COMMENTS
> plugin_kateopenselection.cpp:59
> +QAction *a =
> actionCollection()->addAction(QStringLiteral("file_openselection"));
> +a->setText(i18n("Opens the selected path"));
> +actionCollection()->setDefaultShortcut(a, QKeySequence(Qt::CTRL +
> Qt::SHIFT + Qt::Key_O));
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".
> plugins.docbook:1538
> +
> +CTRL-Shift-O
> +
Should be
O
REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D22199
To: nononux
Cc: yurchor, kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson,
michaelh, ngraham, demsking, skadinna, cullmann, sars, dhaumann
D22199: New plugin to open the selected file path
nononux created this revision. Herald added projects: Kate, Documentation. Herald added subscribers: kde-doc-english, kwrite-devel. nononux requested review of this revision. REVISION 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. REPOSITORY R40 Kate BRANCH openselection (branched from master) REVISION DETAIL https://phabricator.kde.org/D22199 AFFECTED FILES addons/CMakeLists.txt addons/openselection/CMakeLists.txt addons/openselection/Messages.sh addons/openselection/kateopenselectionplugin.desktop addons/openselection/plugin.qrc addons/openselection/plugin_kateopenselection.cpp addons/openselection/plugin_kateopenselection.h addons/openselection/rc.cpp addons/openselection/ui.rc doc/kate/plugins.docbook To: nononux Cc: kwrite-devel, kde-doc-english, gennad, fbampaloukas, domson, michaelh, ngraham, demsking, skadinna, cullmann, sars, dhaumann
