[plasma-nm] Refresh wifi networks as fast as possible
ClosedPublic

Authored by vpilo on Feb 10 2019, 2:44 PM.

Details

Summary

Added a retry mechanism when scanning fails, to prevent waiting unnecessarily long for the next scan.

This usually happens when closing and reopening the applet, or when a scan was started by another component before opening the applet.

Test Plan
  • First enable debug logging, there is no UI feedback
  • Tested opening/closing the applet every now and then: wifis are scanned as early as NM allows
  • Tested switching to the NM KCM, then switching to the applet, then to the KCM again: wifis are always scanned as early as allowed

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vpilo created this revision.Feb 10 2019, 2:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 10 2019, 2:44 PM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald Transcript
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
vpilo requested review of this revision.Feb 10 2019, 2:44 PM

I would keep the 15s interval, everyone does 15s. Also from what I have read, every scan drops your connection for a while, which might be a problem for bad wifi drivers, where the scan can take 15+ seconds so doing this more often is not a good idea. Other thing I would change is to try to repeat the scan if it fails only for the first time (when you open the applet), then I don't think it's necessary to keep spamming NetworkManager with our requests.

jgrulich added inline comments.Feb 11 2019, 7:05 AM
libs/handler.cpp
64

It doesn't need to be in namespace.

441

This is Qt 5.12 only, Plasma 5.16 is going to depend on it so if you plan to you this, you should bump required Qt version.

vpilo added a comment.Feb 12 2019, 1:15 PM

I would keep the 15s interval, everyone does 15s. Also from what I have read, every scan drops your connection for a while, which might be a problem for bad wifi drivers, where the scan can take 15+ seconds so doing this more often is not a good idea. Other thing I would change is to try to repeat the scan if it fails only for the first time (when you open the applet), then I don't think it's necessary to keep spamming NetworkManager with our requests.

Everyone using 15 seconds for a timeout doesn't mean it's good for the users... Anyways, why shouldn't we be doing an attempt every 2 seconds? that's not a lot. I could do two things instead:

  • double the interval between attempts every time they fail
  • Only run the UI timer when the scan request has been successful

Other thing I would change is to try to repeat the scan if it fails only for the first time
then I don't think it's necessary to keep spamming NetworkManager with our requests.

Maybe not necessary, but I don't think it's an issue either.

The return is very early on the NM side, it won't actually spend time doing anything.
DBus overhead is insignificant when doing something every second.

I would keep the 15s interval, everyone does 15s. Also from what I have read, every scan drops your connection for a while, which might be a problem for bad wifi drivers, where the scan can take 15+ seconds so doing this more often is not a good idea. Other thing I would change is to try to repeat the scan if it fails only for the first time (when you open the applet), then I don't think it's necessary to keep spamming NetworkManager with our requests.

Everyone using 15 seconds for a timeout doesn't mean it's good for the users...

I think it's reasonable enough and also as I mentioned above, wifi scanning drops your connection for a while, there is no reason to drop it more often, especially when we will have a fresh scan as soon as possible.

Also see: https://blogs.gnome.org/dcbw/2016/05/16/networkmanager-and-wifi-scans/

Anyways, why shouldn't we be doing an attempt every 2 seconds? that's not a lot. I could do two things instead:

I didn't mean we shouldn't be doing an attempt every 2 seconds, I meant that we should go through this retry logic only in case of initial scan, but keep it that way. Just set the interval back to 15s.

vpilo updated this revision to Diff 51525.Feb 12 2019, 4:31 PM
  • Review comments, rework
  • Put the timer back to 15s
  • Removed trigger on start for QML timers: the timer now only runs if the current scan was successful
  • Don't use any more Qt 5.12 APIs
jgrulich added inline comments.Feb 13 2019, 7:14 AM
applet/contents/ui/main.qml
78

Since this is just one line function, you can use handler.requestScan() directly as before.

vpilo added inline comments.Feb 14 2019, 11:27 AM
applet/contents/ui/main.qml
78

I did it for consistency, so in both files there's only one action. It's also useful for debugging.

jgrulich added inline comments.Feb 14 2019, 11:48 AM
applet/contents/ui/main.qml
78

It would still be one action even if you call "handler.requestScan()" instead of adding additional unnecessary layer.

vpilo updated this revision to Diff 51677.Feb 14 2019, 1:44 PM
  • Review comments
vpilo marked an inline comment as done.Feb 14 2019, 1:44 PM
jgrulich accepted this revision.Feb 14 2019, 1:48 PM

Looks good. Thank you.

This revision is now accepted and ready to land.Feb 14 2019, 1:48 PM
vpilo edited the summary of this revision. (Show Details)Feb 14 2019, 3:52 PM
This revision was automatically updated to reflect the committed changes.