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<PluginKateOpenSelection>();) > +//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 > +<!DOCTYPE kpartgui> > +<gui name="kateopenselection" library="libkateopenselectionplugin" > version="5" translationDomain="kateopenselection"> > + <MenuBar> 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