Visually display scroll velocities, and measure more stably #15
Conversation
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Sharing on all 3 for consistency: I was so very excited to see #13 that I forgot myself and made an error, actually 2. I should have sought a second reviewer on the PR as is our review guidelines. And I should have ensured tests were included, or that test exemption had been granted. 😅
Can you add a test for this PR? It ensures we do not end up removing your change accidentally. :)
Does the test requirement apply to the platform_tests repo? From the linked part of the wiki, it sounds like it may not:
And there doesn't appear to be any existing tests for this app, or for So it seems like the practice within this repo may be to mostly not have tests. Which I think is pretty reasonable, for the purpose these apps have, though a smoke test for each app would probably be helpful. Oh, and I just spotted that that wiki section also says this:
So given that the bot hasn't commented here, I think that testing requirement really is meant to not apply to this repo. Which I think means that it's just the PR template that is a bit misleading due to having been copied from the main repo. (The template definitely hasn't been carefully adjusted to fit this repo, because some of the other items don't really fit either: e.g. Tree Hygiene, as I think this repo isn't part of the Flutter build.) |
|
Yeah I too noticed the inconsistencies, I am following up with the infra team to see if our bot just overlooks this repo intentionally or not. The bot ensures multiple compliances are met before code is merged, so it could be that it really should be enabled here. It is always good to have a test that prevents your change from being reverted, perhaps a smoke test will suffice here to make sure it still runs. I have in the past returned to this tool and found it broken. I'll follow up with what I find out about the template/bot mix up here. https://discord.com/channels/608014603317936148/608021351567065092/1075106318538518558 |
|
Fair enough. I'll add a quick test. |
|
Added a test for the new VelocityOverlay widget; PTAL. Also fixed up some things that were needed in order for it to work in tests. One was pretty puzzling: Another was telling me that the |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Thanks for yak shaving! This also LGTM, but there are merge conflicts from the other PR landing
|
Great, thanks! Will rebase and resolve those conflicts, here and on #16. |
This makes it a bit easier to see what's happening when the numbers are rapidly changing during scrolling.
At 30 per second, the rate evenly divides common display frame rates of 60, 90, or 120 fps. This produces much smoother measurements because most measurements now reflect the same number of frames.
The timer doesn't guarantee firing at exactly the requested times, and in practice it doesn't. Measuring the velocity by taking the real difference in offset compared to the nominal difference in time adds quite a lot of noise to the result, producing a large flicker in the visual velocity indicators. Instead, measure the actual time.
Even when measuring based on the real time, if we take measurements on a periodic timer then the results show an occasional flicker because the new offset is recomputed once per frame, and so occasionally an interval sees one more or fewer updates than usual because it just barely covers a new frame at both ends or just barely misses one at each end. Instead, check in just after each frame. This pretty much eliminates flicker in the display of the Flutter velocity.
This avoids an error when running in the test framework, where the row with "Difference" was overflowing.
gnprice
force-pushed
the
pr-measurement
branch
from
4a43502 to
7d32cac
Compare
|
Done. |
2 participants
This is the further scroll_overlay enhancement I mentioned at #13 (comment) .
It produces a display that looks like this:
or this screen recording, from flutter/flutter#120420 (comment) :
before-norestarts.mp4
This PR also includes several changes to make the measurements more stable. The numbers had been jumping around, which becomes a lot more visible with the visual display.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.