andyduke / styled_text_package

Text widget with formatted text using tags. Makes it easier to use formatted text in multilingual applications.
https://pub.dev/packages/styled_text
BSD 3-Clause "New" or "Revised" License
74 stars 48 forks source link

adds StyledText.builder constructor #70

Closed maeddin closed 12 months ago

maeddin commented 1 year ago

Here a new constructor is introduced that allows you to use the TextSpan generated by StyledText directly. This way you can use this package in combination with other packages (e.g. AutoSizeText) and widgets (e.g. TextFormField by overriding buildTextSpan).

This is not a breaking change.

Closes #66.

andyduke commented 1 year ago

Could you explain why you might need a builder for Spans built inside a StyledText?

maeddin commented 1 year ago

I wanted to use StyledText together with AutoSizeText, which does not work without this new constructor. Now i can use it this way:

StyledText.builder(
  text: text,
  builder: (context, textSpan) => AutoSizeText.rich(textSpan),
)

I am of course open to alternative suggestions on how to solve this problem.

andyduke commented 1 year ago

I understand your problem, but if you just replace the creation of the RichText inside the StyledText with an external call, it will be confusing, because many parameters passed to StyledText will no longer work (eg textAlign, textDirection, etc.). Therefore, we need to come up with another way to implement this functionality. Perhaps you need to split the StyledText widget into two parts, a base class that parses xml and builds a span, and another that assembles from a span and RichText parameters, then it can be easily customized.

maeddin commented 1 year ago

That's actually the way I wanted to implement it first: Rebuilding the widget so that all constructors internally call the builder constructor. However, that would be a larger pull request and large pull requests are less likely to be approved. Alternatively, you could of course introduce a second widget CustomStyledText. So I can rebuild it by using internally StyledText.builder for all other constructors or by introducing an additional widget, depending on how you like. However, it is currently not confusing to use the new constructor, since you can only set the five relevant parameters in it. The current public API of this pull request would not change that much by additional internal changes. In my own packages I'm also using separate widgets and constructors for the custom versions of the widget (e.g. animated_toggle_widget)

andyduke commented 1 year ago

At the moment, 12 parameters are passed to RichText, except for span, and there is also a Selectable version. Your suggestion breaks the work of these parameters, so I think that this can only be done by significantly reconsidering the approach and using composition, when parsing xml and creating spans will be separate, and building the widget will be separate. Then it will be possible to make various options for building widgets, including a widget with AutoSizeText.

andyduke commented 1 year ago

Another approach is to move all the work on xml and building span's into the CustomStyledText widget, which will not build the widget itself, but will call the builder. And already use it in StyledText and build RichText or SelectableText.rich with the necessary parameters.

maeddin commented 1 year ago

At the moment, 12 parameters are passed to RichText, except for span, and there is also a Selectable version. Your suggestion breaks the work of these parameters

In fact, it would not have broken the current parameters, but it would be a breaking change as the constructors would no longer be const with this approach.

Another approach is to move all the work on xml and building span's into the CustomStyledText widget

I think we mean the same thing. So should I outsource the functionality to a separate widget? Then I'll just update this PR to that end.

andyduke commented 1 year ago

In fact, it would not have broken the current parameters, but it would be a breaking change as the constructors would no longer be const with this approach.

This will break the functionality of the StyledText widget, when the parameters passed to it will not affect the Richtext widget.

I think we mean the same thing. So should I outsource the functionality to a separate widget? Then I'll just update this PR to that end.

Yes, you can try to separate the widget into two separate ones and see how it turns out.

maeddin commented 1 year ago

This will break the functionality of the StyledText widget, when the parameters passed to it will not affect the Richtext widget.

I think we are talking about different things. With my version, everything would still work and all parameters would do what they are supposed to. I have already done this in other packages.

Yes, you can try to separate the widget into two separate ones and see how it turns out.

I'll try it.

maeddin commented 1 year ago

@andyduke I have now introduced the widget CustomStyledText. You can take a look at it.

maeddin commented 1 year ago

@andyduke What do you think about my changes? :)

andyduke commented 12 months ago

@maeddin Sorry for the delay, your PR is now online in version 8.1.0.