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

Add support for HtmlWidget.renderMode #484

Merged
merged 30 commits into from
Jun 9, 2021
Merged

Add support for HtmlWidget.renderMode #484

merged 30 commits into from
Jun 9, 2021

Conversation

daohoangson
Copy link
Owner

@daohoangson daohoangson commented Apr 12, 2021

WidgetFactory breaking changes:

  • buildColumnPlaceholder removed trimMarginVertical named param
  • buildColumnWidget removed tsh param
  • onTapAnchor replaced anchorContext param with scrollTo

@daohoangson daohoangson temporarily deployed to vercel April 12, 2021 10:07 Inactive
@github-actions
Copy link

github-actions bot commented Apr 12, 2021

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #484 (12721f1) into next (b68fef3) will increase coverage by 0.08%.
The diff coverage is 99.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #484      +/-   ##
==========================================
+ Coverage   97.94%   98.02%   +0.08%     
==========================================
  Files          47       48       +1     
  Lines        3019     3145     +126     
==========================================
+ Hits         2957     3083     +126     
  Misses         62       62              
Impacted Files Coverage Δ
packages/core/lib/src/internal/core_ops.dart 100.00% <ø> (ø)
packages/enhanced/lib/src/html_widget.dart 100.00% <ø> (ø)
packages/core/lib/src/internal/ops/anchor.dart 98.66% <98.66%> (ø)
packages/core/lib/src/core_helpers.dart 100.00% <100.00%> (ø)
packages/core/lib/src/core_html_widget.dart 100.00% <100.00%> (ø)
packages/core/lib/src/core_widget_factory.dart 99.58% <100.00%> (+0.01%) ⬆️
packages/core/lib/src/data/build_bits.dart 100.00% <100.00%> (ø)
packages/core/lib/src/internal/builder.dart 100.00% <100.00%> (ø)
packages/core/lib/src/internal/ops/column.dart 100.00% <100.00%> (+3.57%) ⬆️
packages/core/lib/src/internal/ops/tag_ruby.dart 95.74% <100.00%> (+0.18%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b68fef3...12721f1. Read the comment docs.

@daohoangson daohoangson temporarily deployed to vercel April 13, 2021 15:39 Inactive
@daohoangson daohoangson temporarily deployed to vercel April 15, 2021 10:37 Inactive
@daohoangson daohoangson temporarily deployed to vercel April 15, 2021 10:56 Inactive
@daohoangson daohoangson temporarily deployed to vercel April 15, 2021 16:56 Inactive
@daohoangson daohoangson temporarily deployed to vercel April 15, 2021 17:22 Inactive
@daohoangson daohoangson temporarily deployed to vercel April 16, 2021 02:33 Inactive
@daohoangson
Copy link
Owner Author

daohoangson commented Apr 16, 2021

Check out this screen recording (demo_app, Android, release mode): https://youtu.be/5U10W0VJHOA

  • Ignoring the parsing phases: shown in video with the circle indicator
  • Column takes 1s (skipped ~60 frames) to render / layout.
  • ListView takes 30ms (skipped 2 frames) to do that
  • SliverList takes 17ms (skipped a frame)
  • After the first frames, scrolling is stable at 60fps in all modes (thanks to the RepaintBoundary)

@daohoangson daohoangson temporarily deployed to vercel April 16, 2021 04:15 Inactive
@DFelten
Copy link
Contributor

DFelten commented Apr 16, 2021

Looks really good :) The demo app with the huge html feels great. Thanks for this feature!

@daohoangson daohoangson temporarily deployed to vercel April 16, 2021 18:19 Inactive
@daohoangson daohoangson temporarily deployed to vercel June 1, 2021 16:07 Inactive
@daohoangson daohoangson temporarily deployed to vercel June 1, 2021 16:36 Inactive
@daohoangson
Copy link
Owner Author

@DFelten yeah, the huge html is quite slow. There's probably some optimizations to make but it will still take some time in the end.

@DFelten
Copy link
Contributor

DFelten commented Jun 4, 2021

@daohoangson I think the duration of 50 milliseconds looks better than 100. At least it feels better in my tests.

One idea here would be to make the value overwritable from the outside. This way, everyone can adjust the value according to their own taste.

@daohoangson
Copy link
Owner Author

In my testings, lowering the duration prevented it from working... That's why I kept it at 100ms. I'll do some more tests. Exposing the duration is possible but not ideal though, I want to avoid having a constructor with too many params.

@DFelten
Copy link
Contributor

DFelten commented Jun 4, 2021

For me it also works at 30, but if I set the value too low, I also get an error. For example, a value of 10 will stop too early. The list of new indexes is built too late or the lists are still identical. But maybe it depends on how strong the device is and how slow it is when building the widgets.

Couldn't the value rather be overwritten via the core Widget Factory? Then we don't need a larger constructor here either.

@DFelten
Copy link
Contributor

DFelten commented Jun 4, 2021

I have just tested a little more. You are right, with a value of 50 it does not work with all articles. But even with 100 there are problems if the article is too long or there are too many images.

But fortunately this happens only on the simulator. On the phone it's working with a duration of 50 and 100.

@daohoangson daohoangson temporarily deployed to vercel June 8, 2021 13:43 Inactive
@daohoangson
Copy link
Owner Author

The latest commit changed the way scrolling is done so it's much faster and more reliable. Please try it if you have a chance @DFelten. I think this PR is almost ready now, it maybe merged into next pretty soon.

@daohoangson daohoangson temporarily deployed to vercel June 8, 2021 15:17 Inactive
@daohoangson daohoangson temporarily deployed to vercel June 8, 2021 15:33 Inactive
@DFelten
Copy link
Contributor

DFelten commented Jun 8, 2021

The speed is so much better :) Thanks for the latest update, so this can definitely be merged.

@daohoangson daohoangson temporarily deployed to vercel June 8, 2021 17:41 Inactive
@daohoangson daohoangson temporarily deployed to vercel June 8, 2021 18:03 Inactive
@daohoangson daohoangson temporarily deployed to vercel June 9, 2021 06:49 Inactive
@daohoangson daohoangson merged commit b549768 into next Jun 9, 2021
@daohoangson daohoangson deleted the feat/render_mode branch June 9, 2021 10:23
@daohoangson
Copy link
Owner Author

v0.7.0-dev.2021061301 has been released with this feature. Please try upgrading and let me know if it works for you @DFelten

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

Successfully merging this pull request may close these issues.

None yet

2 participants