gdx-lml icon indicating copy to clipboard operation
gdx-lml copied to clipboard

[LML] Resolve LmlActor annotation value from field name

Open metaphore opened this issue 7 years ago • 14 comments

I feel like I might be missing something, but still I don't see any reason against this approach.

If there is no value specified for LmlActor annotation, it could be resolved implicitly from related field name, e.g. @LmlActor Button button; = @LmlActor("button") Button button;.

This might be not universal since, value is array and annotation may be assigned to collection field type. But I see singular widget declaration as a general case and duplicating field name most of the time makes code cumbersome.

metaphore avatar Dec 15 '17 04:12 metaphore

There might have been some issues with this approach (valid LML IDs do not have to be valid Java identifiers and actors collections you mentioned), but I don't see why it shouldn't be added. Not sure if LibGDX reflection API allows to do that at this point, but I'll look into it when I finally force myself to update LML. ;) I'll try this weekend.

czyzby avatar Dec 15 '17 10:12 czyzby

I believe so, since you already use com.badlogic.gdx.utils.reflect.Field in AbstractLmlParser, it should be simple like Field#getName().

Thanks a lot, really looking forward for this little but very useful improvement.

metaphore avatar Dec 15 '17 10:12 metaphore

Yeah, you need a reference to the Field to perform the actual injection, I don't expect this to be hard to implement.

czyzby avatar Dec 15 '17 10:12 czyzby

BTW, I just thought the same technique could be applied to LMLAction annotation. ActionContainerWrapper#mapClassMethods(Class<?>) operates reflected com.badlogic.gdx.utils.reflect.Method and thus could easily extract method name.

metaphore avatar Dec 16 '17 01:12 metaphore

I think I originally wanted to avoid that partially because applications can be obfuscated with Proguard before the release, changing field and method names. This will either break your game or force you to exclude extra classes from the Proguard settings. Even though it might seem redundant, it's actually a sensible practice to keep meta-data in the annotations.

That said, it will certainly reduce boilerplate required to define LML view handlers, so I don't see why we shouldn't include that.

czyzby avatar Dec 16 '17 09:12 czyzby

I've never played much with code obfuscation and can only guess, but should not those runtime methods like Field#getName() return actual post obfuscated names?

metaphore avatar Dec 16 '17 09:12 metaphore

Unless you specifically tell the obfuscation lib not to rename fields/methods, getName will still work, but will return meaningless obfuscated names like aaA, which obviously won't match your LML IDs. Obfuscation will not modify IDs passed to annotations, though.

czyzby avatar Dec 16 '17 11:12 czyzby

Ah, text references to fields and methods from LML templates, right. I think it's a common practice to note if library requires any extra Proguard rules, at least, as I could remember, a lot of top used Android libs describe it under corresponding readme/wiki section or supply within an artifact.

metaphore avatar Dec 16 '17 11:12 metaphore

1.9.1.9.8-SNAPSHOT is currently being uploaded. I think I've fixed all crucial issues that were open. @metaphore Can you use the latest version and test if the solution works for you? I'm planning on uploading the 1.9.1.9.8 release next week. Thanks.

czyzby avatar Dec 17 '17 19:12 czyzby

Thanks, will do! But what about VisUI, as of I know it is still not updated to LibGDX 1.9.8 and should do that sometime soon. Maybe it would be better to wait for it and release only then?

metaphore avatar Dec 18 '17 00:12 metaphore

Isn't VisUI development halted? There was no release for 1.9.7. For what it's worth, VisUI 1.4.0 should work with latest LibGDX release.

czyzby avatar Dec 18 '17 10:12 czyzby

I can't say for sure if current VisUI is compatible with latest LibGDX, but @kotcrab is working on the new version https://github.com/kotcrab/vis-editor/issues/275

metaphore avatar Dec 18 '17 11:12 metaphore

@czyzby I've had some time playing with 1.9.1.9.8-SNAPSHOT and it seems pretty stable. I thought it's ready to go, but at the last moment I decided to make PR and drop some of my local LML changes, so maybe you would find them useful and include in this release #67

metaphore avatar Dec 26 '17 17:12 metaphore

Thanks. I'll wait for the release until the latest VisUI version.

czyzby avatar Dec 26 '17 17:12 czyzby