pino added a comment.
In D15093#317960 <https://phabricator.kde.org/D15093#317960>, @andersonbruce wrote: > I've used KDE for years but this is the first time I've written code using Qt so it doesn't surprise me that I didn't use some of the preferred methods of doing things. I have a few questions below and hopefully you will have a little patience with me if any seem like stupid questions. > > In D15093#315890 <https://phabricator.kde.org/D15093#315890>, @pino wrote: > > > Misc notes: > > > > - please remove all the translations (i.e. `Name=[lang]` & `Comment[lang]` from desktop/json files, since KDE has an automatic system to handle them > > > Do the automatic translations get added into the desktop files themselves at some point of the build process? There is an automatic system that extracts translations from desktop files (and alike), and injects translations back. >> - 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 > > What I am trying to do here is to highlight fields that don't have valid entries in them. I did this by turning the background in the field red if it wasn't valid and returning it to default when it became valid. Is there a "KDE way" to do something like this or should I just leave everything with the default background and not give the user any immediate feedback that there is a problem? I mentioned it one point below: >> - a better option to validate an input in a line edit is to use the embedded validator options, see `QLineEdit::setInputMask` and `QValidator` IOW, instead of validating the input //after// the user inputs it, do it //before//. >> - the better option to edit a port number is `QSpinBox` restricted to 0-65536, instead of a line edit > > For the one entry which specifies only a port number, the most common instance is to leave this field empty. In my quick read through of the QSpinBox docs I didn't see any way to do a mixed 'no entry'/number option so I will probably leave this as a line edit. IMHO having 0 as value for that is good; otherwise, a proper QIntValidator for the line edit restricts the input the user can insert, removing the need to do the validation manually. >> - there is a mixture of `virtual` & `Q_DECL_OVERRIDE` for virtual methods; plasma-nm uses `override` exclusively > > Can you please give explicit references to where the problem is? `wireguardwidget.h`, for example. I am not familiar with how Q_DECL_OVERRIDE is used but I did look at how all the other VPN implementations did it and it looks like they use the exact same mix of `virtual` & `Q_DECL_OVERRIDE` as I used in WireGuard. See commit 111ac65ae79f1a777e3b4a6389e916f0dfccd35e <https://phabricator.kde.org/R116:111ac65ae79f1a777e3b4a6389e916f0dfccd35e>. It looks like you are developing against an old version of plasma-nm, or not on master anyway. >> - please do not hardcode sizes and fonts in UI files > > I can understand that fonts shouldn't be specified and have removed them. What I was trying to do was to highlight the text in a couple of labels by making them a little bigger than the default font and making them bold. Is there a "KDE way" to highlight something like this? Maybe a way to say "this is a header" or similar? Rather than "how can I make some text bigger", the question is "what do you need to do". Since you are grouping widgets, then use a QGroupBox. Oh, and that adds another thing to fix: `wireguard.ui` really needs a top-level layout, instead of hardcoding the size of the widgets... >> - 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? > > Again this is due to my inexperience with Qt. Is there some reason that WireGuard doesn't need this but all the other VPN implementations do? This was added by dbb4e2d5d47a6546014d433a1533d4ef4cfb7137 <https://phabricator.kde.org/R116:dbb4e2d5d47a6546014d433a1533d4ef4cfb7137>. Weird choice, I guess this can be left as it is. REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D15093 To: andersonbruce, #plasma, jgrulich, pino Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
