D22199: New plugin to open the selected file path

[Arnaud Ruiz]

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