feat: TsxProviderBase with test function #72

benni-tec · 2023-10-30T08:05:36Z

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

The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
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.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples.

Breaking Change

Yes, this is a breaking change.
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

…ontracts // corrected and extended README.md

benni-tec · 2023-10-30T08:07:52Z

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?

spydon · 2023-10-30T12:58:56Z

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 😅).

kurtome · 2023-10-30T23:06:46Z

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

ufrshubham

I can't seem to understand why TsxProviderBase had to be a separate class. If it is just to avoid breaking filename getter, then I think making that breaking change should be fine.

ufrshubham · 2023-10-31T18:44:18Z

README.md

@@ -29,9 +29,15 @@ Load a TMX file into a string by any means, and then pass the string to TileMapP
    final TiledMap mapTmx = TileMapParser.parseTmx(tmxBody);
 ```

-If your tmx file includes a external tsx reference, you have to add a CustomParser
+If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by using extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.


Suggested change

If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by using extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.

If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.

Whoops missed that on 😅

ufrshubham · 2023-10-31T18:57:26Z

packages/tiled/lib/src/tsx_provider.dart

+abstract class TsxProviderBase {
+  /// Given a filename this function should check whether this Provider
+  /// can provide this source.
+  bool checkProvidable(String filename);


The word "check" in checkProvidable seems to indicate that this method is going to perform some validation on the given filename. How does canProvide sound?

Yes that does sound better 👍

spydon · 2023-12-05T14:27:55Z

@benni-tec any updates on this? :)

benni-tec · 2023-12-07T12:43:29Z

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!

…justed tests

spydon

Just a few comments, over all it looks good :)

spydon · 2024-01-23T11:58:04Z

README.md

@@ -29,9 +29,15 @@ Load a TMX file into a string by any means, and then pass the string to TileMapP
    final TiledMap mapTmx = TileMapParser.parseTmx(tmxBody);
 ```

-If your tmx file includes a external tsx reference, you have to add a CustomParser
+If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by using extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.


Seems like we don't have the markdown linter activated here, the max line length on markdown should be 100.

spydon · 2024-01-23T11:58:24Z

README.md

 ```dart
    final String tmxBody = /* ... */;
-    final TiledMap mapTmx = TileMapParser.parseTmx(tmxBody, tsx: CustomTsxProvider());
+    final TiledMap mapTmx = TileMapParser.parseTmx(tmxBody, tsxProviders: [SingleTsxProvider(), MultipleTsxProvider()]);


Split up line with trailing comma

spydon · 2024-01-23T11:59:28Z

packages/tiled/lib/src/parser.dart

+
+  XmlParser(this.element, {super.tsxProviders, super.templateProviders});
+
+  factory XmlParser.fromString(


This can be a named constructor instead of a factory

spydon · 2024-01-23T11:59:56Z

packages/tiled/lib/src/parser.dart

+
+  JsonParser(this.json, {super.tsxProviders, super.templateProviders});
+
+  factory JsonParser.fromString(


Same here, this can be a named constructor

spydon · 2024-01-23T12:00:16Z

packages/tiled/lib/src/template.dart

@@ -34,3 +34,7 @@ class Template {
          object: parser.getSingleChildOrNullAs('object', TiledObject.parse),
        );
 }
+
+class TemplateReference {


Should this be removed?

spydon · 2024-01-23T12:00:31Z

packages/tiled/lib/src/tiled_map.dart

+  // TODO: why is this here? same as parseTmx???
+  // static Future<TiledMap> fromString(
+  //   String contents, {
+  //   List<ParserProvider>? tsxProviders,
+  //   List<ParserProvider>? templateProviders,
+  //   List<ImagePathProvider>? imageProviders,
+  // }) async {
+  //   final tsxSourcePaths = XmlDocument.parse(contents)
+  //       .rootElement
+  //       .children
+  //       .whereType<XmlElement>()
+  //       .where((element) => element.name.local == 'tileset')
+  //       .map((e) => e.getAttribute('source'));
+  //
+  //   final tsxProviders = await Future.wait(
+  //     tsxSourcePaths
+  //         .where((key) => key != null)
+  //         .map((key) async => tsxProviderFunction(key!)),
+  //   );
+  //
+  //   return TileMapParser.parseTmx(
+  //     contents,
+  //     tsxList: tsxProviders.isEmpty ? null : tsxProviders,
+  //   );
+  // }


spydon · 2024-01-23T12:01:38Z

packages/tiled/lib/src/tiled_map.dart

@@ -286,7 +289,41 @@ class TiledMap {
    );
  }

-  factory TiledMap.parse(Parser parser, {List<TsxProvider>? tsxList}) {
+  static TiledMap parseJson(


This should be a factory just like parseTmx, also needs some dartdocs

jtmcdole · 2024-01-23T15:06:12Z

LFTM (looks fine to me!)

benni-tec · 2024-01-23T15:40:35Z

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?

added TsxProviderBase and updatet TsxProvider to not break existing c…

f8658e1

…ontracts // corrected and extended README.md

Clarification of TsxProvider behaviour in README.md

51ba2a7

spydon requested review from kurtome, jtmcdole and ufrshubham October 30, 2023 12:59

ufrshubham approved these changes Oct 31, 2023

View reviewed changes

benni-tec added 2 commits January 16, 2024 14:11

generalized provider (ready for templates)

6bf2527

added providers to Parser // implemented provider for templates // ad…

838bb52

…justed tests

spydon reviewed Jan 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: TsxProviderBase with test function #72

feat: TsxProviderBase with test function #72

benni-tec commented Oct 30, 2023 •

edited by spydon

Loading

benni-tec commented Oct 30, 2023

spydon commented Oct 30, 2023

kurtome commented Oct 30, 2023

ufrshubham left a comment

ufrshubham Oct 31, 2023

benni-tec Nov 2, 2023

ufrshubham Oct 31, 2023

benni-tec Nov 2, 2023

spydon commented Dec 5, 2023

benni-tec commented Dec 7, 2023

spydon left a comment

spydon Jan 23, 2024

spydon Jan 23, 2024

spydon Jan 23, 2024

spydon Jan 23, 2024

spydon Jan 23, 2024

spydon Jan 23, 2024

spydon Jan 23, 2024

jtmcdole commented Jan 23, 2024

benni-tec commented Jan 23, 2024

	If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by using extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.
	If your tmx file includes a external tsx reference, you have to add a CustomParser. This can either be done by extending the TsxProviderBase, which can match multiple files, or by extending TsxProvider, which only matches on file by its name.


		XmlParser(this.element, {super.tsxProviders, super.templateProviders});

		factory XmlParser.fromString(


		JsonParser(this.json, {super.tsxProviders, super.templateProviders});

		factory JsonParser.fromString(

feat: TsxProviderBase with test function #72

Are you sure you want to change the base?

feat: TsxProviderBase with test function #72

Conversation

benni-tec commented Oct 30, 2023 • edited by spydon Loading

Description

Checklist

Breaking Change

Migration instructions

Related Issues

benni-tec commented Oct 30, 2023

spydon commented Oct 30, 2023

kurtome commented Oct 30, 2023

ufrshubham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spydon commented Dec 5, 2023

benni-tec commented Dec 7, 2023

spydon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtmcdole commented Jan 23, 2024

benni-tec commented Jan 23, 2024

benni-tec commented Oct 30, 2023 •

edited by spydon

Loading