- User Since
- Mar 25 2016, 4:10 PM (130 w, 3 d)
- again, please do not mix unrelated changes, just submit separate and documented review for them
- please follow a bit more the existing coding style
This patch does more things that "remove old style casts". Some of the other changes are OK, but please do split them in own changes, properly documenting them.
Sat, Sep 22
Not sure I understand, what is the exact workflow that involves writing a .pot file, and reading .po files? Is it because peruse so far is not translatable?
If so, then it'd be much better to extract the messages, and make sure to use ki18n to translate the strings at runtime, just like all the other applications.
Wed, Sep 19
What about using the system version of libastyle, instead? I.e. look for libastyle, and if not found then do not build kdev-astyle.
This would avoid a code copy which clearly is not kept up-to-date, with almost no overhead on packagers (since astyle provides a shared library for a long time).
Since astyle 3.0 broke the ABI and changed its API a bit, using that version as minimum could be a reasonable idea.
Tue, Sep 18
note there are still few "not done" comments around (eg using QSpinBox for fwMark)
- please remove all the empty initTestCase() in tests
Mon, Sep 17
- same notes for simpleipv6test.cpp as in simpleipv4test.cpp
- same note as in D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator regarding the commit, and the summary of this review
- in the test, better use the data-driven approach for tests, instead of long copy&paste lines of "set data, check result" sequences
Wed, Sep 12
NACK, this is not the correct approach.
The right approach is translate the messages via the .po file that contains them: https://websvn.kde.org/trunk/l10n-kf5/ru/messages/kde-workspace/plasma-browser-extension.po?view=markup (and the same in branches/stable/l10n-kf5). There is an automatic system that extracts the messages into plasma-browser-extension.pot, and injects all the translations back to the repository in form of messages.json files.
Sun, Sep 9
Thanks Bruce for all the updates, and patience! In return, I have more notes :)
- I'd send the changes to the existing SimpleIpV4AddressValidator in an own review request, since it affects other code than just this new wireguard plugin (maybe with unit tests)
- I'd send the addition of SimpleIpListValidator in an own review request too, since it is an independent code (possibly with unit tests)
- something I forgot (sorry) in the past: QRegExp is the "old" class for regexps, while in new code QRegularExpression should be preferred/used; should be mostly a transparent change for your uses
- I notes various nits for the UI strings: this is to follow the HIG, see https://hig.kde.org/style/writing/index.html in particular
- if possible, please avoid HTML markup in strings in UI files -- they make translations slightly more complicated (e.g. more prone to break the string with a typo in the markup)
Sat, Sep 8
Please commit it in the Applications/18.08 branch.
Tue, Sep 4
I stopped at Viewer/ViewerWidget.cpp, but the main point is: please do not add whitespace/indentation changes to this patch. This patch is already big because of all the i18n changes, no need to make it way bigger by adding unrelated changes; they make it harder to review what was the actual topic of the commit.
Mon, Sep 3
Much better now!
Sun, Sep 2
Fri, Aug 31
I don't see how loading it once per process would be a big issue, compared to embedding data in the library.
Also, the proposed approach here creates another kind of static data in the sources (isotocountrymap_data.cpp); considering it is not automatically generated, this means that it will be hardly updated by anyone, and happily rot in the sources. Just look at the existing countrytransl.map, for example.
NACK, duplicate of D14932: l10n fixes
Please lookup the strings at runtime, instead of embedding them in the library.
Otherwise, an update to iso-codes (like updating the name of a country) will break the mapping, since the installed translations will not reflect the old text embedded.
Thu, Aug 30
Wed, Aug 29
Tue, Aug 28
I'd say to use this version, and merge it to master.
As noted in D15133: Drop functionality that depends on libraw features removed in 0.19, this approach IMHO is way safer, and less disruptive for users of libkdcraw (and downstreams too...).
No need to break the API/ABI of libkdcraw just because of changes in libraw -- just mark the public stuff affected as deprecated.
Regarding the internal usage: please keep the old code for older versions of libraw; see the libraw_version.h header (provided by libraw) with some defines & macros for this.
Mon, Aug 27
- please remove all the translations (i.e. Name=[lang] & Comment[lang] from desktop/json files, since KDE has an automatic system to handle them
- please use KConfig to read & write ini-like files, instead of doing everything manually
- the PasswordField class is already available as libs/editor/widgets/passwordfield.h, part of the plasmanm_editor library, so you do not need to copy it locally
- wireguard_global.h seems not used, so just drop it
- nm-wireguard-service.h has lots of NM_OPENVPN_ keys that are not used
- as @lbeltrame said, please use QHostAddress to parse IP addresses
- as @lbeltrame said, hardcoding colors is a bad idea, and it will not play nice with user choice of different color schemes, or accessibility purposes
- a better option to validate an input in a line edit is to use the embedded validator options, see QLineEdit::setInputMask and QValidator
- the better option to edit a port number is QSpinBox restricted to 0-65536, instead of a line edit
- there is a mixture of virtual & Q_DECL_OVERRIDE for virtual methods; plasma-nm uses override exclusively
- please do not hardcode sizes and fonts in UI files
- there are various texts in UI files with double spaces between words, please simply them to a single space
- manually calling KAcceleratorManager::manage(this); is not needed, why were they added?
Sun, Aug 26
Sat, Aug 25
For the fix itself: you do not need a diff review for this kind of changes, especially for master branches that were recently opened again for development...
Aug 25 2018
Aug 22 2018
Note that all the i18n() calls where you add a context that was not there before (say i18n("foo") -> i18n("@title:window", "foo")) they will not build, since i18n() does not take a context. You must switch them to i18nc() instead.
This, plus the other note I left, makes me think that this was not build-tested... please make sure that this change at least builds fine.
Aug 21 2018
Aug 20 2018
I have more notes, but no time right now.
Duplicated of D14119: Add screenshot to gdrive.appdata.xml..
Aug 19 2018
Aug 18 2018
FWIW, there are still lots of my concerns that remain unfixed -- sad to see that this patch was committed like this.