andersonbruce (Bruce Anderson)
User

Projects

User does not belong to any projects.

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Friday

  • Clear sailing ahead.

User Details

User Since
Aug 26 2018, 6:24 AM (59 w, 3 d)
Availability
Available

Recent Activity

Jul 5 2019

andersonbruce requested review of D22283: Change validator for Endpoint Address entry field.
Jul 5 2019, 6:21 AM · Plasma

May 13 2019

andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

The changes to the key passwordfield menus was made to remove the AlwaysAsk option. This is the last change I am aware of necessary to release this.

May 13 2019, 7:44 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Disable "AlwaysAsk" option in key password fields
May 13 2019, 7:42 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Updated SecretAgent class to always try to get the secrets from kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random characters long and we don't expect the user to enter these manually when trying to make a connection. If data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

Great! Should I make the change in this review? If so, what is the correct procedure (with git, arc, ...) to merge that change into this review?

Should be enough just to merge current master into your branch I guess.

May 13 2019, 6:58 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Updated SecretAgent class to always try to get the secrets from kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random characters long and we don't expect the user to enter these manually when trying to make a connection. If data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

May 13 2019, 6:36 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Return non-WireGuard logic to original state
May 13 2019, 6:24 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Updated SecretAgent class to always try to get the secrets from kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random characters long and we don't expect the user to enter these manually when trying to make a connection. If data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

May 13 2019, 2:47 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Don't allow save of configuration with "AlwaysAsk" flag for keys
May 13 2019, 2:46 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Always get encrypted data from kwallet
May 13 2019, 2:23 AM · Plasma

May 11 2019

andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Update to fix some comments.

May 11 2019, 6:57 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Correct review comments
May 11 2019, 6:16 AM · Plasma

May 9 2019

andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Update labels on Add and Remove peer buttons
May 9 2019, 5:49 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Personally I like the Add and Remove buttons to be together where they were, and to simply change the wording to "Add New Peer" and "Remove This Peer". I think this still makes it clear that the peer in the tab currently being displayed in the dialog is the one that will be removed

All right, let's do that, then!

I'll update to this.

May 9 2019, 5:28 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

So much better!

I have one more little suggestion for the buttons: move the "Remove Peer" button into the bottom of the tab for that peer to make it clear what it applies to, and change the text to say, "Remove this Peer". The Remove button can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" the correct verb? Would "Connect to" be more appropriate?

May 9 2019, 1:14 AM · Plasma

May 8 2019

andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

I'm afraid I don't have the ability to test this; could you post a new screenshot? Thanks!

May 8 2019, 5:23 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

A fairly large update was made to change the input of peer data to a use a QTabWidget interface to switch between peers rather than a spin box.

May 8 2019, 12:42 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.

Change to tab bar interface for peers

May 8 2019, 12:37 AM · Plasma

May 3 2019

andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

When VDG assistance is requested, screenshots of the current/proposed UI are appreciated. :)

May 3 2019, 3:48 AM · Plasma
andersonbruce added a comment to D20930: Update WireGuard to match NetworkManager 1.16 interface.

Requested changes are made.

May 3 2019, 3:27 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Correct review comment
May 3 2019, 3:20 AM · Plasma
andersonbruce updated the diff for D20930: Update WireGuard to match NetworkManager 1.16 interface.
  • Correct CamelCase of Wireguard to WireGuard
  • Disable adding WireGuard for NetworkManager < 1.16
  • Correct review comments
May 3 2019, 3:08 AM · Plasma

May 1 2019

andersonbruce requested review of D20930: Update WireGuard to match NetworkManager 1.16 interface.
May 1 2019, 2:03 AM · Plasma

Mar 15 2019

andersonbruce added a comment to D19434: Change entry of Endpoint UI.

Shouldn't we maybe use QSpinBox for the port? With the spinbox you can also set min and max values so you don't need to validate it.

My preference is to only use a spinbox for entries with less than 100 possible values. I believe that the up/down arrows give the impression that clicking on them is the "proper" method of selecting a value and while some people know that you can type in a value into a spinbox, I believe that there are others who do not and I really don't want users to be metaphorically cursing my name when they are trying to click their way up to a port number of say, 23517. Also, using a simple LineEdit with color coding for indicating an unacceptable input is consistent with the rest of the interface.

In that case you can use https://doc.qt.io/qt-5/qlineedit.html#inputMask-prop to limit what users can type there. You can also assign validators to other lineedits so users will be able to type only correct values.

Mar 15 2019, 10:12 AM · Plasma

Mar 7 2019

andersonbruce updated the diff for D19434: Change entry of Endpoint UI.
  • Add validator for Endpoint Port field
Mar 7 2019, 12:58 AM · Plasma

Mar 3 2019

andersonbruce added a comment to D19434: Change entry of Endpoint UI.

Shouldn't we maybe use QSpinBox for the port? With the spinbox you can also set min and max values so you don't need to validate it.

Mar 3 2019, 4:38 AM · Plasma

Mar 1 2019

