Differential D22199 Diff 61027 addons/openselection/plugin_kateopenselection.cpp

Changeset View

Standalone View

View Options

addons/openselection/plugin_kateopenselection.cpp

  • This file was added.
1​/* This file is part of the KDE project
2​ Copyright (C) 2019 Arnaud Ruiz
3​
4​ Created from large parts of:
5​ - openheader plugin
6​ Copyright (C) 2001 Joseph Wenninger
7​ Copyright (C) 2009 Erlend Hamberg <ehamberg@gmail.com>
8​ - search plugin
9​ Copyright (C) 2011-2013 by Kåre Särs <kare.sars@iki.fi>
10​
11​ This library is free software; you can redistribute it and/or
12​ modify it under the terms of the GNU Library General Public
13​ License version 2 as published by the Free Software Foundation.
14​
15​ This library is distributed in the hope that it will be useful,
16​ but WITHOUT ANY WARRANTY; without even the implied warranty of
17​ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
18​ Library General Public License for more details.
19​
20​ You should have received a copy of the GNU Library General Public License
21​ along with this library; see the file COPYING.LIB. If not, write to
22​ the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
23​ Boston, MA 02110-1301, USA.
24​*/
25​
26​#include "plugin_kateopenselection.h"
27​
28​#include <ktexteditor/editor.h>
29​#include <ktexteditor/view.h>
30​#include <ktexteditor/document.h>
31​#include <ktexteditor/application.h>
32​
33​#include <QFileInfo>
34​#include <kpluginfactory.h>
35​#include <kpluginloader.h>
36​#include <kaboutdata.h>
37​#include <QAction>
38​#include <klocalizedstring.h>
39​#include <kactioncollection.h>
40​#include <KXMLGUIFactory>
41​#include <KJobWidgets>
42​#include <KIO/StatJob>
43​#include <QDir>
44​
45​
46​K_PLUGIN_FACTORY_WITH_JSON(KateOpenSelectionFactory,"kateopenselectionplugin.json", registerPlugin<PluginKateOpenSelection>();)
47​
48​
pinoUnsubmitted
Done

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

pino: I guess there is no need for commented code in new code.
49​PluginViewKateOpenSelection::PluginViewKateOpenSelection(PluginKateOpenSelection *plugin, KTextEditor::MainWindow *mainwindow)
50​ : KTextEditor::Command(QStringList() << QStringLiteral("open-selection"), mainwindow),
51​ KXMLGUIClient(),
52​ m_plugin(plugin),
53​ m_mainWindow(mainwindow)
54​{
55​ KXMLGUIClient::setComponentName(QStringLiteral("kateopenselectionplugin"), i18n("Open Selection"));
56​ setXMLFile( QStringLiteral("ui.rc") );
57​ QAction *a = actionCollection()->addAction(QStringLiteral("file_openselection"));
58​ a->setText(i18n("Open selected path"));
59​ actionCollection()->setDefaultShortcut(a, QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_O));
pinoUnsubmitted
Not Done

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

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

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.

nononux: Ok, changed. In fact, menus are not capitalized in my native language, but they are with…
60​ connect(a, &QAction::triggered, plugin, &PluginKateOpenSelection::slotOpenSelection);
yurchorUnsubmitted
Done

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".

yurchor: This seems to be a non-standard way to define a menu item. The other items are just verbs…
pinoUnsubmitted
Done

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

pino: In addition to what Yuri said:a better action text, more in line with our HIG [1] is IMHO "Open…
nononuxAuthorUnsubmitted
Done

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

nononux: I've put "Open selected path" as it seems the others texts in my menu doesn't have all first…
61​ mainwindow->guiFactory()->addClient(this);
62​}
63​
64​PluginViewKateOpenSelection::~PluginViewKateOpenSelection()
65​{
66​ m_mainWindow->guiFactory()->removeClient(this);
67​}
68​
69​PluginKateOpenSelection::PluginKateOpenSelection( QObject* parent, const QList<QVariant>& )
70​ : KTextEditor::Plugin(parent)
71​{
72​}
73​
74​PluginKateOpenSelection::~PluginKateOpenSelection()
75​{
76​}
77​
78​QObject *PluginKateOpenSelection::createView (KTextEditor::MainWindow *mainWindow)
79​{
80​ return new PluginViewKateOpenSelection(this,mainWindow);
81​}
dhaumannUnsubmitted
Not Done

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.

dhaumann: A PluginViewKateOpenSelection instance is created here for every KTextEditor::MainWindow - this…
nononuxAuthorUnsubmitted
Not Done

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

