D22199: New plugin to open the selected file path
noreply at phabricator.kde.org
Tue Jul 2 19:36:28 BST 2019
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.
> 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  is IMHO "Open Selected Path".
> "Opens the selected path" can be a good tooltip or whatsthis text.
>  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 //.
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...
More information about the KWrite-Devel