Skip to content

NetworkMgr: Handle non-blocking turnOnWifi implementations better #10863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 39 commits into from
Sep 21, 2023

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Aug 30, 2023

  • 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.

Initial outdated message:

Draft, needs testing. Might just not do anything and let the status quo as is.

Because, with this PR as-is, this essentially risks silently losing sync on document close when offline, which I view as much worse than a wifi nag popup.


This change is Reviewable

@NiLuJe
Copy link
Member Author

NiLuJe commented Aug 30, 2023

What needs testing is whether enforcing "turn on" behavior is goOnlineToRun actually works without breaking all kinds of shit in weird and interesting ways.

(Because we know prompt will break, but I'm not entirely sure how the focus loss would affect this).

@NiLuJe
Copy link
Member Author

NiLuJe commented Aug 30, 2023

I'm not entirely sure how the focus loss would affect this

And if it adversely affects it, it's quite likely already broken today anyway.

No time to test this right now, though.

@NiLuJe NiLuJe changed the title Android: Add a dedciated dev cap to denote its UX breaking wifi toggle Android: Add a dedicated dev cap to denote its UX breaking wifi toggle Aug 30, 2023
@Frenzie
Copy link
Member

Frenzie commented Aug 30, 2023

I'm not sure if I quite grok what breaking the wifi toggle means? Do you mean that in making other platforms a bit more like Android, it was accidentally broken a bit along the way? :-P

@NiLuJe
Copy link
Member Author

NiLuJe commented Aug 30, 2023

It's not in-KOReader, you lose focus to the native system settings.

@dlgoodr
Copy link

dlgoodr commented Aug 30, 2023

It's not in-KOReader, you lose focus to the native system settings.

And it's super annoying. I am grateful you're trying to be creative 😁

@mergen3107
Copy link
Contributor

Overall I think KOReader shouldn’t really have controls for Wi-Fi, Brightness and other misc stuff on Android. Android usually allows to control all these by other means (pull down notification curtain, etc).

If an app needs Wi-Fi, it complains so, and then user needs to fix this on their end :D

@Frenzie
Copy link
Member

Frenzie commented Aug 30, 2023

It's a touch more difficult on some of the (ancient?) Android ereader devices I believe but in essence I would agree.

I still don't quite understand what the problem is though; maybe it's explained in some ticket?

@Frenzie
Copy link
Member

Frenzie commented Aug 30, 2023

Put another way, the fact that https://github.com/koreader/koreader/releases/tag/v2023.06.1 was the final release to support Android 4.0 – 4.2 might be relevant, potentially removing the reason it was added in the first place.

@NiLuJe
Copy link
Member Author

NiLuJe commented Aug 30, 2023

I still don't quite understand what the problem is though

Anything that requires WiFi will want to enable it, forcing a focus switch if it isn't already on.

This is already dumb and annoying enough in general, but the very particulars of the KOSync onDocumentClose handling make what's actually viable even trickier.

@pazos
Copy link
Member

pazos commented Sep 1, 2023

@NiLuJe: IIRC the desktop versions don't handle wifi at all. How they behave with kosync when they have no network connection?

Overall I think KOReader shouldn’t really have controls for Wi-Fi, Brightness and other misc stuff on Android. Android usually allows to control all these by other means (pull down notification curtain, etc).

Totally agree with the wifi thing. Totally disagree with brightness.

I already enjoy so much to be able to use gestures to set brightness, like in any other plaftorm. And that behaviour works fine in 99'99% of the android devices out there. We're currently using the recommended method of dealing with in-app brightness without affecting system wide settings (pretty much like VLC, for instance, is doing).

For some e-ink devices I understand it is a pain and it is easier to forget about the inner controls and just rely on the system. But that's something the end user can do actually, without screwing up other user that take advantage of the feature.

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 1, 2023

How they behave with kosync when they have no network connection?

They're flagged !hasWifiToggle, so the code assumes WiFi is always available (and will let networking stuff fall on its ass and/or timeout if it's not).

That's one of the options on the table here (and, obviously, the easier, because I don't have anything to do ;p).

@NiLuJe NiLuJe force-pushed the android_wifi_toggle branch from 0e8e01c to 1f3f122 Compare September 8, 2023 16:08
i.e., behave as if it were ù*not* flagged hasWifiToggle (because it
really shoulmdn't be).

A case could be made to just nip this in the bud and stop flagging it
entirely, but there are a *few* nice to have things hidden behind that
capcheck, so, for now, just special-case this one...

Fix koreader#10790 (comment)
!hasWifiToggle

Instead, add an Android-specific before <wifi action that does
*nothing*, it'll just run the callback if online, or schedule a
connectivity check to wait for that to happen automatically.

This is intended for devices where the system already manages WiFi
somewhat sensibly on its own.
@NiLuJe NiLuJe force-pushed the android_wifi_toggle branch from 3d3f3e7 to 7417e96 Compare September 18, 2023 16:55
until we're back in focus

That's not at all how I imagined this would work.

This means willRerun basically loops for ever if you return, because
isConnected is never true. Joy.

That's score one for !hasWifiToggle, methinks
…se of the focus switch from opening the system settings...
i.e., drop the subsequent callbacks entirely.

We're unlikely to have different ones running concurrently, anyway,
so the original one still iterating in the original connectivity check
is likely to be enough.
Nothing actually sets the pending connection flag to true, so we
wouldn't be delaying anything... assuming nothing actually unschedules
the connectivity check.
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

Okay, I've got something that appears to behave.

As a bonus, I actually fixed turn_on so that it's somewhat usable on Android, where turnOnWifi returns immediately, and as such doesn't guarantee isConnected, which the beforeWifiAction stuff kinda assumed.

This meant that, for instance, if you returned from the wifi settings without doing anything, you'd be thrown back to it... indefinitely (yay?). (Assuming you're actually offline, e.g., I tested on a phone in Airplane mode).

Anyway, a bit of extra clunk in NetworkManager now ensures we only call turnOnWifi once until that attempt either fails or succeeds.

What happens to subsequent callbacks if an attempt is still pending... varies (but generally, you shouldn't expect the newer ones to persist). (In practice, if it's an interactive callback, it'll probably persist, but if it's a beforeWifiAction shenanigan, it'll be dropped).

As far as Android goes, I've also added an "Ignore" before wifi action mode, which... will do nothing but silently schedule the callback in a connectivity check, in a vain hope that the system will eventually connect on its own ;).
I've currently allowed goOnlineToRun in ignore mode, but the constraints of that method means that one is not silent, and requires a tap to cancel (as usual).

I'm not sure if it's great and/or useful, but given that the only caller is KoSync, and it runs on CloseDocument, if you have wifi always on and handled by the system, it's liable to be on at that point; I think the behavior matters more for stuff that happens on wakeup from suspend, and I didn't want to break sync on close in that mode, because that's arguably the most important sync ;p.

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

Still need to test that I didn't horribly break stuff on Kobo, though ;).

(Note to self: also, a few stray debug logs left).

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

I'm not sure if it's great and/or useful

Doubly so because the only thing that could actually enable wifi at that point is the system: if you tried to open the WiFi settings yourself, that would involve input that would cancel the whole thing and close the document ;).

Note that all of this only applies when offline, if you're online, the sync happens no matter your before_wifi setting ;).

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

Anyway, a bit of extra clunk in NetworkManager now ensures we only call turnOnWifi once until that attempt either fails or succeeds.

The other approach would have been something Android-specific to make sure turnOnWifi only returns once we exit the settings and focus back to KOReader, but I'm not even sure that's doable (e.g., possibly something like the lights dialog stuff?).
At which point we could at least check if we're still not connected at all, as NetworkManager handles turnOnWifi returning false in those cases. (That would at least have prevented the infinite loop if you back out of the settings ;p).

Not really interested in that approach anymore, because what I went with might actually help other async platforms (e.g., Kindle), but, for science, would that be technically feasible, @pazos?

The contract with ignore is that you're okay about stuff not happening
if you're offline, and you wouldn't be able to toggle wifi yourself
during the wait in this specific case, so it doesn't make much sense.

If you're online, stuff works just fine, which is good enough for me.
By dropping non-interactive callbacks if busy
i.e., if there are pending connectivity checks, cancel them, so that the
one that wa sjust requested actually rusn (because that one has a
callback, while the one from turnOnWifi imps won't).
checks like turnOnAndWait... ;p

Do it right, this time around.
@NiLuJe NiLuJe marked this pull request as ready for review September 19, 2023 18:51
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

Okay, tested on Android, Kindle & Kobo, this ought to be ready now :}

@NiLuJe NiLuJe changed the title Android: Add a dedicated dev cap to denote its UX breaking wifi toggle NetworkMgr: Handle non-blocking turnOnWifi implementations better Sep 19, 2023
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

Updated the first post with the tentative commit log for the squash.

@Frenzie
Copy link
Member

Frenzie commented Sep 19, 2023

Might as well throw it in on account of the OTA situation

@NiLuJe NiLuJe added this to the 2023.09 milestone Sep 19, 2023
i.e., only for the actual connection attempt, not the concurrent ones.
For the standard menu toggle, the actual callback is wifi_cb ;).
Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2023

More Android testing and prompt fixes, because prompt is definitely the absolute worst ;).

@NiLuJe NiLuJe merged commit 34ba2fa into koreader:master Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants