Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse xml synchronously #65

Open
Maatteogekko opened this issue Feb 3, 2023 · 11 comments
Open

Parse xml synchronously #65

Maatteogekko opened this issue Feb 3, 2023 · 11 comments
Assignees

Comments

@Maatteogekko
Copy link

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
Copy link
Owner

andyduke commented Feb 3, 2023

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

@Maatteogekko
Copy link
Author

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
Copy link
Author

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":

Screen.Recording.2023-02-19.at.11.04.54.mp4

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

async_6000.mov

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

Sync version

sync_6000.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

Screen.Recording.2023-02-19.at.11.42.22.mov

Still 60 fps and still jumps.

Sync version

Screen.Recording.2023-02-19.at.11.44.23.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
Copy link

Pomis commented Mar 7, 2023

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

@andyduke
Copy link
Owner

andyduke commented May 5, 2023

@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
Copy link
Author

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

@LastMonopoly
Copy link

Any update? This is actually quite important.

@Maatteogekko
Copy link
Author

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 andyduke self-assigned this Oct 24, 2023
andyduke added a commit that referenced this issue May 28, 2024
@andyduke
Copy link
Owner

@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
Copy link
Author

Nice!
Just a question... shouldn't


be onParsed(node, false)?

@andyduke
Copy link
Owner

Nice! Just a question... shouldn't

be onParsed(node, false)?

Yes, you are absolutly right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants