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

avm2: stub desktop AIR APIs #17299

Merged
merged 1 commit into from
Sep 8, 2024
Merged

avm2: stub desktop AIR APIs #17299

merged 1 commit into from
Sep 8, 2024

Conversation

Mesteery
Copy link
Contributor

@Mesteery Mesteery commented Jul 29, 2024

No description provided.

@Mesteery Mesteery changed the title Add some AIR desktop/display APIs stub avm2: Add some AIR desktop/display APIs stub Jul 29, 2024
@evilpie
Copy link
Collaborator

evilpie commented Jul 29, 2024

Can you remove the AIR argv handling from this PR? I think we shouldn't combine it.

@Mesteery
Copy link
Contributor Author

Can you remove the AIR argv handling from this PR? I think we shouldn't combine it.

I put air argv handling in the same PR because it's needed by NativeApplication.
But yes I can split if you want.

@Mesteery Mesteery force-pushed the air-desktop branch 3 times, most recently from 80f8fda to 69a75a5 Compare August 5, 2024 00:39
@evilpie evilpie added the waiting-on-review Waiting on review from a Ruffle team member label Aug 18, 2024
@evilpie
Copy link
Collaborator

evilpie commented Aug 21, 2024

This PR is huge and does a lot of different stuff in one commit. @Mesteery if you split it up it will be easier and quicker to review.
Please add least split out the air_arguments and screen_size code.

@Mesteery
Copy link
Contributor Author

Mesteery commented Aug 22, 2024

Split into #17609 and #17608

@Mesteery Mesteery changed the title avm2: Add some AIR desktop/display APIs stub avm2: stub desktop AIR APIs Aug 22, 2024
@evilpie evilpie added waiting-on-author Waiting on the PR author to make the requested changes and removed waiting-on-review Waiting on review from a Ruffle team member labels Aug 24, 2024
@Mesteery
Copy link
Contributor Author

Thanks you for your review. I've normally addressed all the changes

@torokati44
Copy link
Member

Thank you! 🥳 I'll take another look, and still have two files left to read through - but gotta sleep first. 😌😴

Copy link
Member

@torokati44 torokati44 left a comment

Choose a reason for hiding this comment

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

I think this is all from me. Thank you for dealing with all my nitpicking!
I'll squash-merge later today unless anyone else objects, or finds something else.

@torokati44 torokati44 removed the waiting-on-author Waiting on the PR author to make the requested changes label Aug 25, 2024
setTimeout(function():void
{
dispatchEvent(new InvokeEvent(InvokeEvent.INVOKE, false, false, null, []));
}, 500);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this is fake behavior - can you add some comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with #17609, [] will be replaced by the real arguments passed to the application, but it stay fake behaviour I think

Copy link
Collaborator

@adrian17 adrian17 Aug 25, 2024

Choose a reason for hiding this comment

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

Oh, sorry for not specifying - I mostly mean the 500ms delay. If you have an intuition of when this should actually trigger in application lifecycle, can you add it as a comment? If not, that's okay, just a // TODO will be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know when INVOKE is triggered in AIR so I put a delay in case, so there are no issues.

Copy link
Collaborator

@Lord-McSweeney Lord-McSweeney Aug 28, 2024

Choose a reason for hiding this comment

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

InvokeEvent objects are dispatched by the NativeApplication object (NativeApplication.nativeApplication). To receive invoke events, call the addEventListener() method of the NativeApplication object. When an event listener registers for an invoke event, it will also receive all invoke events that occurred before the registration.

And in airglobal I can see an override public function addEventListener for NativeApplication.

import __ruffle__.stub_getter;
// According to the documentation, it should be [API("661")]
// but airglobal.swc disagrees with that:
[API("667")]
public class NativeMenuItem extends EventDispatcher {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in FP, ContextMenu(Item) derives from NativeMenu(Item). Do we not want to do it here, or would it just complicate the current code too much?

Copy link
Member

Choose a reason for hiding this comment

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

I have a confuse.
We already have class ContextMenuItem extends NativeMenuItem and class ContextMenu extends NativeMenu. Do you mean that this version gate could cause problems for there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you're right!
The APIs between these two are super confusing tbh; I thought they'd overlap at least a little bit (as in, ContextMenuItem use some fields from the NativeMenuItem because why else use inheritance), but... no? ContextMenuItem has caption, while NativeMenuItem has a label etc? Super weird :P I wonder why there's inheritance at all then.

Anyway, if there's no expected conflict between fields/methods of these two pairs at all, then this can be closed :D

Copy link
Member

Choose a reason for hiding this comment

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

Alright. :D
Could you please confirm that NativeMenu[Item] getting version-gated away won't mess with ContextMenu[Item] being usable from non-AIR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It shouldn't, playerglobal gets the highest versions

@evilpie evilpie added the air Adobe AIR label Aug 25, 2024

[API("661")]
public function get nativeWindow():NativeWindow {
if (!this._nativeWindow) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably do a stub warning

Copy link
Member

Choose a reason for hiding this comment

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

Added.

{
private static var _instance:NativeApplication;

public var _openedWindows:Array = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any way we could make sure that this isn't public?

Copy link
Member

Choose a reason for hiding this comment

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

Made the access of it go through the getter. I assume it returns a reference to this array.

@evilpie evilpie added the waiting-on-author Waiting on the PR author to make the requested changes label Aug 31, 2024
@torokati44 torokati44 added waiting-on-review Waiting on review from a Ruffle team member and removed waiting-on-author Waiting on the PR author to make the requested changes labels Sep 5, 2024
@evilpie evilpie merged commit b616f55 into ruffle-rs:master Sep 8, 2024
16 of 17 checks passed
@torokati44
Copy link
Member

Thank you for addressing all the review notes!

@evilpie evilpie removed the waiting-on-review Waiting on review from a Ruffle team member label Sep 8, 2024
@Dinnerbone Dinnerbone added A-avm2 T-compat Type: Compatibility with Flash Player A-avm2 Area: AVM2 (ActionScript 3) labels Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-avm2 Area: AVM2 (ActionScript 3) air Adobe AIR newsworthy T-compat Type: Compatibility with Flash Player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants