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
