Fix duplicate code in PlatformTarget subclasses, fix assets being included twice.#1980
Merged
player-03 merged 7 commits intoopenfl:8.4.0-devfrom Oct 23, 2025
Merged
Conversation
Having eight separate implementations made it harder to maintain. While an asset embedding bug got fixed on desktop targets, it got overlooked on others. This lead to assets being included twice on those other targets: once embedded, and once normally. (openfl#1898) Now, that behavior is controlled from one place, and the bug is fixed for eight targets at once. This also standardizes the case of `asset.embed == true && asset.type == TEMPLATE`. Previously, some targets would prioritze `embed` over `type`, embedding the template into the app without even processing it as a template. Now, it will always prioritize `type`, processing templates as templates and never trying to embed them. Three targets are left out since they have more complex asset embedding behavior. I haven't checked if the bug is present on any of those.
The old code appeared to be trying to do this, but it didn't work. When it found appinfo.json, it would call `copyAsset()` with an extra `context` argument, which is what you do for templates. However, `copyAsset()` only processes templates when `type == TEMPLATE`, otherwise it ignores the extra argument and processes it as a normal asset.
Contributor
Author
|
@MAJigsaw77 Check this out and let me know if it works. |
The implementation was copy-pasted, so it makes more sense to have only a single copy. `HTML5Platform` added a comment about possible future changes, so I kept that. Future changes will still be possible by overriding the function.
player-03
changed the title
PlatformTarget subclasses, fix assets being included twice.
All of the implementations were identical, except for `FlashPlatform`, which was identical with extra steps.
Contributor
Author
|
Cleaned up some more duplicate code while I was at it. I think that's it, though; the rest of the functions look unique. |
player-03
commented
Sep 1, 2025
player-03
commented
Sep 1, 2025
|
|
||
| @ignore public override function uninstall():Void {} | ||
|
|
||
| @ignore public override function watch():Void {} |
Contributor
Author
There was a problem hiding this comment.
watch() relies on getDisplayHXML(), which is missing from AIRPlatform and TizenPlatform, so that explains why we don't have it there.
However, WebAssemblyPlatform has getDisplayHXML() and doesn't have watch(). I don't know why, but I kept that behavior anyway. It seems like it ought to work, if we just enable it.
Contributor
Can confirm that it works on |
Two reasons: 1. This is how it always worked in practice. The old special case actually did nothing due to an oversight. 2. Per the principle of least astonishment, Lime should avoid messing with the user's assets. If they wanted it to be a template, they would have specified `type="template"`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Having eight separate implementations made it harder to maintain.
When an asset embedding bug got fixed on desktop targets, it got overlooked on others. This lead to assets being included twice on those other targets: once embedded, and once normally. (#1898) Now, that behavior is controlled from one place, and the bug is fixed for eight targets at once.
This also standardizes the case of
asset.embed == true && asset.type == TEMPLATE. Previously, some targets would prioritzeembedovertype, embedding the template into the app without even processing it as a template. Now, it will always prioritizetype, processing templates as templates and never trying to embed them.I left out
AIRPlatform,FlashPlatform, andHTML5Platformsince they have more complex asset embedding behavior. I haven't checked if the bug is present on any of those.