andersonbruce requested review of D19435: Remove redundant parameter fields.
Mar 1 2019, 5:12 AM · Plasma
andersonbruce requested review of D19434: Change entry of Endpoint UI.
Mar 1 2019, 5:10 AM · Plasma

Feb 13 2019

andersonbruce updated the summary of D18867: Remove redundant DNS field.
Feb 13 2019, 12:47 AM · Plasma

Feb 9 2019

andersonbruce added a comment to D18867: Remove redundant DNS field.

Any connection created by the previous version or the basic NetworkManager plugin should continue to work even though the DNS server list will not be viewable in the new interface.
I did not provide an automatic conversion from a previously created connection to the new storage format. Neither did I create a warning (popup?) if an old style config is displayed by the editor. If there is a consensus to add either of these items, I can do that.

Feb 9 2019, 4:07 AM · Plasma
andersonbruce requested review of D18867: Remove redundant DNS field.
Feb 9 2019, 3:48 AM · Plasma

Nov 8 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

Ah, thanks. And presumably shipping that networkmanager plugin would be up to distros, right?

Nov 8 2018, 4:50 AM · Plasma

Nov 7 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

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?

Nov 7 2018, 10:49 PM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

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?

Nov 7 2018, 3:33 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Allocate palettes in advanced widget rather than passing them from main interface
Nov 7 2018, 3:22 AM · Plasma

Oct 25 2018

andersonbruce added inline comments to D15093: Add WireGuard capability..
Oct 25 2018, 6:15 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

A couple of notes on the latest upload:

  1. 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.
Oct 25 2018, 6:05 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Address review comments
Oct 25 2018, 6:05 AM · Plasma

Oct 18 2018

andersonbruce added inline comments to D15093: Add WireGuard capability..
Oct 18 2018, 10:20 AM · Plasma

Oct 15 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

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.

Oct 15 2018, 8:38 AM · Plasma

Oct 1 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

I've uploaded everything I wanted to get done now. Are there any new comments? And by the way, what is the process once the all the comments do get cleared?

Oct 1 2018, 10:13 AM · Plasma

Sep 25 2018

andersonbruce updated the diff for D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
  • Fix review comments
Sep 25 2018, 10:31 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Add entry widget background color change based on entry validity
  • Change QSpinBoxes back to QLineEdits
Sep 25 2018, 8:43 AM · Plasma
andersonbruce updated the diff for D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
  • Fix a case that was being reported as Acceptable rather than Intermediate
Sep 25 2018, 6:49 AM · Plasma

Sep 24 2018

andersonbruce abandoned D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..

Changes for this revision were merged into D15520

Sep 24 2018, 8:58 AM · Plasma

Sep 22 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

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.

Sep 22 2018, 3:51 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Update from more (but not all) review comments
  • Change 'setting' functions to use blank NMStringMap rather than incoming data.
Sep 22 2018, 3:19 AM · Plasma

Sep 19 2018

andersonbruce added a comment to D15093: Add WireGuard capability..
In D15093#328246, @pino wrote:
In D15093#327917, @pino wrote:

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

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.

Sep 19 2018, 2:11 AM · Plasma

Sep 18 2018

andersonbruce added a comment to D15093: Add WireGuard capability..
In D15093#327917, @pino wrote:

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

Sep 18 2018, 9:27 PM · Plasma
andersonbruce updated the diff for D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
  • Remove unnecessary includes and member functions
Sep 18 2018, 5:35 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

Since I added a validator function for the WireGuard style keys, is there any way to assign a validator to the PasswordField widget without a fairly substantial rewrite of that class?

Sep 18 2018, 5:14 AM · Plasma
andersonbruce added a comment to D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..

Maybe merge this review with D15520. I think they should go together.

Sep 18 2018, 4:55 AM · Plasma
andersonbruce updated the diff for D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
  • Merge new IP list validator into this branch
  • Update from review comments
Sep 18 2018, 4:42 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Remove changes moved to review D15520
Sep 18 2018, 4:41 AM · Plasma

Sep 17 2018

andersonbruce added inline comments to D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..
Sep 17 2018, 10:15 AM · Plasma
andersonbruce added a comment to D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..

Maybe merge this review with D15520. I think they should go together.

Sep 17 2018, 6:35 AM · Plasma
andersonbruce added inline comments to D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..
Sep 17 2018, 6:17 AM · Plasma
andersonbruce updated the diff for D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..
  • Correct CMakefile I screwed up
Sep 17 2018, 6:15 AM · Plasma
andersonbruce added inline comments to D15093: Add WireGuard capability..
Sep 17 2018, 5:43 AM · Plasma

Sep 16 2018

andersonbruce added inline comments to D15093: Add WireGuard capability..
Sep 16 2018, 7:59 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Add tests for new and updated ip validators
  • Update ip validators
  • Add validator for WireGuard keys
  • Update per review comments
  • Add error messages for failure cases
Sep 16 2018, 7:37 AM · Plasma
andersonbruce updated the diff for D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..
  • Correct overlooked comments from review D15093
Sep 16 2018, 5:19 AM · Plasma

Sep 15 2018

