D22199: New plugin to open the selected file path

[Pino Toscano]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20190702/5f417657/attachment-0001.html>