Middle click scrolling #2040

Vijay-Nirmal · 2018-05-01T06:57:26Z

Issue: #1795

PR Type

What kind of change does this PR introduce?

Feature
Documentation content changes
Sample app changes

What is the current behavior?

Middle click scrolling is not available

What is the new behavior?

Added an attached property for ScrollViewer (or any ancestor of ScrollViewer) for middle click scrolling

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Docs have been added/updated which fit documentation template. (for bug fixes / features)
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

I will add a separate sample for MiddleClickScrolling when multisample for a sample page is available.

… App

michael-hawker

This and a couple of your other PRs need to be updated to the latest master as well.

michael-hawker · 2018-05-06T06:16:00Z

docs/extensions/ScrollViewerExtensions.md

-			...
-	</ListView>
+<ListView Name="listView"
+            extensions:ScrollViewerEx.VerticalScrollBarMargin="{Binding MinHeight, ElementName=MyHeaderGrid, Converter={StaticResource DoubleTopThicknessConverter}}">


Class is still ScrollViewerExtensions (as it should be), if you want to abbreviate, I usually make the namespace ex: now. So it'd be ex:ScrollViewerExtensions.

My bad. But why ex:ScrollViewerExtensions? We are using extensions:ScrollViewerExtensions in the master branch.

michael-hawker · 2018-05-06T06:16:34Z

docs/extensions/ScrollViewerExtensions.md

+- Cursor number 102, 103, 104, 105, 106, 107, 108, 109 must be the NorthArror, NorthEastArror, EastArror, SouthEastArror, SouthArror, SouthWestArror, WestArror, NorthWestArror respectively
+- Every cursor will be automatically attached to the corresponding direction of scrolling
+
+### Attached Properties


Do we want one Attached Properties section?

This section is already many docs. It will resolve confusion between Attached Properties and Properties in Properties section

michael-hawker · 2018-05-06T06:19:37Z

docs/extensions/ScrollViewerExtensions.md

+### Changing Cursor Type
+
+> [!IMPORTANT]
+Resource file must be manually added to change the cursor type when middle click scrolling.


I'm not sure if I follow what this means to the developer. Is it required to do these steps to use the extension?

michael-hawker · 2018-05-06T06:19:51Z

docs/extensions/ScrollViewerExtensions.md

+- You need 9 cursor resource in your resource file
+- Your cursor number should be 101 to 109
+- Cursor number 101 must be the centre cursor
+- Cursor number 102, 103, 104, 105, 106, 107, 108, 109 must be the NorthArror, NorthEastArror, EastArror, SouthEastArror, SouthArror, SouthWestArror, WestArror, NorthWestArror respectively


Typos Arrow not Arror

