From 34ba2fab301f43533ee6876d78809f685dd05614 Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Thu, 21 Sep 2023 18:21:09 +0200 Subject: [PATCH] NetworkMgr: Handle non-blocking turnOnWifi implementations better (#10863) * Device: Add a `hasSeamlessWifiToggle` devcap to complement `hasWifiToggle`, to denote platforms where we can toggle WiFi without losing focus, as this has obvious UX impacts, and less obvious technical impacts on some of the NetworkMgr innards... * Android: Mark as `!hasSeamlessWifiToggle`, as it requires losing focus to the system settings. Moreover, `turnOnWifi` returns *immediately* and we *still* run in the background during that time, for extra spiciness... * NetworkMgr: Ensure only *one* call to `turnOnWifi` will actually go on when stuff gets re-scheduled by the `beforeWifiAction` framework. * NetworkMgr: Ensure the `beforeWifiAction` framework will not re-schedule the same thing *ad vitam aeternam* if a previous connection attempt is still ongoing. (i.e., previously, on Android, if you backed out of the system settings, you entered the Benny Hill dimension, as NetworkMgr would keep throwing you back into the system settings ;p). This has a few implications on callbacks requested by subsequent connection attempts, though. Generally, we'll try to honor *explicitly interactive* callbacks, but `beforeWifiAction` stuff will be dropped (only the original cb is preserved). That's what prevents the aforementioned infinite loop, as the `beforeWifiAction` framework was based on the assumption that `turnOnWifi` somewhat guaranteed `isConnected` to be true on return, something which is only actually true on `hasWifiManager` platforms. * NetworkMgr: In `prompt` mode, the above implies that the prompt will not even be shown for concurrent attempts, as it's otherwise extremely confusing (KOSync on Android being a prime example, as it has a pair of Suspend/Resume handlers, so the initial attempt trips those two because of the focus switch >_<"). * NetworkMgr: Don't attempt to kill wifi when aborting a connection attempt on `!hasSeamlessWifiToggle` (because, again, it'll break UX, and also because it might run at very awkward times (e.g., I managed to go back to KOReader *between* a FM/Reader switch at one point, which promptly caused `UIManager` to exit because there was nothing to show ;p). * NetworkMgr: Don't drop the connectivity callback when `beforeWifiAction` is set to prompt and the target happens to use a connectivity check in its `turnOnWifi` implementation (e.g., on Kindle). * Android: Add an `"ignore"` `beforeWifiAction` mode, that'll do nothing but schedule the connectivity check with its callback (with the intent being the system will eventually enable wifi on its own Soon(TM)). If you're already online, the callback will run immediately, obviously. If you followed the early discussions on this PR, this closely matches what happens on `!hasWifiToggle` platforms (as flagging Android that way was one of the possible approaches here). * NetworkMgr: Bail out early in `goOnlineToRun` if `beforeWifiAction` isn't `"turn_on"`. Prompt cannot work there, and while ignore technically could, it would serve very little purpose given its intended use case. * KOSync: Neuter the Resume/Suspend handlers early on `CloseDocument`, as this is how focus switches are handled on Android, and if `beforeWifiAction` is `turn_on` and you were offline at the time, we'd trip them because of the swap to system settings to enable wifi. * KOSync: Allow `auto_sync` to be enabled regardless of the `beforeWifiAction` mode on `!hasSeamlessWifiToggle` platforms. Prompt is still a terrible idea, but given that `goOnlineToRun` now aborts early if the mode is not supported, it's less of a problem. --- frontend/device/android/device.lua | 1 + frontend/device/generic/device.lua | 1 + frontend/ui/network/manager.lua | 182 ++++++++++++++++++++++---- plugins/autosuspend.koplugin/main.lua | 5 +- plugins/kosync.koplugin/main.lua | 11 +- 5 files changed, 168 insertions(+), 32 deletions(-) diff --git a/frontend/device/android/device.lua b/frontend/device/android/device.lua index 76d58daf8..a7a9bd218 100644 --- a/frontend/device/android/device.lua +++ b/frontend/device/android/device.lua @@ -74,6 +74,7 @@ local Device = Generic:extend{ model = android.prop.product, hasKeys = yes, hasDPad = no, + hasSeamlessWifiToggle = no, -- Requires losing focus to the sytem's network settings and user interaction hasExitOptions = no, hasEinkScreen = function() return android.isEink() end, hasColorScreen = android.isColorScreen, diff --git a/frontend/device/generic/device.lua b/frontend/device/generic/device.lua index ed93dc040..5b3555599 100644 --- a/frontend/device/generic/device.lua +++ b/frontend/device/generic/device.lua @@ -46,6 +46,7 @@ local Device = { hasExitOptions = yes, hasFewKeys = no, hasWifiToggle = yes, + hasSeamlessWifiToggle = yes, -- Can toggle Wi-Fi without focus loss and extra user interaction (i.e., not Android) hasWifiManager = no, hasWifiRestore = no, isDefaultFullscreen = yes, diff --git a/frontend/ui/network/manager.lua b/frontend/ui/network/manager.lua index e8de416d1..db7399abb 100644 --- a/frontend/ui/network/manager.lua +++ b/frontend/ui/network/manager.lua @@ -19,11 +19,17 @@ local T = ffiutil.template -- We'll need a bunch of stuff for getifaddrs in NetworkMgr:ifHasAnAddress require("ffi/posix_h") +-- We unfortunately don't have that one in ffi/posix_h :/ +local EBUSY = 16 + local NetworkMgr = { is_wifi_on = false, is_connected = false, - pending_connectivity_check = false, interface = nil, + + pending_connectivity_check = false, + pending_connection = false, + _before_action_tripped = nil, } function NetworkMgr:readNWSettings() @@ -33,10 +39,7 @@ end -- Common chunk of stuff we have to do when aborting a connection attempt function NetworkMgr:_abortWifiConnection() -- Cancel any pending connectivity check, because it wouldn't achieve anything - if self.pending_connectivity_check then - UIManager:unschedule(self.connectivityCheck) - self.pending_connectivity_check = false - end + self:unscheduleConnectivityCheck() self.wifi_was_on = false G_reader_settings:makeFalse("wifi_was_on") @@ -45,7 +48,27 @@ function NetworkMgr:_abortWifiConnection() os.execute("pkill -TERM restore-wifi-async.sh 2>/dev/null") end -- We were never connected to begin with, so, no disconnecting broadcast required - self:turnOffWifi() + if Device:hasSeamlessWifiToggle() then + -- We only want to actually kill the WiFi on platforms where we can do that seamlessly. + self:turnOffWifi() + end + -- We're obviously done with this connection attempt + self.pending_connection = false +end + +-- Attempt to deal with platforms that don't guarantee isConnected when turnOnWifi returns, +-- so that we only attempt to connect to WiFi *once* when using the beforeWifiAction framework... +function NetworkMgr:requestToTurnOnWifi(wifi_cb, interactive) + if self.pending_connection then + -- We've already enabled WiFi, don't try again until the earlier attempt succeeds or fails... + return EBUSY + end + + -- Connecting will take a few seconds, broadcast that information so affected modules/plugins can react. + UIManager:broadcastEvent(Event:new("NetworkConnecting")) + self.pending_connection = true + + return self:turnOnWifi(wifi_cb, interactive) end -- Used after restoreWifiAsync() and the turn_on beforeWifiAction to make sure we eventually send a NetworkConnected event, @@ -61,7 +84,6 @@ function NetworkMgr:connectivityCheck(iter, callback, widget) UIManager:close(widget) UIManager:show(InfoMessage:new{ text = _("Error connecting to the network") }) end - self.pending_connectivity_check = false return end @@ -89,6 +111,8 @@ function NetworkMgr:connectivityCheck(iter, callback, widget) end end self.pending_connectivity_check = false + -- We're done, so we can stop blocking concurrent connection attempts + self.pending_connection = false else UIManager:scheduleIn(0.25, self.connectivityCheck, self, iter + 1, callback, widget) end @@ -99,6 +123,11 @@ function NetworkMgr:scheduleConnectivityCheck(callback, widget) UIManager:scheduleIn(0.25, self.connectivityCheck, self, 1, callback, widget) end +function NetworkMgr:unscheduleConnectivityCheck() + UIManager:unschedule(self.connectivityCheck) + self.pending_connectivity_check = false +end + function NetworkMgr:init() Device:initNetworkManager(self) self.interface = self:getNetworkInterfaceName() @@ -247,20 +276,54 @@ end -- Wrappers around turnOnWifi & turnOffWifi with proper Event signaling function NetworkMgr:enableWifi(wifi_cb, connectivity_cb, connectivity_widget, interactive) - -- Connecting will take a few seconds, broadcast that information so affected modules/plugins can react. - UIManager:broadcastEvent(Event:new("NetworkConnecting")) - local status = self:turnOnWifi(wifi_cb, interactive) + local status = self:requestToTurnOnWifi(wifi_cb, interactive) -- If turnOnWifi failed, abort early if status == false then logger.warn("NetworkMgr:enableWifi: Connection failed!") self:_abortWifiConnection() return false - end + elseif status == EBUSY then + logger.warn("NetworkMgr:enableWifi: A previous connection attempt is still ongoing!") + -- We warn, but do keep going on with scheduling the callback iff it was interactive. + -- If it wasn't, it might have been from a beforeWifiAction, and, much like in turnOnWifiAndWaitForConnection, + -- we don't want to risk rescheduling the same thing over and over again. + if interactive then + -- Unlike the next branch, turnOnWifi was *not* called, so we don't need the extra checks. + self:scheduleConnectivityCheck(connectivity_cb, connectivity_widget) + else + -- No connectivity check to handle that for us, so close the widget *now* + if connectivity_widget then + UIManager:close(connectivity_widget) + end + end + return + else + -- Some turnOnWifi implementations may fire a connectivity check, + -- but we *need* our own, because of the callback & widget passing, + -- as we might have been called by the "prompt" beforeWifiAction... + -- NOTE: We *could* arguably have multiple connectivity checks running concurrently, + -- but only having a single one running makes things somewhat easier to follow... + -- NOTE: Also, most of the platforms that use a connectivity check in turnOnWifi do it to handle wifi_cb, + -- so we'll want to preserve it by wrapping both possible callbacks in a single function... + local wrapped_cb + if self.pending_connectivity_check then + self:unscheduleConnectivityCheck() + + wrapped_cb = function() + if wifi_cb then + wifi_cb() + end + if connectivity_cb then + connectivity_cb() + end + end + else + -- If the turnOnWifi implementation didn't rely on a connectivity check, assume wifi_cb was already run. + wrapped_cb = connectivity_cb + end - -- Some turnOnWifi implementations may already have fired a connectivity check... - if not self.pending_connectivity_check then -- This will handle sending the proper Event, manage wifi_was_on, as well as tearing down Wi-Fi in case of failures. - self:scheduleConnectivityCheck(connectivity_cb, connectivity_widget) + self:scheduleConnectivityCheck(wrapped_cb, connectivity_widget) end return true @@ -309,12 +372,21 @@ function NetworkMgr:toggleWifiOff(complete_callback, interactive) end -- NOTE: Only used by the beforeWifiAction framework, so, can never be flagged as "interactive" ;). -function NetworkMgr:promptWifiOn(complete_callback, long_press) +function NetworkMgr:promptWifiOn(complete_callback) + -- If there's already an ongoing connection attempt, don't even display the ConfirmBox, + -- as that's just confusing, especially on Android, because you might have seen the one you tapped "Turn on" on disappear, + -- and be surprised by new ones that popped up out of focus while the system settings were opened... + if self.pending_connection then + -- Like other beforeWifiAction backends, the callback is forfeit anyway + logger.warn("NetworkMgr:promptWifiOn: A previous connection attempt is still ongoing!") + return + end + UIManager:show(ConfirmBox:new{ text = _("Do you want to turn on Wi-Fi?"), ok_text = _("Turn on"), ok_callback = function() - self:toggleWifiOn(complete_callback, long_press) + self:toggleWifiOn(complete_callback) end, }) end @@ -360,23 +432,27 @@ function NetworkMgr:turnOnWifiAndWaitForConnection(callback) UIManager:show(info) UIManager:forceRePaint() - -- This is a slightly tweaked variant of enableWifi, because of our peculiar connectivityCheck usage... - UIManager:broadcastEvent(Event:new("NetworkConnecting")) + -- NOTE: This is a slightly tweaked variant of enableWifi, because of our peculiar connectivityCheck usage... -- Some implementations (usually, hasWifiManager) can report whether they were successfull - local status = self:turnOnWifi() - -- Some implementations may fire a connectivity check, - -- but we *need* our own, because of the callback & widget passing. - if self.pending_connectivity_check then - UIManager:unschedule(self.connectivityCheck) - self.pending_connectivity_check = false - end - + local status = self:requestToTurnOnWifi() -- If turnOnWifi failed, abort early if status == false then logger.warn("NetworkMgr:turnOnWifiAndWaitForConnection: Connection failed!") self:_abortWifiConnection() UIManager:close(info) return false + elseif status == EBUSY then + logger.warn("NetworkMgr:turnOnWifiAndWaitForConnection: A previous connection attempt is still ongoing!") + -- We might lose a callback in case the previous attempt wasn't from the same action, + -- but it's just plain saner to just abort here, as we'd risk calling the same thing over and over... + UIManager:close(info) + return + else + -- Some turnOnWifi implementations may fire a connectivity check, + -- but we *need* our own, because of the callback & widget passing. + if self.pending_connectivity_check then + self:unscheduleConnectivityCheck() + end end self:scheduleConnectivityCheck(callback, info) @@ -384,6 +460,18 @@ function NetworkMgr:turnOnWifiAndWaitForConnection(callback) return info end +-- This is only used on Android, the intent being we assume the system will eventually turn on WiFi on its own in the background... +function NetworkMgr:doNothingAndWaitForConnection(callback) + if self:isWifiOn() and self:isConnected() then + if callback then + callback() + end + return + end + + self:scheduleConnectivityCheck(callback) +end + --- This quirky internal flag is used for the rare beforeWifiAction -> afterWifiAction brackets. function NetworkMgr:clearBeforeActionFlag() self._before_action_tripped = nil @@ -400,6 +488,8 @@ end --- @note: The callback will only run *after* a *succesful* network connection. --- The only guarantee it provides is isConnected (i.e., an IP & a local gateway), --- *NOT* isOnline (i.e., WAN), se be careful with recursive callbacks! +--- Should only return false on *explicit* failures, +--- in which case the backend will already have called _abortWifiConnection function NetworkMgr:beforeWifiAction(callback) -- Remember that we ran, for afterWifiAction... self:setBeforeActionFlag() @@ -407,6 +497,8 @@ function NetworkMgr:beforeWifiAction(callback) local wifi_enable_action = G_reader_settings:readSetting("wifi_enable_action") if wifi_enable_action == "turn_on" then return self:turnOnWifiAndWaitForConnection(callback) + elseif wifi_enable_action == "ignore" then + return self:doNothingAndWaitForConnection(callback) else return self:promptWifiOn(callback) end @@ -557,16 +649,37 @@ function NetworkMgr:goOnlineToRun(callback) return true end + -- If beforeWifiAction isn't turn_on, we're done. + -- We don't want to go behind the user's back by enforcing "turn_on" behavior, + -- and we *cannot* use prompt, as we'll block before handling the popup input... + -- NOTE: Ignore *technically* works, but unlike doNothingAndWaitForConnection, we *would* be displaying an InfoMessage + -- (and block/wait for input as usual). The only difference with turn_on would be the fact that we wouldn't *ever* even try to call turnOnWifi. + -- Given that "ignore" is supposed to be silent, and that you wouldn't actually be able to enable WiFi yourself at that point + -- (because that requires user input, which would cancel the whole thing), there's probably not much to gain by allowing "ignore" here... + if G_reader_settings:readSetting("wifi_enable_action") ~= "turn_on" then + logger.warn("NetworkMgr:goOnlineToRun: Cannot run callback because device is offline and wifi_enable_action is not turn_on") + return false + end + -- We'll do terrible things with this later... local Input = Device.input -- In case we abort before the beforeWifiAction, we won't pass it the callback, but run it ourselves, -- to avoid it firing too late (or at the very least being pinned for too long). local info = self:beforeWifiAction() + -- NOTE: Unlike turnOnWifiAndWaitForConnection, we're not reentrant, + -- so if there's already a connection attempt pending, + -- we can afford to *try* to wait for its success, + -- especially since we can be cancelled. + -- The following call *will* murder any and all pending callbacks though, + -- which is a *slightly* different behavior than turnOnWifiAndWaitForConnection, + -- but a necessity to ensure sane lifecycles... -- We'll basically do the same but in a blocking manner... - UIManager:unschedule(self.connectivityCheck) - self.pending_connectivity_check = false + -- NOTE: Since UIManager won't tick, they wouldn't really have a chance to run anyway... + -- Given the constraints of our callers, they would very likely affect dead/dying objects anyway, + -- so it's much saner to just drop them. + self:unscheduleConnectivityCheck() -- If connecting just plain failed, we're done if info == false then @@ -657,6 +770,8 @@ function NetworkMgr:goOnlineToRun(callback) self:_abortWifiConnection() UIManager:show(InfoMessage:new{ text = _("Error connecting to the network") }) end + -- We're done, reset the pending connection flag, as we don't have any scheduled connectivity check to do it for us. + self.pending_connection = false return success end @@ -782,6 +897,9 @@ function NetworkMgr:getBeforeWifiActionMenuTable() turn_on = {_("turn on"), _("Turn on")}, prompt = {_("prompt"), _("Prompt")}, } + if Device:isAndroid() then + wifi_enable_actions.ignore = {_("ignore"), _("Ignore")} + end local action_table = function(wifi_enable_action) return { text = wifi_enable_actions[wifi_enable_action][2], @@ -794,7 +912,8 @@ function NetworkMgr:getBeforeWifiActionMenuTable() end, } end - return { + + local t = { text_func = function() return T(_("Action when Wi-Fi is off: %1"), wifi_enable_actions[wifi_enable_action_setting][1] @@ -805,6 +924,11 @@ function NetworkMgr:getBeforeWifiActionMenuTable() action_table("prompt"), } } + if Device:isAndroid() then + table.insert(t.sub_item_table, action_table("ignore")) + end + + return t end function NetworkMgr:getAfterWifiActionMenuTable() diff --git a/plugins/autosuspend.koplugin/main.lua b/plugins/autosuspend.koplugin/main.lua index 65f902c9b..892aaf0f1 100644 --- a/plugins/autosuspend.koplugin/main.lua +++ b/plugins/autosuspend.koplugin/main.lua @@ -673,7 +673,10 @@ end function AutoSuspend:onNetworkConnecting() logger.dbg("AutoSuspend: onNetworkConnecting") self:_unschedule_standby() - -- Schedule the next check in 60s. If something goes wrong, the subsequent checks are in `self.auto_standby_timeout_seconds`. + -- Schedule the next check in 60s, which should account for the full duration of NetworkMgr's connectivity check (it times out at 45s), + -- with some extra breathing room. + -- If the connection attempt fails (in which case we get neither a NetworkConnected nor a NetworkDisconnected event), + -- the subsequent checks are in the usual `self.auto_standby_timeout_seconds`. self:_start_standby(time.s(60)) end diff --git a/plugins/kosync.koplugin/main.lua b/plugins/kosync.koplugin/main.lua index f4ef33397..22f7ca3e4 100644 --- a/plugins/kosync.koplugin/main.lua +++ b/plugins/kosync.koplugin/main.lua @@ -84,7 +84,7 @@ function KOSync:init() self.device_id = G_reader_settings:readSetting("device_id") -- Disable auto-sync if beforeWifiAction was reset to "prompt" behind our back... - if self.settings.auto_sync and Device:hasWifiToggle() and G_reader_settings:readSetting("wifi_enable_action") ~= "turn_on" then + if self.settings.auto_sync and Device:hasSeamlessWifiToggle() and G_reader_settings:readSetting("wifi_enable_action") ~= "turn_on" then self.settings.auto_sync = false logger.warn("KOSync: Automatic sync has been disabled because wifi_enable_action is *not* turn_on") end @@ -179,6 +179,9 @@ function KOSync:onReaderReady() self:getProgress(true, false) end) end + -- NOTE: Keep in mind that, on Android, turning on WiFi requires a focus switch, which will trip a Suspend/Resume pair. + -- NetworkMgr will attempt to hide the damage to avoid a useless pull -> push -> pull dance instead of the single pull requested. + -- Plus, if wifi_enable_action is set to prompt, that also avoids stacking three prompts on top of each other... self:registerEvents() self:onDispatcherRegisterActions() @@ -229,7 +232,7 @@ function KOSync:addToMainMenu(menu_items) help_text = _([[This may lead to nagging about toggling WiFi on document close and suspend/resume, depending on the device's connectivity.]]), callback = function() -- Actively recommend switching the before wifi action to "turn_on" instead of prompt, as prompt will just not be practical (or even plain usable) here. - if Device:hasWifiToggle() and G_reader_settings:readSetting("wifi_enable_action") ~= "turn_on" and not self.settings.auto_sync then + if Device:hasSeamlessWifiToggle() and G_reader_settings:readSetting("wifi_enable_action") ~= "turn_on" and not self.settings.auto_sync then UIManager:show(InfoMessage:new{ text = _("You will have to switch the 'Action when Wi-Fi is off' Network setting to 'turn on' to be able to enable this feature!") }) return end @@ -831,6 +834,10 @@ end function KOSync:_onCloseDocument() logger.dbg("KOSync: onCloseDocument") + -- NOTE: Because everything is terrible, on Android, opening the system settings to enable WiFi means we lose focus, + -- and we handle those system focus events via... Suspend & Resume events, so we need to neuter those handlers early. + self.onResume = nil + self.onSuspend = nil -- NOTE: Because we'll lose the document instance on return, we need to *block* until the connection is actually up here, -- we cannot rely on willRerunWhenOnline, because if we're not currently online, -- it *will* return early, and that means the actual callback *will* run *after* teardown of the document instance