andersonbruce requested review of D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..
Sep 15 2018, 3:02 AM · Plasma
andersonbruce updated the summary of D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
Sep 15 2018, 2:23 AM · Plasma
andersonbruce updated the summary of D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
Sep 15 2018, 2:20 AM · Plasma
andersonbruce requested review of D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.
Sep 15 2018, 2:18 AM · Plasma

Sep 12 2018

andersonbruce added a comment to D15093: Add WireGuard capability..
In D15093#323095, @pino wrote:

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)

A couple of questions on opening other review requests:

  • Should I open a bug request on bugs.kde.org before opening the review?
  • Is there any special way I need to mark dependencies? That is, the simpleiplistvalidator is dependent on the updates to the simpleipv4/6 validators and WireGuard is dependent on both.
  • Is there a preferred unit test framework that KDE uses? And do you possibly have a pointer to a sample of other projects that use it? (Especially if they use QValidator since I tried to write some test code for my own use and I couldn't get it to not crash before it even got to main()).
Sep 12 2018, 6:49 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Correct capitalization
  • Remove HTML from tooltip strings
  • Update some tooltip strings for clarity
Sep 12 2018, 6:17 AM · Plasma

Sep 9 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

I think I've gotten most of the fixes in. I made changes to simpleipv4addressvalidator and simpleipv6addressvalidator to add checking of addresses with CIDR and Port suffixes. Please check these carefully, I think I did the changes such that all existing uses will not be affected but I want to make sure. Oh, except I made one fix in ipv6 where it was disallowing "::" at the beginning of the address followed by anything. This is a totally valid and not uncommon usage so I included that in the change.
I also added a new file simpleiplistvalidator which allows the checking of comma separated lists of IPv4, IPv6, or mixed addresses.

Sep 9 2018, 7:24 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..
  • Fixed comment
  • Expand capabilities of SimpleIP validators.
  • Changes per review comments
  • Update for updated ip validators
Sep 9 2018, 7:00 AM · Plasma

Sep 6 2018

andersonbruce added inline comments to D15093: Add WireGuard capability..
Sep 6 2018, 4:32 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

I think you can completely remove WireguardAuth dialog if there is no use for it. I also spotted few trailing spaces in the patch, please remove them.

Sep 6 2018, 4:03 AM · Plasma

Sep 5 2018

andersonbruce added a comment to D15093: Add WireGuard capability..
Sep 5 2018, 9:54 AM · Plasma

Sep 3 2018

andersonbruce added inline comments to D15093: Add WireGuard capability..
Sep 3 2018, 9:11 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..
In D15093#319253, @pino wrote:

Much better now!

  • regarding the UI for all the pre/post scripts: since they are file paths, better use a KUrlRequester widget (limited to local existing files only, no URLs), so the users have a Browse button next to each line edit that can be used to open a file dialog
Sep 3 2018, 8:23 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

I'm not sure if the author or the reviewer is supposed to check the "Done" box on the inline comments but I think that I have addressed all the various comments made, both inline and separately.

Sep 3 2018, 4:38 AM · Plasma
andersonbruce updated the diff for D15093: Add WireGuard capability..

Updates from review comments

  • Add copyright notices
  • Delete unneeded files
  • Formatting changed to agree with coding standards
  • Changed validation method removing color overrides
  • Removed hard coded fonts and sizes from UI files
Sep 3 2018, 4:28 AM · Plasma

Aug 31 2018

acrouthamel awarded D15093: Add WireGuard capability. a Mountain of Wealth token.
Aug 31 2018, 4:01 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

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.

Aug 31 2018, 2:35 AM · Plasma

Aug 27 2018

andersonbruce added a comment to D15093: Add WireGuard capability..

Some smallish nitpicks I noticed there. Also, does this mean that NM has WG support now?

Yes there is an add-on available. The original is at https://github.com/max-moser/network-manager-wireguard however I have made a couple of mods and issued pull requests but they have not been incorporated in the original yet. I anyone is interested in the latest I am using it is available at: https://github.com/Druco/network-manager-wireguard/tree/AllLocalFixes

I'll also let @jgrulich comment on this, but I'm not too fond of the manual parsing of the INI-like file used by WG. While I can't check at the moment, there should be KF5 or Qt classes to handle tihs.

Aug 27 2018, 5:14 AM · Plasma
andersonbruce updated the summary of D15093: Add WireGuard capability..
Aug 27 2018, 5:03 AM · Plasma
andersonbruce added a comment to D15093: Add WireGuard capability..

Wow, what a first patch! And you even used arc too, how nice.

Since this fixes https://bugs.kde.org/show_bug.cgi?id=397572, can you indicate it as such in the Summary section per https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords?

Just add the following to the Summary section:

FEATURE: 397572
FIXED-IN: 5.14.0

Done

In addition, I see one coding style issue right off the bat: we don't put opening braces on their own lines. That'll need to be changed in all the filed you've added. And speaking of those added files, I think you need to add your copyright to them. Finally, do we really need to duplicate passwordfield.h and passwordfield.cpp?

Aug 27 2018, 4:56 AM · Plasma
andersonbruce requested review of D15093: Add WireGuard capability..
Aug 27 2018, 4:05 AM · Plasma