core: implement displayState for Stage in avm1/2

[hatal175]

Relevant to several issues (#1562, #4785, #274).

This pull request implements the displayState member for Stage for both avm1 and avm2. I had to rewrite several parts so fullscreen actions would go through the Stage class in core.
I've also implemented the full screen events for both AVMs.

I would say that the non-interactive part of full screen is not implemented.

Several issues that I've run into are the following:

1. weird rendering issues in log in desktop about extent. It seemed to still go to full screen, so yay.
2. in web, the fullscreen functions are asynchronous. This was happily ignored before and still is but it could cause issues.
3. the state can be fullScreenInteractive even though you set it to fullScreen. No clue what's that about.

[hatal175]

Not sure what's up with that check. Doesn't seem related to change.

[torokati44]

I have seen that same check spuriously fail in a different PR as well.
The error message element ("<ruffle-object />") still not existing after 30000ms hints that something could have hanged, or possibly there is a race condition?

[kmeisthax]

The error you're getting is a recurring problem with our CI infrastructure. Sometimes the web tests just fail to create the Ruffle element correctly, and we haven't been able to determine why. The solution in every case is to just force the test to run again.

[adrian17]

A general thing I just noticed in docs:

Full-screen mode is initiated in response to a mouse click or key press by the user; the movie cannot change Stage.displayState without user input

While Flash Player is in full-screen mode, all keyboard input is disabled (except keyboard shortcuts that take the user out of full-screen mode).

I imagine the latter isn't really relevant anymore now that we use "browser fullscreen", but the former sounds reasonable - we probably don't want movies to fullscreen themselves at will? Or do browsers handle that automatically even if there's wasm in the middle?

I'm not saying it's a blocker or obligatory to implement, but that does sound like something we should maybe talk about here or on discord?

As for AVM2 stuff, probably best for kmeisthax to review this.

[hatal175]

Only within an html page and that's actually enforced by browser. See note within Element.requestFullscreen() Documentation in web API.

…

On Sun, Aug 29, 2021, 20:13 Adrian Wielgosik ***@***.***> wrote: A general thing I just noticed in docs: Full-screen mode is initiated in response to a mouse click or key press by the user; the movie cannot change Stage.displayState without user input While Flash Player is in full-screen mode, all keyboard input is disabled (except keyboard shortcuts that take the user out of full-screen mode). I imagine the latter isn't really relevant anymore now that we use "browser fullscreen", but the former sounds reasonable - we probably don't want movies to fullscreen themselves at will? Or do browsers handle that automatically even if there's wasm in the middle? I'm not saying it's a blocker or obligatory to implement, but that does sound like something we should maybe talk about here or on discord? As for AVM2 stuff, probably best for kmeisthax to review this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYQXXIVXWTG6VQQYX3ONRDT7JTDRANCNFSM5CYQSLMQ> .

[kmeisthax]

I have one very minor nitpick, and it's not even with the AVM2 stuff. Everything else seemed straightforward.

[danielhjacobs]

I don't know if you meant to (I see a force push and a review request so I assume you did) but you never changed the name of the method run_context_menu_fullscreen_callback despite marking the conversation as resolved.

[hatal175]

I might have misunderstood. I thought he talked about the parameter name. That said, it's pretty much like another context function...

…

On Fri, Sep 3, 2021, 19:50 Daniel Jacobs ***@***.***> wrote: I don't know if you meant to (I see a force push and a review request so I assume you did) but you never changed the name of the method run_context_menu_fullscreen_callback despite marking the conversation as resolved. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYQXXNCGKKPP3EXDVXXHG3UAD4EVANCNFSM5CYQSLMQ> .

[danielhjacobs]

Yeah, it's consistent with the names @adrian17 gave the functions added in #4173. I can't say I like those names, but it does match what already exists, so perhaps it should be kept. That would be @kmeisthax 's (or the other core dev's) call though.

[adrian17]

That said, it's pretty much like another context function...

Yeah, it's consistent with the names @adrian17 gave the functions added in #4173

Not really. The intention behind run_context_menu_callback is to be a single interface from clicked JS context menu item to the Rust/AS callback function attached to it. IMO if we wanted to be consistent with that interface, instead of adding run_context_menu_fullscreen_callback, the Fullscreen button should be handled the same way as all other builtin context menu items, and there should be another branch added to the former function, like this:

ContextMenuCallback::Play => Self::toggle_play_root_movie(context),
ContextMenuCallback::Forward => Self::forward_root_movie(context),
ContextMenuCallback::Back => Self::back_root_movie(context),
ContextMenuCallback::Rewind => Self::rewind_root_movie(context),
ContextMenuCallback::Fullscreen=> Self::toggle_fullscreen(context),
// or: Enable/DisableFullscreen pair?
// or: ContextMenuCallback::Fullscreen(value) => Self::set_fullscreen(context, value),

That said, not sure how much of this refactor should be part of this PR.

[hatal175]

Renamed function.

[danielhjacobs]

IMO if we wanted to be consistent with that interface, instead of adding run_context_menu_fullscreen_callback, the Fullscreen button should be handled the same way as all other builtin context menu items, and there should be another branch added to the former function

Then it would be hidden by menu="0", and I disagree with doing that personally as a user, as I mentioned here: #4258 (comment)

[adrian17]

IMO, using a single API internally doesn't mean that internal logic can't be changed to always show Fullscreen.

Anyway, this can still be done in a separate PR if needed; as for now, the set_fullscreen is a nice rename, thanks :)

[ousia]

Any news on this?

[hatal175]

Still waiting for an ok on this. I don't want to keep rebasing every time.

[ousia]

@hatal175, I wonder whether @kmeisthax could review and approve the changes (and maybe even commit the pull request).

[kmeisthax]

@hatal175 Sorry for the wait, I'll merge this on the next rebase

[ousia]

Many thanks for the implementation, @hatal175.