-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Optimize the scrolling experience #3234
Conversation
SlimeNull
commented
Jul 15, 2024
- Makes scrolling smoother when using the mouse
- Makes scrolling distances more reasonable when scrolling with the touchpad
We are currently not going to add new WPF package dependencies. Do you see a way to change our existing WPF code to incorporate some or all of your improvements? |
Ok, I'll copy the code in a moment and only reference the parts we need |
@ltrzesniewski could you please join the review? Wrt to does it actually do what it says on the tin and whether it could be further simplified? |
The current approach is to put the scrolling optimization part into a separate project. Since both ICSharpCode.TreeView and ILSpy projects need to use this logic, if we don't put it in a separate project, it means that we need to copy this code twice. Is there a better way to do it? After all, scroll optimization is just a rewrite of ScrollViewer's OnMouseWheel |
Sure, I can help, but not before next week - I'm currently on vacation and I don't have access to my dev tools. In the meantime, let's ping @tom-englert: would you be willing to add a smooth scrolling feature to TomsToolbox.Wpf.Styles if this PR could be further simplified? IMO that would be a better place for such code than a separate project here. |
@ltrzesniewski generally yes, however the actual implementation looks like a huge overkill for "just a rewrite of ScrollViewer's OnMouseWheel" |
Yes, I'll see if it can be attached to an existing control and simplified instead of having an inherited control type. |
Maybe @SlimeNull can point out where the core functionality is located? |
The ScrollViewer class in project “EleCho.WpfSuite” is the core logic of scrolling optimization |
After all, there are many things to judge. Such as device type. WPF does not provide how to determine whether the device is a touchpad, so it can only be achieved here by analyzing the scrolling delta value. Of course, the effect is also very good, and there is no misjudgment in the existing test. Also, because this code comes from a WPF library, some customization options are reserved, such as whether to use scrolling easing, whether to use scroll easing in TextBox, and how long the easing is. In addition to this, it also needs to handle the virtualized layout panel to ensure that the scrolling is correct. (The size of the scroll area provided by the virtual panel is also virtual, not pixel size) That's why it's so complicated |
In addition, I also have a scrolling adaptation solution for the stylus device here, I wonder if ILSpy needs this? The core logic is to simulate a touch device to achieve scrolling without affecting existing mouse operations. About touch simulation:WPF Suite - Enhanced Control, Touch Simulation |
If it's all about to capture |
It should be a little bit tricky, because for virtualized panels, we also need to calculate the real scroll distance based on the control's template child, PART_ScrollContentPresenter. GetTemplatedChild is protected. Of course we can use VisualTree to find it. |
Another interesting approach: https://www.wpf-controls.com/wpf-smooth-scroll-viewer/ |
Yes, it would need to find both the scroll viewer and the content presenter via the visual tree, so the behavior can be attached to the original control without modifying the template. |
@SlimeNull here's the scaffold for the behavior: Actually it just blocks mouse wheel; you can just add your scrolling logic. |
Wait, one more question. There is a ScrollViewer in ILSpy that responds to zooming, and it does so by overriding the OnMouseWheel method. If we set IsHandled to true in the PreviewMouseWheel handler, we're not going to be able to get rid of that scaling logic, are we? |
Of course you have to implement it correctly and only set it to true if it was a scroll |
PS. I have tried this with the mouse wheel, and the improvement is marginal, if even noticeable, is it really worth spending time on this? |
For now, the cost of change is just to change the base class of ScrollViewer, ListBox, ListView, but if you want to abstract it into behavior, it will take more time |
I don't really want to abstract it into a behavior, which means you may need to write scaling logic in the behavior. It's not very good. Because not every ScrollViewer has a zoom feature. |
and DataGrid, TextBox, ... and whatever control uses a scroll viewer. Also you not only override the code, you also need to maintain a full copy of all templates of all affected controls - which in your current implementation even messes up the design of the affected controls. And the problem with deriving instead of decorating is that you never can combine any two behaviors if you are not in full control of the source code. |
No one requested to rewrite the scaling logic, just need a behavior for the scrolling logic. |
I have now moved smooth movement to the Behavior implementation. However, once I set Handled to true in the Preview event, the ScrollViewer's OnMouseWheel method is not executed. What should I do? Put the ZoomScrollViewer's zoom-related logic into PreviewMouseWheel as well? I don't want to write zoom-related logic in the Behavior. |
I've switched to the Behavior implementation, but I've also changed the ZoomScrollViewer's OnMouseWheel to OnPreviewMouseWheel. They work fine now. Are there any other changes you'd like to make? @tom-englert |
</Setter.Value> | ||
</Setter> | ||
</Style> | ||
<Application.Resources> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to review if you completely reformat whole files. (Applies to many others, too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you append ?w=1
to the GitHub URL, e.g. https://github.com/icsharpcode/ILSpy/pull/3234/files?w=1 then you can review everything without whitespace changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree that this should be reverted, especially because the only change in this file is the indentation.
@@ -36,7 +36,9 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ICSharpCode.Decompiler.Test | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ICSharpCode.ILSpyX", "ICSharpCode.ILSpyX\ICSharpCode.ILSpyX.csproj", "{F8EFCF9D-B9A3-4BA0-A1B2-B026A71DAC22}" | |||
EndProject | |||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ICSharpCode.BamlDecompiler", "ICSharpCode.BamlDecompiler\ICSharpCode.BamlDecompiler.csproj", "{81A30182-3378-4952-8880-F44822390040}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is BamlDecompiler receiving a new GUID and the new project has the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a VS issue, when editing a solution, old guids will be updated, but new projects start with an old guid.
However the plan is to move the behavior as a public available standard item to toms-toolbox, so this change will be reverted anyway.