nononux: As a class cannot inherit from 2 QObjects, I've made another class included in the plugin as a…
82​
83​static bool isValidChar(QChar c)
84​{
85​ return !c.isSpace() && c != QLatin1Char('"') && c != QLatin1Char('\'');
86​}
87​
88​void PluginKateOpenSelection::slotOpenSelection()
89​{
90​ KTextEditor::Application *application = KTextEditor::Editor::instance()->application();
91​ if (!application->activeMainWindow()) {
92​ return;
dhaumannUnsubmitted
Done

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

dhaumann: A view always has a valid document, no need to check editView->document()
93​ }
94​
95​ KTextEditor::View* editView = application->activeMainWindow()->activeView();
96​ if (editView) {
97​ QString selection;
98​
99​ // Selected text
100​ if (editView->selection()) {
101​ selection = editView->selectionText();
pinoUnsubmitted
Done

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

pino: `selection.chop(1)` is easier. Even better, directly trim the string to remove all whitespaces…
102​ selection = selection.trimmed();
103​ }
104​
105​ // Try to extract path under cursor if no selected text
106​ if (selection.isEmpty()) {
107​ const KTextEditor::Cursor &cursor = editView->cursorPosition();
108​ const QString line = editView->document()->line(cursor.line());
dhaumannUnsubmitted
Done

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

dhaumann: QString is implicitly shared. Please remove the '&' here.
dhaumannUnsubmitted
Done

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.

dhaumann: Please factor this code part out into a separate function: QString findPathAtCursor(...).
109​ const int lineLength = line.size();
110​ if (lineLength > 0) {
111​ const int col = cursor.column();
112​ int start = col;
113​ int end = col;
dhaumannUnsubmitted
Done

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

dhaumann: In general: Please use spaces around operators and adhere to the standard KDE coding guidelines…
114​
115​ // Stop at first white space or quote (for path with space, user should select the text)
pinoUnsubmitted
Done

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.

pino: This condition (and the same below for `end`) is hard to read, as it mixes checks and an…
nononuxAuthorUnsubmitted
Done

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 ?

nononux: Done, I use isSpace() to capture more characters (\n, \r, ...). Maybe it is better to put the…
116​ while (start > 0 && isValidChar(line.at(start - 1))) {
117​ --start;
118​ }
119​ while (end+1 < lineLength && isValidChar(line.at(end + 1))) {
120​ ++end;
121​ }
122​ selection = line.mid(start, end - start + 1);
123​ }
124​ }
125​
126​ // Relative file path : set relative to current document path
127​ if (QDir::isRelativePath(selection)) {
128​ const QUrl currentUrl = editView->document()->url();
129​ if (currentUrl.isValid() && !currentUrl.isEmpty()) {
130​ const QDir currentDir(QFileInfo(currentUrl.path()).absolutePath());
131​ selection = currentDir.absoluteFilePath(selection);
132​ }
133​ }
134​
135​ // Open the file
pinoUnsubmitted
Not Done

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

pino: Excluding the newline in the code above (see my `isValidChar()` suggestion) can avoid the need…
nononuxAuthorUnsubmitted
Not Done

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).

nononux: I think the test should be kept : if a multi-line text is selected, it can contain a newline…
pinoUnsubmitted
Not Done

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.

pino: This is more general thing to take into account, as I mentioned in another comment: what if the…
nononuxAuthorUnsubmitted
Not Done

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 ?

nononux: It won't open anything if there are more than one path in the selection (the file existence…
136​ if (!selection.isEmpty() && !selection.contains(QLatin1Char('\n'))) {
pinoUnsubmitted
Done

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

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

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 //.

nononux: The name of the function 'fromLocalFile' is a bit ambiguous. The QUrl doc (https://doc.qt.io/qt…
pinoUnsubmitted
Not Done

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.

pino: No, QUrl::fromLocalFile() always assumes it is a local file, using a local "file" protocol. The…
nononuxAuthorUnsubmitted
Not Done

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

nononux: I changed the code to support http://-like paths.
137​ const QUrl url = QUrl::fromLocalFile(selection);
138​ if (fileExists(url)) {
139​ application->activeMainWindow()->openUrl(url);
140​ }
141​ }
142​ }
143​}
144​
145​bool PluginKateOpenSelection::fileExists(const QUrl &url)
146​{
147​ if (url.isLocalFile()) {
148​ return QFile::exists(url.toLocalFile());
149​ }
150​ KIO::JobFlags flags = KIO::DefaultFlags;
151​ KIO::StatJob *job = KIO::stat(url, flags);
152​ KJobWidgets::setWindow(job, KTextEditor::Editor::instance()->application()->activeMainWindow()->window());
153​ job->setSide(KIO::StatJob::DestinationSide);
154​ job->exec();
155​ return !job->error();
156​}
157​
158​bool PluginViewKateOpenSelection::exec(KTextEditor::View *view, const QString &cmd, QString &msg, const KTextEditor::Range &)
159​{
160​ Q_UNUSED(view)
161​ Q_UNUSED(cmd)
162​ Q_UNUSED(msg)
163​
164​ m_plugin->slotOpenSelection();
165​ return true;
166​}
167​
168​bool PluginViewKateOpenSelection::help(KTextEditor::View *view, const QString &cmd, QString &msg)
169​{
170​ Q_UNUSED(view)
171​ Q_UNUSED(cmd)
172​
173​ msg = i18n("<p><b>open-selection &mdash; open the selected path</b></p>"
174​ "<p>usage: <tt><b>open-selection</b></tt></p>"
175​ "<p>This command will open the file under the cursor (path in selection or auto-detection)</p>");
176​
177​ return true;
178​}
179​
180​#include "plugin_kateopenselection.moc"