Add Scrollbar Context Menu #19413
Add Scrollbar Context Menu #19413
Conversation
|
You can test this PR using the following package version. |
|
Any reasons to keep it in C# code? I think whole context flyout can be moved to XAML, and methods like For the ":vertical" and ":horizontal" we already have pseudoclasses to be used in XAML. RTL makes it a bit more complicated though. Upd: nevermind, |
|
I had cloned it from the original WPF code, which is in C# and internal. So, it's not technical; I wanted to ensure it operated the same. Now that I have unit tests there, I would be open to changing it. This control is interesting, since it's less a "WPF" feature, but a Windows thing that's implemented in WPF. WPF doesn't allow for changes to this control, but I would be open to it. |
|
|
||
| private Track? Track => this.GetTemplateChildren().OfType<Track>().FirstOrDefault(); | ||
|
|
||
| private class SimpleCommand : ICommand |
There was a problem hiding this comment.
We shouldn't really put classes in the same file, even private ones. This could be used other places in the future. On top of that -- why not just use a more standard RelayCommand-based approach?
There was a problem hiding this comment.
This could be used other places in the future
Then it could be moved at that time? This was intended for this specific control, not something for other controls to use, and if it ever did, it could be moved then.
That said I don't really care if it's here or its own file, as long as it's not public.
On top of that -- why not just use a more standard RelayCommand-based approach?
I'm not against it. WPF uses static RoutedCommands. I generally go for simpiler implementations when I can and try to limit what's needed for a thing to just the things needed, and using a straight ICommand seemed right, especially since the intention for this was not a public API for a very specific implementation for this control.
If the intention is to expand it as a public interface to change, then that would change the needs.
| } | ||
| } | ||
|
|
||
| private static void ScrollToTopAction(ScrollBar scrollBar) |
There was a problem hiding this comment.
Adding the suffix Action at the end doesn't really follow any conventions I'm aware of. Additionally, there is an argument to be made that these could become public in the future. and also... not static.
There was a problem hiding this comment.
If we made it public, it could be changed then. But it's not. My thought process was "I need a name for this so I can call it" and that's what I wrote, lol.
If there is enough push to change it, I will, especially if it were to be made public since then it would matter more but it's not, so i'm not sure that matters.
| } | ||
| } | ||
|
|
||
| private void OnContextRequested(object? sender, ContextRequestedEventArgs e) |
There was a problem hiding this comment.
Even Microsoft has forgotten the original convention here. But naming with OnContextRequested is traditionally reserved for protected virtual methods that can be overridden (like OnPointerPressed above). Event handlers have the Control_ContextRequested convention -- yes, with an underscore. That is an old tradition I know but it makes total sense in cases like this. You know which one is which immediately.
I think this all started getting convoluted around the time of Xamarin Forms and then Maui.
There was a problem hiding this comment.
As I said above, I was just trying to map to WPF here. As it's private anyway, I don't think it matters that much but changing it is no cost so if people want to change it, everyone should decide on a name and I'll change it to it.
|
In case we stick with the code behind context Menu, would it be possible to override it? I mean what if I as the App developer have a special need for own context Menu? Take an IDE where you can have an entry "Scroll to next error" or something. |
|
Since you keep track of lastRightPosition, that field should also be consumeable from the XAML version. Note Drawback of the XAML solution is that 3rd party themes have to pick up these changes, so it has to be highlighted in release notes. |
Fixes #19404
What does the pull request do?
Adds a context menu to the scroll bar, matching functionality in WPF
What is the current behavior?
Windows (and WPF) have a context menu for scrollbars where you can right click and navigate them. This PR tries to match that function.
What is the updated/expected behavior with this PR?
Right click the scroll bar, interact with context menu.
How was the solution implemented (if it's not obvious)?
Replicating the functions of the WPF ScrollBar implementation. It should more-or-less match the original code that was used in WPF, apart from the resource file handling, which was put into the Invariant Strings XAML file.