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

Optimize the scrolling experience #3234

Closed
wants to merge 8 commits into from

Conversation

SlimeNull
Copy link

  1. Makes scrolling smoother when using the mouse
  2. Makes scrolling distances more reasonable when scrolling with the touchpad

ilspy-scrolling2

@christophwille
Copy link
Member

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?

@SlimeNull
Copy link
Author

SlimeNull commented Jul 15, 2024

Ok, I'll copy the code in a moment and only reference the parts we need

@christophwille
Copy link
Member

@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?

@SlimeNull
Copy link
Author

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

@ltrzesniewski
Copy link
Contributor

@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?

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.

@tom-englert
Copy link
Contributor

@ltrzesniewski generally yes, however the actual implementation looks like a huge overkill for "just a rewrite of ScrollViewer's OnMouseWheel"

@ltrzesniewski
Copy link
Contributor

ltrzesniewski commented Jul 16, 2024

Yes, I'll see if it can be attached to an existing control and simplified instead of having an inherited control type.

@tom-englert
Copy link
Contributor

Maybe @SlimeNull can point out where the core functionality is located?

@SlimeNull
Copy link
Author

The ScrollViewer class in project “EleCho.WpfSuite” is the core logic of scrolling optimization

@SlimeNull
Copy link
Author

@ltrzesniewski generally yes, however the actual implementation looks like a huge overkill for "just a rewrite of ScrollViewer's OnMouseWheel"

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

@SlimeNull
Copy link
Author

SlimeNull commented Jul 16, 2024

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

@tom-englert
Copy link
Contributor

tom-englert commented Jul 16, 2024

If it's all about to capture OnMouseWheel, maybe a Behavior that finds the ScrollViewer as a child of the control and captures the PreviewMouseWheel event will do the job?

@SlimeNull
Copy link
Author

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.

@tom-englert
Copy link
Contributor

Another interesting approach: https://www.wpf-controls.com/wpf-smooth-scroll-viewer/

@tom-englert
Copy link
Contributor

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.

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.

@tom-englert
Copy link
Contributor

@SlimeNull here's the scaffold for the behavior:

https://github.com/tom-englert/ILSpy/blob/dev/smoothscrolling/EleCho.WpfSuite/Controls/SmoothScrollingBehavior.cs

Actually it just blocks mouse wheel; you can just add your scrolling logic.

@SlimeNull
Copy link
Author

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?

@tom-englert
Copy link
Contributor

Of course you have to implement it correctly and only set it to true if it was a scroll

@tom-englert
Copy link
Contributor

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?

@SlimeNull
Copy link
Author

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

@SlimeNull
Copy link
Author

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.

@tom-englert
Copy link
Contributor

For now, the cost of change is just to change the base class of ScrollViewer, ListBox, ListView,

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.

@tom-englert
Copy link
Contributor

I don't really want to abstract it into a behavior, which means you may need to write scaling logic in the behavior

No one requested to rewrite the scaling logic, just need a behavior for the scrolling logic.

@SlimeNull
Copy link
Author

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.

@SlimeNull
Copy link
Author

SlimeNull commented Jul 23, 2024

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

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)

Copy link
Member

@siegfriedpammer siegfriedpammer Jul 23, 2024

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.

Copy link
Member

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}"
Copy link
Member

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?

Copy link
Contributor

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.

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.

5 participants