tiled.dart icon indicating copy to clipboard operation
tiled.dart copied to clipboard

feat: TsxProviderBase with test function

Open benni-tec opened this issue 1 year ago • 7 comments

Description

Currently a TsxProvider needs a getter for the filename it can provider. This PR seeks to introduce a new base class TsxProviderBase which instead uses a bool checkProvidable(String filename) function to determine if a TsxProviderBase is able to provide the source.

To accomplish this it also has Parser getSourceBase(String filename) and Parser? getCachedSourceBase(String filename) methods to retrieve the source when needed.

While do not like attaching Base to everything this naming scheme seems to be the easiest option to not break current implementations.

This new functionality is usefull for example to resolve all .tsx-files relative to a single directory.

Checklist

  • [X] The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • [X] I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • [ ] I have updated/added tests for ALL new/updated/fixed functionality.
  • [X] I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • [ ] I have updated/added relevant examples in examples.

Breaking Change

  • [X] Yes, this is a breaking change.
  • [X] No, this is not a breaking change.

While the internal functionig does change and new APIs are exposed I have recreated the old behaviour in TsxProvider as a sub class of the new TsxProviderBase. Therefore classes extending TsxProvider should work as before with no breaking changes.

However classes only implementing it will require additional methods, however if only implemented they should directly implement TsxProviderBase since (apart from the implementation of the old behaviour) TsxProvider does not offer more functionality. It is simply a Wrapper to emulate the old behaviour and not break convention.

Migration instructions

All classes only implementing TsxProvider should either

  • extend TsxProvider if the old API is needed (this allows you to not have to look at any new functionality) or
  • directly implement TsxProviderBase.

Related Issues

Completes #70 Allows for usecase described in #69

benni-tec avatar Oct 30 '23 08:10 benni-tec

I've got some questions about the checklist:

  • The old functionality was not covered by tests as far as I can see, nevertheless I will write some soon.
  • There is no example folder in this repo. Where should examples go?

benni-tec avatar Oct 30 '23 08:10 benni-tec

I've got some questions about the checklist:

  • The old functionality was not covered by tests as far as I can see, nevertheless I will write some soon.

Super!

  • There is no example folder in this repo. Where should examples go?

The checklist is copied from Flame, you can create an example directory if we don't have one (shame on us 😅).

spydon avatar Oct 30 '23 12:10 spydon

While do not like attaching Base to everything this naming scheme seems to be the easiest option to not break current implementations.

I would consider just breaking current implementations, as long as you are willing to write a PR for flame_tiled, which is the only major client of this library.

Overall it's hard to me to tell if this PR makes sense without concrete examples, maybe tests would help with that

kurtome avatar Oct 30 '23 23:10 kurtome

@benni-tec any updates on this? :)

spydon avatar Dec 05 '23 14:12 spydon

Not yet. My fork currently does everything I need for my current project. I do plan to submit proper PRs for this and the other issues I've opened, however I am currently pretty time constrained.

As of now my current project should be wrapped up in January, I am currently planning to clean up and submit PRs then.

Is this something that is time-sensitive for you? If so I will see if can get the PRs done this year!

benni-tec avatar Dec 07 '23 12:12 benni-tec

LFTM (looks fine to me!)

jtmcdole avatar Jan 23 '24 15:01 jtmcdole

Thanks for the review, I didn't have to time yet to write a proper comment:

I have generalised into a generic Provider that can also be used for templates, therby also resolving #73, also as being extensible to more easily discover the location of images in flame_tiled.

I also moved the methods from TileMapParser into TiledMap since there were already parsing method there, and I always thought it was confusing to have this in a different class that does nothing otherwise. While moving it over I also made the names nicer and more descriptive.

The fromString method seems to only have been used in the tests to generate TsxProvider's for every included tileset, which always seemed like a hack to me! Anyways this is not neccessary anymore since the new Provider can resolve multiple or even all files (see tests for an example). The rest of the functionality can be found in parseTmx()!

I took the liberty of adding the Lists of Provider's to the parser to make it more convenient and clean (instead of passing it through all parse calls, only the initial Parser creation is affected and the copies)!

However I split them into multiple Lists so one for tmx and one for templates. This however does not allow to integrate the same mechanism for images (e.g. of tilesets), because I don't think the actual loading of the images is supposed to be in this library and nothing is ever done with them I don't think that is a big deal. flame_tiled could still use the same Provider mechanism to discorver images (this would be great improvment compared to the current state), but just create it's own List when parsing the RenderableMap!

However I could also only pass one List and the Provider's could instead report their capability. What do you think is preferable?

benni-tec avatar Jan 23 '24 15:01 benni-tec