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

Parse xml synchronously #65

Open Maatteogekko opened 1 year ago

Maatteogekko commented 1 year ago

The only issue I have with this package is that it parses xml asynchronously. This makes widgets jump on first frame if they size themself according to the text (the simplest case is a Container wrapping some text).

This makes for a really poor user experience.

So I forked the xmlstream package and added a synchronous read method. Then used it in a fork of styled_text (here). I put it together quickly just for my use, but if anybody is interested I could polish it and make a PR

andyduke commented 1 year ago

@Maatteogekko Do you have any benchmarks on how much synchronous parsing slows down the rendering and responsiveness of the application?

Maatteogekko commented 1 year ago

I plan on testing it during this weekend, but just from a quick look at the performance tab of the dev tools it doesn't look bad (for a couple hundred characters at least)

Maatteogekko commented 1 year ago

Ook here we go (better late than never 😅)

For starters, I added a property to StyledText to choose between sync or async parsing (and I believe this could be a good solution). To make things more clear, here is what I mean by "poor user experience":

https://user-images.githubusercontent.com/54111322/219941564-987df2dd-2588-40d6-b3c8-0ef98744a077.mov

When I click on the top right arrow I call setState on the whole page. The asynchronous text is empty for the first frame and when the parsing is complete it suddenly jumps to the correct size; the synchronous version instead does all in one frame, so no jumps. This also happens on first build but there is too much going on and is difficult to see.

Now some benchmarks (ran on a physical device in profile mode). I first tried with a very long text, and then with a more reasonable one (for mobile apps at least).

~6000 characters

Async version

https://user-images.githubusercontent.com/54111322/219942507-639e4d2b-963b-4732-8202-e9f8c833307c.mov

We get 60 fps but there is the ugly jump both when first loading the page and when refreshing.

Sync version

https://user-images.githubusercontent.com/54111322/219942719-3ef58abf-2f48-4776-87f2-0fe5615d9379.mov

We still get 60 fps and there is no jump. But the frame where we parse the text janks pretty badly (30 to 50 ms).

~600 characters

Async version

https://user-images.githubusercontent.com/54111322/219943086-18a76810-85af-420e-b68b-157e8b2f8f4b.mov

Still 60 fps and still jumps.

Sync version

https://user-images.githubusercontent.com/54111322/219943199-49e7d7f4-a8a7-4195-a4af-169a5fc2e850.mov

Still 60 fps and no jumps, but the frame where we parse now stays under 16ms


There is a clear performance penalty when parsing the text synchronously, but I think in some cases (let's say with less than 500 characters of text) that is preferable to having the UI jump around. And here comes my solution to add a property to choose between sync and async parsing (with default to async): when you have a layout that suffers from the text resizing you can set it to sync and be happy.

I am pretty sure that the synchronous parsing I quickly hacked together could be improved performance wise.

What do you think?

Pomis commented 1 year ago

Also need this. Currently I have to wrap StyledTexts into SizedBoxes to avoid layout jumping

andyduke commented 1 year ago

@Maatteogekko I think you have make a great job of researching this issue. I agree with you that we need a parameter to enable synchronous parsing, but this will need to be documented and clearly explained when to use synchronous parsing and when not.

Maatteogekko commented 1 year ago

Thank you @andyduke. How do we proceed from here? I can open a PR and then we can discuss things there?

LastMonopoly commented 9 months ago

Any update? This is actually quite important.

Maatteogekko commented 9 months ago

I have been using my fork for a while and have had no problems with going full sync (the latest commit on my fork removed the async parser for a faster sync parser).

For implementing it in the package I am waiting for @andyduke directions

andyduke commented 1 month ago

@Maatteogekko @Pomis @LastMonopoly Text parsing is now synchronous in StyledText starting from version 9.0.0. While this version is in beta status, you can already try to see if this solves your difficulties.

PS @Maatteogekko - thank you for your contribution.

Maatteogekko commented 1 month ago

Nice! Just a question... shouldn't https://github.com/andyduke/styled_text_package/blob/cc4498c74cd533366e72b5aeeb74f924ee9d6945/styled_text/lib/src/parsers/text_parser_sync.dart#L58 be onParsed(node, false)?

andyduke commented 1 month ago

Nice! Just a question... shouldn't

https://github.com/andyduke/styled_text_package/blob/cc4498c74cd533366e72b5aeeb74f924ee9d6945/styled_text/lib/src/parsers/text_parser_sync.dart#L58

be onParsed(node, false)?

Yes, you are absolutly right.