michael-hawker · 2018-05-06T06:32:42Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+
+            if (_oldCursorID != cursorID)
+            {
+                RunInUIThread(() =>


Use the DispatcherHelper?

RunInUIThread is way smaller than DispatcherHelper. Also, It increases readability.

michael-hawker · 2018-05-06T06:34:01Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+    /// </summary>
+    public static partial class ScrollViewerExtensions
+    {
+        private static double _threshold = 50;


Have you tested with multiple scrollviewers?

All these static properties and variables means it's tied to a single instance of a scroll viewer. If a page has multiple scrollviewers trying to use this property it could breakdown. It'd be better to make state variables private attached properties to actually store state attached to the scrollviewer that's being manipulated.

Yes. I have a SubscribeMiddleClickScrolling(). This will we resetting every value depend open the currently active ScrolViewer.

I don't think the static variables are the problem here, but actually the events. They need to be static, so they'll never dispose, and the deregistration of the events is only happening when the EnableMiddleClickScrolling is set to false, which is... never. So this is leaking a reference to the scrollviewer.

michael-hawker · 2018-05-06T06:38:51Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+            {
+                cursorID = 101;
+            }
+            else


Can these all be collapsed in the else if, there's only going to be one cursorid in the end, eh?

Not sure if the compiler would optimize, but you could split out the Abs calculation above as you use it in multiple places.

Vijay-Nirmal · 2018-05-11T17:36:55Z

@michael-hawker ping

michael-hawker · 2018-05-11T19:46:32Z

Was able to grab my mouse with a wheel finally. I noticed if I middle-click (vs hold) to enter scroll mode, sometimes the mouse cursor won't change to the cross.

Also, the cursors should be white with the black outline by default for better contrast in more scenarios.

Vijay-Nirmal · 2018-05-12T12:51:59Z

I noticed if I middle-click (vs hold) to enter scroll mode, sometimes the mouse cursor won't change to the cross

I think I have fixed it. @michael-hawker Could you check that again?

nmetulev

Setting the value to False after setting it to True throws an exception

nmetulev · 2018-05-14T00:11:51Z

...oft.Toolkit.Uwp.SampleApp/SamplePages/ScrollViewerExtensions/ScrollViewerExtensionsCode.bind

@@ -13,7 +13,8 @@

  <Grid Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
    <ListView Name="listView"
-              extensions:ScrollViewerExtensions.VerticalScrollBarMargin="{Binding MinHeight, ElementName=MyHeaderGrid, Converter={StaticResource DoubleTopThicknessConverter}}">
+              extensions:ScrollViewerExtensions.VerticalScrollBarMargin="{Binding MinHeight, ElementName=MyHeaderGrid, Converter={StaticResource DoubleTopThicknessConverter}}"
+              extensions:ScrollViewerExtensions.EnableMiddleClickScrolling="True">


This should be using the sample bind syntax @[EnableMiddleClickScrolling:Bool:True]

nmetulev · 2018-05-14T00:17:22Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+        }
+
+        /// <summary>
+        /// This function will be called for every small intervel by <see cref="Timer"/>


typo: interval

nmetulev · 2018-05-14T00:18:11Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+            _currentPosition = default(Point);
+            _isDeferredMovingStarted = false;
+            _oldCursorID = 100;
+            _timer.Dispose();


_timer could be null here if SubscribeMiddleClickScrolling is not called before

nmetulev · 2018-05-14T00:24:50Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+
+                // Event to stop deferred scrolling if pointer pressed
+                Window.Current.CoreWindow.PointerPressed -= CoreWindow_PointerPressed;
+                Window.Current.CoreWindow.PointerPressed += CoreWindow_PointerPressed;


Why subscribe to both CoreWindow and scrollviewer events?

ScrollViewer.PointerPressed is for SubscribeMiddleClickScrolling. CoreWindow.PointerPressed is to stop deferred scrolling. I can't use ScrollViewer.PointerPressed because I should stop scrolling in PointerPressed in the whole app.

nmetulev · 2018-05-14T00:27:25Z

docs/extensions/ScrollViewerExtensions.md

+### Changing Cursor Type
+
+> [!IMPORTANT]
+Resource file must be manually added to change the cursor type when middle click scrolling. If you didn't add then the cursor won't change when middle click scrolling but funcanilaty won't be affected.


typo: functionality

nmetulev · 2018-05-14T00:45:49Z

docs/extensions/ScrollViewerExtensions.md

+1. Download [MiddleClickScrolling-CursorType.res](https://github.com/Microsoft/UWPCommunityToolkit/tree/master/Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/MiddleClickScrolling-CursorType.res) file
+2. Move this file into your project's folder
+2. Open .csproj file of your project in [Visual Studio Code](https://code.visualstudio.com/) or in any other code editor
+3. Added `<Win32Resource>MiddleClickScrolling-CursorType.res</Win32Resource>` in the first `<PropertyGroup>`


nmetulev · 2018-05-14T00:48:17Z

docs/extensions/ScrollViewerExtensions.md

+
+### Using Your Own Resource File
+
+- You need 9 cursor resource in your resource file


Typo: 9 cursor resources

Changed the whole section

nmetulev · 2018-05-14T00:48:46Z

docs/extensions/ScrollViewerExtensions.md

+2. Open .csproj file of your project in [Visual Studio Code](https://code.visualstudio.com/) or in any other code editor
+3. Added `<Win32Resource>MiddleClickScrolling-CursorType.res</Win32Resource>` in the first `<PropertyGroup>`
+
+### Using Your Own Resource File


Is there documentation that we can link to for creating custom resource files?

There is an old doc for Windows 8 but it is not necessary. We can just edit the existing resource file in Visual Studio.

Changed the whole section

nmetulev · 2018-05-14T00:49:16Z

docs/extensions/ScrollViewerExtensions.md


-The HorizontalScrollBarMargin works exactly as VerticalScrollBarMargin.
+[ScrollViewerExtensions sample page Source](https://github.com/Microsoft/UWPCommunityToolkit/tree/master/Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ScrollViewerExtensions). You can see this in action in [UWP Community Toolkit Sample App](https://www.microsoft.com/store/apps/9NBLGGH4TLCQ).


change to Windows Community Toolkit Sample App

I will change it

azchohfi · 2018-05-18T18:27:02Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+    /// </summary>
+    public static partial class ScrollViewerExtensions
+    {
+        private static double _threshold = 50;


I don't think the static variables are the problem here, but actually the events. They need to be static, so they'll never dispose, and the deregistration of the events is only happening when the EnableMiddleClickScrolling is set to false, which is... never. So this is leaking a reference to the scrollviewer.

azchohfi · 2018-05-18T18:30:13Z

Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs

+            _currentPosition = default(Point);
+            _isDeferredMovingStarted = false;
+            _oldCursorID = 100;
+            _timer = new Timer(Scroll, null, 5, 5);


Just to be extra safe, I would do a _timer?.Dispose() before this line.

OK

This is reply is for above comment. For some reason, I have no replay section 😄

EnableMiddleClickScrolling is set to false, which is... never

Why? Since this is an attached property. Devs can set any value at any time

but actually the events. They need to be static, so they'll never dispose, and the deregistration of the events is only happening when the EnableMiddleClickScrolling is set to false

Deregistration also haves when the user changes the focused ScrollViewer (in multiple ScrollViewer scenario).

Vijay-Nirmal added 2 commits May 1, 2018 10:30

Added middle click scrolling attached property

1b4afac

Fix crash when resource is not available and added resource to Sample…

5b22241

… App

Vijay-Nirmal self-assigned this May 2, 2018

Vijay-Nirmal added feature 💡 reviewers wanted extensions ⚡ labels May 2, 2018

Vijay-Nirmal added this to the 3.0 milestone May 2, 2018

michael-hawker requested changes May 6, 2018

View reviewed changes

Review fixes

c285f80

ScrollViewerEx name fix

3d01eab

Bug Fix

c1d7530

Vijay-Nirmal and others added 2 commits May 12, 2018 21:56

Updated arrows with white color border

ad288a1

Merge branch 'master' into MiddleClickScrolling

9fe6286

nmetulev requested changes May 14, 2018

View reviewed changes

Vijay-Nirmal and others added 2 commits May 15, 2018 20:04

Review fixes and a bug fix

8a88deb

Merge branch 'master' into MiddleClickScrolling

2ef09c1

nmetulev approved these changes May 15, 2018

View reviewed changes

azchohfi added 2 commits May 17, 2018 11:50

Merge branch 'master' into MiddleClickScrolling

837d849

Merge branch 'master' into MiddleClickScrolling

4e7cfaf

azchohfi requested changes May 18, 2018

View reviewed changes

Vijay-Nirmal and others added 2 commits May 20, 2018 19:04

Review changes

ec79f84

Merge branch 'master' into MiddleClickScrolling

709f28f

azchohfi approved these changes May 21, 2018

View reviewed changes

azchohfi merged commit 8190e6d into CommunityToolkit:master May 21, 2018


		### Using Your Own Resource File

		- You need 9 cursor resource in your resource file


		The HorizontalScrollBarMargin works exactly as VerticalScrollBarMargin.
		[ScrollViewerExtensions sample page Source](https://github.com/Microsoft/UWPCommunityToolkit/tree/master/Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ScrollViewerExtensions). You can see this in action in [UWP Community Toolkit Sample App](https://www.microsoft.com/store/apps/9NBLGGH4TLCQ).

Middle click scrolling #2040

Middle click scrolling #2040

Conversation

Vijay-Nirmal commented May 1, 2018

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

michael-hawker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal commented May 11, 2018

michael-hawker commented May 11, 2018

Vijay-Nirmal commented May 12, 2018 • edited Loading

nmetulev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmetulev May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-Nirmal May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Vijay-Nirmal May 6, 2018 •

edited

Loading

Vijay-Nirmal commented May 12, 2018 •

edited

Loading

nmetulev May 14, 2018 •

edited

Loading

Vijay-Nirmal May 19, 2018 •

edited

Loading