Skip to content
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

Add Occluded events based on foreground/background on iOS and MemoryWarning on iOS/Android #2980

Merged
merged 15 commits into from
Oct 12, 2023

Conversation

mockersf
Copy link
Contributor

Adopted from #2144, and updated.

From #2144 (comment) I'm not confident on adding events Foreground and Background to Android, they seem to be equivalent to Suspend / Resume where iOS split that in two steps.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@madsmtm
Copy link
Member

madsmtm commented Jul 28, 2023

That's really nice, thank you!

From my reading, to be maximally cross-platform compatible, it sounds like we should send an artificial Background event just before Suspend on Android, and an artificial Foreground just after Resumed (on all platforms, actually)?

@madsmtm
Copy link
Member

madsmtm commented Jul 28, 2023

Hmm, actually it sounds like applicationDidEnterBackground: matches the semantics of the Suspend event on Android better than applicationWillResignActive:?

@madsmtm
Copy link
Member

madsmtm commented Jul 28, 2023

@rib: Is there an event on Android that requests the application to pause things (e.g. pause the game) and go into a low-power state, without necessarily suspending everything?

@mockersf mockersf force-pushed the add_lifetime_events branch from 85fe4f5 to 60a597f Compare July 29, 2023 00:59
@mockersf
Copy link
Contributor Author

mockersf commented Aug 1, 2023

From my reading, to be maximally cross-platform compatible, it sounds like we should send an artificial Background event just before Suspend on Android, and an artificial Foreground just after Resumed (on all platforms, actually)?

Is it OK if I add those artificial events on other platforms, with a TODO to move them to the correct events on Android for now?

@mockersf mockersf force-pushed the add_lifetime_events branch from cd0ddbe to 3d1bf95 Compare August 2, 2023 21:00
@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

Web should probably use these for the pagehide and pageshow events, currently it uses Occluded instead.
Should we also emit Occluded with Foreground and Background?
Should this be folded into the new WindowState (#2929)?

@kchibisov
Copy link
Member

If the whole intent is to stop rendering based on foreground/background Occluded sounds like a better choice.

@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

My suggestion was to use both, Occluded and Foreground/Background.

Maybe I'm misunderstanding Foreground/Background, my assumption was that if something is in the background it's not visible, ergo occluded.

@kchibisov
Copy link
Member

I mean, what's the reason to add them, if Occluded could indicate Foreground/Background?

@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

I mean, what's the reason to add them, if Occluded could indicate Foreground/Background?

For the same reason this PR adds Foreground/Background: it's interesting to some applications. Occluded can mean different things, not only that it's Foreground/Background.

@kchibisov
Copy link
Member

@daxpedda it's simply a hint saying that you could stop rendering.

@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

I mean, what's the reason to add them, if Occluded could indicate Foreground/Background?

@daxpedda it's simply a hint saying that you could stop rendering.

I agree, that's why I am proposing to send both, because Occluded should not indicate anything else then to stop rendering.

@kchibisov
Copy link
Member

@daxpedda I'm just arguing against adding special events for ios where we already can use existing Occluded event.

@mockersf
Copy link
Contributor Author

mockersf commented Aug 4, 2023

Occluded is a WindowEvent, while Foreground/Background are application events. When dealing with them, you're not bound to any specific window, and putting them as WindowEventwould (in my opinion) break some of their meaning.

That said, that limit on iOS at least is quite blurry as it's not possible to have more than one window, so it may be ok to mix those there...

@kchibisov
Copy link
Member

kchibisov commented Aug 5, 2023

Occluded is a WindowEvent, while Foreground/Background are application events. When dealing with them, you're not bound to any specific window, and putting them as WindowEventwould (in my opinion) break some of their meaning.

That said, that limit on iOS at least is quite blurry as it's not possible to have more than one window, so it may be ok to mix those there...

I mean, as long as window is not destroyed it's fine to use Occluded for that. You can even send them for each window in a loop if multiple windows will be a thing. The good thing about this event is that you'll get expected handling on other platforms, because they usually stop rendering when occluded happens.

@mockersf
Copy link
Contributor Author

Moved to WindowEvent::Occluded

@mockersf mockersf changed the title Add events Foreground, Background on iOS and MemoryWarning on iOS/Android Add Occluded events based on foreground/background on iOS and MemoryWarning on iOS/Android Sep 18, 2023
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/event.rs Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov merged commit 93f1000 into rust-windowing:master Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants