Windows scrolling integration #32094
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Could somebody please advise what is the standard method of mocking system calls like SystemParametersInfo out when writing tests? |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. |
| lines_per_scroll = 3; | ||
| } | ||
|
|
||
| // This logic is based off Chromiums implementation |
There was a problem hiding this comment.
| // This logic is based off Chromiums implementation | |
| // This logic is based off Chromium's implementation |
I don't believe you can mock out global functions like that. I don't see an easy way to test that part, perhaps only a check that |
|
@moffatman I've added the test as you described, but I couldn't think of a way to avoid making I'm not sure what is usually done here as in the current setup the compiler won't be able to inline |
| MOCK_METHOD2(OnPointerLeave, void(FlutterPointerDeviceKind, int32_t)); | ||
| MOCK_METHOD0(OnSetCursor, void()); | ||
| MOCK_METHOD4(OnScroll, | ||
| void(double, double, FlutterPointerDeviceKind, int32_t)); |
There was a problem hiding this comment.
Why was OnScroll removed?
There was a problem hiding this comment.
This is because that function can't be mocked as the non-mocked version needs to be called as part of the test.
|
@Jon-Salmon I guess it needs to be virtual to be mocked and tested. Personally I think the performance impact is not so meaningful here. |
|
@Jon-Salmon is this still being actively worked on? Agreed with @moffatman that the performance impact of the virtual should be negligible here; go for it :) |
|
@cbracken I'm happy to continue to work on this if more changes are needed, but I am of the opinion that this is complete and ready for review. As mentioned in the first comment, the windows setting is only checked during startup. I felt this wasn't a major issue but would be happy to add a listener to the windows event if somebody could provide guidance as to how this should be appreciated. As far as I could work out, in the current flutter architecture, this would have to be added to the runner code (in the same way that WM_FONTCHANGE is handled). |
|
@stuartmorgan or @gspencergoog, do y'all have concerns with this? |
|
@Jon-Salmon it looks the CI is failing, I would try rebasing onto the latest |
d50f423 to
6a0f5cc
Compare
|
@loic-sharma Thanks, rebasing and formatting did the trick |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
@stuartmorgan or @gspencergoog, do y'all have concerns with this?
Nope, this is definitely along the lines of the code I expected we'd add at some point when the TODO was first added. Getting it at startup without listening for updates is still a strict improvement over the placeholder value.
Updates scrolling in Windows to respect the 'lines to scroll per scroll wheel tick' setting at app startup. The scrolling amount per tick is also increased to match Chrome (at 100px with default windows settings). This is also similar to what UWP does natively (approximately 96px based on observation). Resolves flutter/flutter#97924
Updates scrolling in Windows to respect the 'lines to scroll per scroll wheel tick' setting at app startup. The scrolling amount per tick is also increased to match Chrome (at 100px with default windows settings). This is also similar to what UWP does natively (approximately 96px based on observation). Resolves flutter/flutter#97924
Updates scrolling in Windows to respect the 'lines to scroll per scroll wheel tick' windows setting.
The scrolling amount per tick is also increased to match chrome (at 100px with default windows settings). This is also similar to what UWP does natively (approximately 96px based on observation).
Resolves flutter/flutter#97924
In this PR, the windows 'lines per tick' value is only checked at startup. It should be possible to be notified when this changes by listening for the WM_SETTINGCHANGE event, but I couldn't get this to work. I suspect there is something I have missed in how events are delivered to the embedding. My attempt at this can be seen in Jon-Salmon@c037310In this PR, the windows 'lines per tick' value is only checked at startup. It is possible to listen for the WM_SETTINGCHANGE event which is passed through to the runner. I'm not sure what the best way would be to add this, it looks like WM_FONTCHANGE is picked up in the runner's code and then passed to the engine. The same could be done for WM_SETTINGSCHANGE, but this would require developers to refresh their runner code.
Pre-launch Checklist
writing and running engine tests.
///).