avm2: Add actionscript event classes #7597
Conversation
|
I'm not super familiar with the new AS classes process, but I believe you need to add these to https://github.com/ruffle-rs/ruffle/blob/master/core/src/avm2/globals/globals.as. |
|
Almost all |
|
Thanks for pointing these out, should now be fixed |
| // [override] Creates a copy of the UncaughtErrorEvent object and sets the value of each property to match that of the original. | ||
| override public function clone():Event | ||
| { | ||
| return new UncaughtErrorEvent(this.type, this.bubbles, this.cancelable, this.text, this.errorID, ); |
There was a problem hiding this comment.
As the CI checks pointed out, there is a syntax error here, at the end of the line.
|
Thank you for pointing out the syntax errors, this latest commit would not build on my computer due to lack of known types namely |
|
It looks like some of these have been implemented in rust but not exposed to actionscript, if I am understanding things correctly. |
|
As you mentioned in your latest comment some are implemented with Rust. To allow the Actionscript classes to recognize them, add them to https://github.com/ruffle-rs/ruffle/blob/master/core/src/avm2/globals/stubs.as. I do not believe the ones that have not been implemented can be used unfortunately, but I may be mistaken. |
|
See 5817c17 for an example of adding a stub |
|
A couple of event classes (for example |
|
Of the ones commented out, the following are AIR-only:
Of the ones not commented out, the following are AIR-only:
While I don't think there would be too much harm in keeping them, they are currently completely useless, since they would never appear in any non-AIR files. See the classes with the AIR-only symbol on https://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/events/package-detail.html. Edit: You could get all the non-AIR ones by doing |
|
Yeah, they would only be unnecessary noise IMHO. Thanks for checking them @danielhjacobs! |
|
Thanks for letting me know! I wasn't sure if AIR was going to be supported so I left it in. I'll make those adjustments |
|
It's not out of the question for AIR to one day be supported, but it's not within the current or near-future scope. I know it's something Mike (repo owner and creator of the Ruffle project) has said he wants to support someday:
He said this on the #sponsors channel on Discord (only visible to sponsors of the project, mods, and people who have been granted the developer role - usually granted after someone has had a few PRs merged from what I've seen). Edit: Basically, I'd just say to keep that commit you have that removes the AIR-only classes (just as you have done), and don't force-push things together. That way it's easy to revert the removal in the future if/when we start trying to support AIR classes. |
|
This might be nit-picking, and I understand that this is due to the generated nature of the code, but still, looking at the final result: |
|
I agree, and have made the edits |
|
I'm not trying to be a downer all the time, but TBH I'm not a fan of adding a bunch of unused files (those referring to unknown types), and commented out Don't get me wrong, I really appreciate the effort you put into this, and even in the tool you did it with, and it will be really nice to have a bunch of missing classes defined in one swell foop. There are a couple of them that several pieces of content I'd like to see working have been missing! But we should be careful not to put quantity over quality. I'm open to be voted down on this though. |
|
I wouldn't say you're being a downer, for these classes the code is relatively easy to recreate so I am happy to split this apart into several PRs as ruffle becomes more mature. I am going to have to do that anyway as this PR originally didn't have every event inside it, just the ones I could easily create at the time. The feedback is also useful for improving my generation tool and myself as a new contributor. That being said, I can reorganize the commits for this PR in this way:
|
|
I would personally suggest that reorganization of code exactly as you described. That said, I'd like another opinion, since I rarely contribute to core. I also noticed two things:
Additionally, I'm unsure if other developers may request tests be added for some of these classes. Tests are one of the larger barriers to entry for new contributors, and they can theoretically be added in a follow-up PR, so you may not need to add them now depending what other developers think. You can see https://github.com/ruffle-rs/ruffle/blob/HEAD/CONTRIBUTING.md#test-guidelines for information about adding tests. That said, perhaps tests are overkill for purely ActionScript-based classes. |
|
Good observations, @danielhjacobs!
Also great point, but IMHO these event classes are so simple, being almost "plain old data"-like, having only a bunch of properties and no real functionality in themselves, that unit/regression tests are not really useful for them. What I think should be done, is a manual overview of all the classes, to see if everything is as it should be in them. But that's just my pair of pennies. |
|
The only thing that I think needs attention is whether to keep |
|
Following advice from the discord, the |
| // Returns a string that contains all the properties of the AVHTTPStatusEvent object. | ||
| override public function toString():String | ||
| { | ||
| return this.formatToString("AVHTTPStatusEvent","type","bubbles","cancelable","eventPhase","status"); |
There was a problem hiding this comment.
Have you verified the order of these properties in the result in FP?
There was a problem hiding this comment.
No, I just looked at the "format" piece of code from the documentation, the reason I added the "eventPhase" to these was because I noticed it added in already existing event code. I now have a proper AS3 and FP Dev Environment set up to check these against what FP spits out and for this event class in particular it does spits out the responseURL and responseHeaders.
There was a problem hiding this comment.
Ah, didn't notice that line in the docs for toString. That's good as a starting point. And again, I think it's fine to assume that it is correct (although, you're saying that in this example it is in fact not?), and then fix these minor things later when they come up.
There was a problem hiding this comment.
(It's also these things, along with whether clone() leaves something out, that could use some testing via comparison with FP, but maybe this too, is a "perfect is the enemy of good" situation, as I read recently on Discord... :) )
| // Returns a string that contains all the properties of the AccelerometerEvent object. | ||
| override public function toString():String | ||
| { | ||
| return this.formatToString("AccelerometerEvent","type","bubbles","cancelable","eventPhase","timestamp","accelerationX","accelerationY","accelerationZ"); |
There was a problem hiding this comment.
(And the same question for all toString() overrides as above.)
| public var timestamp: Number; // The number of milliseconds at the time of the event since the runtime was initialized. | ||
| public var accelerationX: Number; // Acceleration along the x-axis, measured in Gs. | ||
| public var accelerationY: Number; // Acceleration along the y-axis, measured in Gs. | ||
| public var accelerationZ: Number; // Acceleration along the z-axis, measured in Gs. |
There was a problem hiding this comment.
Have you checked the order of declaration for the fields - using describeType()?
Same question for all classes.
There was a problem hiding this comment.
I used the order of the inputs into the constructor with any additional properties not connected to an input from the constructor afterwards. That was my best guess based purely off the documentation when I wrote the generating code.
There was a problem hiding this comment.
I see. See the relevant conversation at: #7558 (review)
I did reorder the stuff in Vector3D accordingly. But again, this is not crucial, and as said above, in many cases we already have a mismatch with FP.
|
Do you think we should maybe add a little note in a comment at the top of every newly added, autogenerated file, stating something like "this file was autogenerated from the official AS3 reference at |
|
I think the formatting of the code could use some adjustments (I saw a bit too many empty lines for my taste), but I guess that can be addressed together with #7666, if this is merged first. Also, any potential funky business (bugs, omissions) in |
|
Was |
Yes, here is the message I currently thought up:
Agreed, if that does get merged first then this PR will be changed to reflect the recommendations the linter will make.
This makes me feel that at the moment, until issues are encountered, I should stick with my first guesses about the
Yes, after removing the AIR-only properties there wasn't much left and felt like it was better to leave out; that said I am not iron clad on this and can add it back in |
|
Also as part of the next rebase/confict resolution I am thinking of putting the |
That sounds about right. Maybe the first line could be split into two somewhere, but this is also nitpicking... :)
Yeah, I personally think this is fine. But if anyone else (of any other reviewers) feels like this is too negligent, feel free to override me on this.
Yeah, that would be good. |
| public var ctrlKey: Boolean; // On Windows or Linux, indicates whether the Ctrl key is active (true) or inactive (false). | ||
| public var altKey: Boolean; // Indicates whether the Alt key is active (true) or inactive (false). | ||
| public var shiftKey: Boolean; // Indicates whether the Shift key is active (true) or inactive (false). | ||
| public var commandKey: Boolean; // Indicates whether the command key is activated (Mac only). |
There was a problem hiding this comment.
I think this property is also AIR-only.
| package flash.events | ||
| { | ||
|
|
||
| public class GestureEvent extends Event |
There was a problem hiding this comment.
I think updateAfterEvent() is missing here. It is also not present in MouseEvent ... but why could that be?
There was a problem hiding this comment.
I saw that it was missing in MouseEvent and replicated that behavior, maybe at the time (even now?) the API isn't complete enough to be able to do it?
There was a problem hiding this comment.
Yeah, that's true, it wouldn't work anyway, and it's just not stubbed either.
| package flash.events | ||
| { | ||
|
|
||
| public final class PermissionEvent extends Event |
There was a problem hiding this comment.
Perhaps this class should also be removed, as the status property, and all 3 methods (incl. the ctor.) are marked in the docs as AIR-only.
| package flash.events | ||
| { | ||
|
|
||
| public class StageOrientationEvent extends Event |
There was a problem hiding this comment.
I think this entire class is also marked as AIR-only.
| public static const RENDER_STATUS_ACCELERATED:String = "accelerated"; // Deprecated since Flash Player 10.2, AIR 3: Please Use flash.media.VideoStatus.ACCELERATEDIndicates that hardware is decoding and displaying the video. | ||
| public static const RENDER_STATUS_SOFTWARE:String = "software"; // Deprecated since Flash Player 10.2, AIR 3: Please Use flash.media.VideoStatus.SOFTWAREIndicates that software is decoding and displaying the video. | ||
| public static const RENDER_STATUS_UNAVAILABLE:String = "unavailable"; // Deprecated since Flash Player 10.2, AIR 3: Please Use flash.media.VideoStatus.UNAVAILABLEIndicates that displaying the video using the StageVideo object is not possible. |
There was a problem hiding this comment.
These three constants are also AIR-only.
There was a problem hiding this comment.
Additionally, there is a missing space in all their comments:
... Please Use flash.media.VideoStatus.SOFTWAREIndicates that software ...
| package flash.events | ||
| { | ||
|
|
||
| public class VsyncStateChangeAvailabilityEvent extends Event |
There was a problem hiding this comment.
This class, along with all of its members, is also marked as AIR-only.
|
Sorry, I am not sure why it says it's out of date; but I ended up updating and force-pushing twice. |
This comment was marked as resolved.
This comment was marked as resolved.
core/src/avm2/globals.rs
Outdated
| @@ -90,6 +90,7 @@ pub struct SystemClasses<'gc> { | |||
| pub sharedobject: ClassObject<'gc>, | |||
| pub mouseevent: ClassObject<'gc>, | |||
| pub progressevent: ClassObject<'gc>, | |||
| pub gestureevent: ClassObject<'gc>, | |||
There was a problem hiding this comment.
This can be removed, as this field isn't used anywhere
core/src/avm2/globals.rs
Outdated
| @@ -719,6 +721,7 @@ fn load_playerglobal<'gc>( | |||
| ("flash.events", "IOErrorEvent", ioerrorevent), | |||
| ("flash.events", "MouseEvent", mouseevent), | |||
| ("flash.events", "FullScreenEvent", fullscreenevent), | |||
| ("flash.events", "GestureEvent", gestureevent), | |||
| ("flash.geom", "Matrix", matrix), | |||
This PR would add 29 event classes partially generated by this tool I made. This PR focuses on the classes which are simple in structure and are not much more then some properties with a
cloneandtoString. The comments describing each part are copied from the documentation directly and may be more verbose then needed. I was unsure if all properties need asetandget; so I only createdgetfunctions for properties indicated as "read-only".Edit - Changed number of classes to reflect PR updates