Functionally I think that the current implementation does this (although I can change the name of the function if you want). It starts with a blank NMStringMap and uses setOrClear on it. Are you possibly referring to the same function name used in wireguardwidget.cpp rather than here in wireguardadvancedwidget.cpp?
note there are still few "not done" comments around (eg using QSpinBox for fwMark)
|28 ↗||(On Diff #41874)|
no need to specify this as parent, since it is on stack, and thus it will be deleted automatically
since earlier there is a keyPos variable used for basically the same purpose (i.e. passing it to the validate() of validators), just move pos earlier in the function, and use one everywhere
a bit confusing to call intVal a variable which is (not correctly) a long; OTOH, it can be removed altogether (see below)
just inline the calls to value(), just like done below
no need for Q_DECL_EXPORT, as it is in a plugin, and thus it does not need to be exported as public symbol
this is not "done", btw
We don't do validation inside the passwordfield widget, this is done outside in widgets using it, you should do the same. Given it's a QWidget, you cannot directly assign a validator to it, but inside is QLineEdit which can have validator, you would have add a method to the main class which would just assign the validator to the QLineEdit widget inside, still please do the validation outside.
Yes I know, I'll get to the spinbox in the next day or so. I've only been putting it off because I think it is such a bad idea and I hate making something I wrote worse rather than better but I know that I am playing in your house so I need to play by your rules. I'll hold my nose and get to it soon.
What you say sounds like you are understanding my notes as "imposing on you" "bad choices", which is definitely not the case. What I note are things based on my experience, and what I read from the patch.
If I see a configuration key which is validated as positive integer, then my conclusion is that it makes sense to use the proper widget for it, i.e. QSpinBox (with proper range). If you think it is not correct, please do explain it! I could be missing anything, or simply knowing better about it is a good idea regardless.
But please do not play the card of "you are the bad guy imposing things on me", thanks.
I'm sorry, I am not trying to paint you as a bad guy here. What I meant to say is that I disagree with you that using a QSpinBox in these cases is a good idea. I just realize that different projects have different design philosophies / standards and that since I am working on your project which I am a newcomer to, that I am going to follow your recommendations even if they disagree with my personal design philosophy because I realize it is important to be consistent across multiple projects of which this is only a tiny piece of just one project.
In the case of the spinbox, I personally don't think that they should be used for anything with more than about 100 different steps because, by their nature, they tell the user to use the up and down arrows to change the value. Now you know and I know that you can just type in a value but not everyone does and on a value that can range from 0 to a very large number (in this case 2000000000+) I believe typing in a value makes sense 99.99% of the time and that using a text box tells the user: "Type a number in here" better than a spinbox does. The same is true to a slightly lesser extent for the other two items which have been changed to spinboxes. Also since all of these will almost always not be used, again my personal opinion is that a blank text box conveys that better than sticking a "default" or "automatic" down at the low end of the spinbox range again because not everyone is going to know to type in 0 to get back to "default" if somehow it gets changed.
It was late at night when your message about not forgetting about the spinbox (which I hadn't forgotten) came in and it sounded at the time a little like someone saying "I've told you this before, now you have to do this". Obviously this was not your intention and I apologize that I let my frustration get to me a little.
Perhaps mistakenly, I have taken most of the 'design' related comments (as opposed to the 'implementation' issues) as being "this is the KDE way of doing things" and since that is a perfectly valid reason for doing things a particular way, I've just said "Okay, I'll do this even though I disagree that it is a good idea" and got a little frustrated about them. Maybe I should have pushed back a little harder on this and other issues I disagreed with. Who knows. It's all water under the bridge now. Again, my apologies.
We generally try to operate on the principle of consensus here in the KDE community, so if there's a disagreement, we talk about it until someone changes their mind or we arrive at a compromise.
FWIW, I would tend to agree that using a spinbox doesn't make sense when the range is enormous and there isn't much of a chance that it would ever make sense to use the arrows to adjust the value.
Okay, I just uploaded changes for most of the rest of the comments but I would like to revisit a couple of earlier issues.
The first is the SpinBoxes. As I said in another comment, I don't think that the SpinBoxes are appropriate for any of the entries and would like to return these to be plain LineEdit widgets. The smallest range for any of these is the port number box at 0-65535 and I don't think any of them will be entered using the SpinBox arrows so I think simple text entry is better from an HMI perspective.
Secondly I would like to reconsider entry validation. Currently it is using Pino's suggestion of using Validators on each entry to not allow incorrect keys to be entered and I think this is a good approach but I don't think it is sufficient. Right now there are 5 items checked for validity on the main screen (plus a couple on the Advanced settings widget) which can be checked for validity and when they are all valid, the "Save" button is enabled.
These are obviously different in that one tests to see if the entry is completely valid while the other only checks to see if it can still be made to be complete and valid. My problem is that if there are five things that need to be checked and only one bit of information (whether the Save button is enabled or disabled) to indicate to the user that there is a problem and no indication of which element might be in error.
I would like to add some type of indicator on each entry line to indicate whether that particular line is valid so if the user has entered: "BBBB:0:1223:" for an IPv6 address (which is not valid) and all the other lines are valid, the user would be able to see immediately which line they need to look at to go forward. This was the idea behind my initial code which changed the background color of widgets to indicate validity. I realize that there are problems with this approach so I would like to come up with some other means to supply this information to the user. I've done three mockups of some possibilities:
The difference between the last two is whether the CheckBoxes are enabled or not. I think that the enabled CheckBoxes is the best looking although it requires a little more programming because since it would be utilized as a strictly output widget I would have to intercept any clicks on the check box and ensure that it stayed in the correct state.
Any comments would be appreciated.
How about changing the outline or background of the text box to the Positive or Negative color from the active color theme? That way (for people with sensible color themes), the text box would show red if the value is invalid. I believe GNOME does something similar to this for their IP address validator, and in my experience it works quite nicely.
Well for the first time since July there has been activity on network-manager-wireguard including the addition of an additional parameter supported so I am going to get going on adding it to this. Also, it looks like the pre- and post- options now do something so against my better judgment I am going to add them back in as well.
Linking against following ones is enough:
plasmanm_internal plasmanm_editor KF5::ConfigCore KF5::CoreAddons KF5::I18n KF5::KIOWidgets KF5::WidgetsAddons
Name could be just WireGuard I guess
This validation doesn't seem to work, I can click on "ok" button even when the advanced widget is empty. Also it's a bit weird that the function name suggests something else then it's doing. I would suggest splitting this into something like "updatePresharedKeyValidation()" and to the one you have now. One would really do just the check, the other would update the color.
Same please do for classic wireguard widget.
Perhaps you should write a destructor for the private widget and here just delete the d-pointer.
KColorScheme has been removed/deprecated in KDE Frameworks and it's only in KDELibs4support which we don't want to use. You should use QPalette instead.
You also will not need this, because QPalette already should have used colors thanks to our KDE platform theme.
Do not use KColorScheme. Use QPalette instead and you don't need to keeping it as class variable.
Split isFooValid() functions, as mentioned in the advanced widget. Also slotWidgetChanged() will call isValid() I think, which means that each isFooValid() is called twice.
My apologies if I sound a little frustrated on this but I spent more than 4 hours trying to follow the very unclear documentation on handling colors before making this change. I thought the whole idea was that I was supposed to use the scheme the user picked to tell what color to make the background when it wasn't valid rather than assigning an arbitrary color. KColorScheme allows you to do this using the "NegativeBackground" role but there doesn't appear to be any corresponding concept using QPalette. What would you suggest that I use as the background for invalid entries? Or better yet. do you know of an example of something that uses QPalette in a similar context since I have been unable to find one?
As far as putting it in a class variable, I only did that so that I wouldn't be creating them each time I wanted to change a background on one of the widgets and to be able to pass them to the advanced widget and not have to create them there as well. If you think that palette creation is a relatively time efficient process, I'll just create them on the stack in the setBackground() function each time they are needed.
I'm sorry, I was wrong and I thought that KColorPalette is deprecated, but it's actually not and it's part of KConfigWidgets. Still, given you just use those two colors, you don't need to save full palette, just construct a one to get the colors and save these as member variables. You can do basically same for the advanced widget, you don't need to pass them in constructor.
A couple of notes on the latest upload:
- I left the background color change code alone. Doing searches on changing widget background color, it looks like the preferred method is to use setPalette() which requires a QPalette, and after trying a couple of things I decided it made more sense to me to pass around two palettes by reference than to pass just the colors and create palettes when they were needed.
- I backed out the change that used a blank setting rather than the stored values. I agree that it would be nice to not hang on to any extraneous elements if they happened to be created by some other application but what I found was that I had somehow not tested it after making this change and when I did, I found that it had broken the entire "Advanced" widget settings. The problem is that if you start with a blank setting and don't open the Advanced Widget then all the settings in that are lost. To fix this I would have to duplicate a bunch of functionality in both widgets which I don't like doing. Also, if the underlying NetworkManager app adds something I think that it is probably better to not delete it if plasma-nm hasn't been updated yet so I ended up deciding to put it back to the way it was.
I left this alone because it is consistent with the names of other VPNs.
I did this slightly different from the suggestion but it only does the checks once per change and I think it is correctly enabling/disabling the "Ok" button. By the way, the "Ok" button is enabled when all entries on the Advanced widget are blank because all of the entries are optional and therefore all blank is perfectly valid.
I still don't like the way how to get QPalette in the advanced dialog, can you please just simply construct it the same way you do it in the standard dialog? Other than that it looks good and I think it's ready to go. Those mentioned coding style can be fixed later, I can go through that after it's merged.
Coding style, but can be fixed afterwards.
Coding style, can be fixed afterwards.
Fair enough. I changed it so the palettes are created in the advanced widget as well as the main interface.
I fixed one of the coding style errors which I just missed last time. The other ones I will let you fix because I'm not sure what the correct format would be.
Thanks for your patience with all my mistakes in this process.
Very cool! Does this actually implement full native WireGuard support, or only add support for the NetworkManager plugin available at https://github.com/max-moser/network-manager-wireguard?
It only provides a front end for the NetworkManager plugin. This keeps the network handling all in NetworkManager but allows setup and display to be integrated into the KDE/Plasma applet.
I assume this is the case.
There are several NetworkManager VPN plugins that are handled this way where the code is not part of the official release but are referenced on the Gnome homepage for the project as being supported by third parties. Most if not all of these are already supported in the plasma-nm release and this just adds WireGuard to that group. I know that at least openSuse Tumbleweed supplies some of these third party plugins (although not WireGuard yet) so hopefully as WireGuard becomes more popular it will join this group as well.
Thanks for the explanation! Just what I needed to feature this in https://pointieststick.wordpress.com/2018/11/07/this-week-in-usability-productivity-part-44 (link will work starting this Sunday).