Expose some more settings in an Advanced dialog
ClosedPublic

Authored by apol on Jul 9 2019, 4:38 PM.

Details

Summary

Was requested by some users who needed these fields and which are already offered by NM.

Test Plan

Changed them, then changed them back.

Diff Detail

Repository
R116 Plasma Network Management Applet
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13788
Build 13806: arc lint + arc unit
apol created this revision.Jul 9 2019, 4:38 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 9 2019, 4:38 PM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald Transcript
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Jul 9 2019, 4:38 PM
ngraham added a subscriber: ngraham.Jul 9 2019, 5:58 PM
ngraham added inline comments.
libs/editor/settings/ipv4widget.cpp
431 ↗(On Diff #61426)

Add a colon and don't use Title Case: "Send hostname:"

435 ↗(On Diff #61426)

Add a colon and don't use Title Case: "DHCP hostname:"

440 ↗(On Diff #61426)

Add a colon and don't use Title Case: "DAD timeout:"

broulik added a subscriber: broulik.Jul 9 2019, 8:02 PM
broulik added inline comments.
libs/editor/settings/ipv4widget.cpp
424 ↗(On Diff #61426)

What's that QPointer for?
You might want to dlg->setAttribute(Qt::WA_DeleteOnClose);

427 ↗(On Diff #61426)

I would prefer that you used a UI file for all of this

455 ↗(On Diff #61426)

Doesn't seem to work? Or did you disable the dimming effect or is that a quirk in conjunction with QQuickWidget in systemsettings?

apol marked 4 inline comments as done.Jul 9 2019, 10:52 PM
apol added inline comments.
libs/editor/settings/ipv4widget.cpp
455 ↗(On Diff #61426)

Worked for me, I've seen issues with the list view on the left though. :/

apol updated this revision to Diff 61474.Jul 9 2019, 10:52 PM

Addressed comments

Don't you miss loading/saving of those properties in loadConfig() and setting() methods?

I think this UI is missing hints/tips about meaning of these options. Otherwise it would be unclear:

  • What is the meaning of dad-timeout = -1
  • What is the meaning of dad-timeout = 0
  • What happens if dhcp-send-hostname is true, but dhcp-hostname is empty
  • What happens if dhcp-send-hostname is false, but dhcp-hostname is not empty (if it would be ignored, consider disabling the line edit)
apol updated this revision to Diff 61510.Jul 10 2019, 1:46 PM

addressed comments

You still need to add following lines to the loadConfig() method, otherwise those properties won't be filled when editing an existing connection:

m_tmpIpv4Setting.setDhcpHostname(ipv4Setting->dhcpHostname());
m_tmpIpv4Setting.setDhcpSendHostname(ipv4Setting->dhcpSendHostname());
m_tmpIpv4Setting.setDadTimeout(ipv4Setting->dadTimeout());
apol updated this revision to Diff 61520.Jul 10 2019, 3:32 PM

add loadConfig bits

jgrulich accepted this revision.Jul 11 2019, 5:57 AM

Looks good to me now.

This revision is now accepted and ready to land.Jul 11 2019, 5:57 AM
This revision was automatically updated to reflect the committed changes.
broulik added inline comments.Jul 11 2019, 9:56 AM
libs/editor/settings/ipv4widget.cpp
443 ↗(On Diff #61426)

a specialValueText() for -1 "Default" would have been lovely

452 ↗(On Diff #61426)

Missing a this context