Skip to contentSkip to conversation

ui/cocoa.m: replace scrollingDeltaY with deltaY

PATCH In review John Arbuckle 2 participants 4 messages Jul 9, 2018 – Jul 12, 2018

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 ui/cocoa.m: replace scrollingDeltaY with deltaY

The NSEvent class method scrollingDeltaY is available for Mac OS 10.7 and newer. Since QEMU supports Mac OS 10.5 and up, we need to be using a method that is available on these version of Mac OS X. The deltaY method is a method that does the same thing as scrollingDeltaY and is available on Mac OS 10.5 and up. So we replace scrollingDeltaY with deltaY.

We only check deltaY's value if it is not zero because zero means no scrolling took place.

---
v3 changes:
- Added a comment explaining why we drop scrolling events in both the code and the patch comment.

v2 changes:
- Added a condition that drops scroll events that have a deltaY value of zero.

 ui/cocoa.m | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
ui/cocoa.m +12 -7
index 2991ed4f19..3bae090101 100644
@@ -802,14 +802,19 @@ - (void) handleEvent:(NSEvent *)event
              * This is in-line with standard Mac OS X UI behaviour.
              */
 
+            /*
+             * When deltaY is zero, it means the scrolling device did not move
+             * for this event. So we drop the event.
+             */
+            if ([event deltaY] != 0) {
             /* Determine if this is a scroll up or scroll down event */
-            buttons = ([event scrollingDeltaY] > 0) ?
-                INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
-            qemu_input_queue_btn(dcl->con, buttons, true);
-            qemu_input_event_sync();
-            qemu_input_queue_btn(dcl->con, buttons, false);
-            qemu_input_event_sync();
-
+                buttons = ([event deltaY] > 0) ?
+                    INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
+                qemu_input_queue_btn(dcl->con, buttons, true);
+                qemu_input_event_sync();
+                qemu_input_queue_btn(dcl->con, buttons, false);
+                qemu_input_event_sync();
+            }
             /*
              * Since deltaY also reports scroll wheel events we prevent mouse
              * movement code from executing.
Signature · 1 line
2.14.3 (Apple Git-98)
#1 lore raw
Quoted 10 lines · John Arbuckle

The NSEvent class method scrollingDeltaY is available for Mac OS 10.7 and newer. Since QEMU supports Mac OS 10.5 and up, we need to be using a method that is available on these version of Mac OS X. The deltaY method is a method that does the same thing as scrollingDeltaY and is available on Mac OS 10.5 and up. So we replace scrollingDeltaY with deltaY.

We only check deltaY's value if it is not zero because zero means no scrolling took place.

Not quite -- it means that the scrolling was fine enough that it doesn't add up to a big movement. If you use a fine-scrolling input device (I used the trackpad on my macbook air), then you will get a sequence of events like this for a very slow trackpad scroll:

scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -4
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY 0

(interestingly it does give events with both deltas 0; maybe that event had a horizontal scroll component to it).

Larger scroll gestures get you this sort of thing:

scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY -3 scrollingDeltaY -31
scroll: deltaY -3 scrollingDeltaY -36
scroll: deltaY -2 scrollingDeltaY -30
scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY -1 scrollingDeltaY -16
scroll: deltaY -3 scrollingDeltaY -32
scroll: deltaY -3 scrollingDeltaY -30
scroll: deltaY -2 scrollingDeltaY -28
scroll: deltaY -2 scrollingDeltaY -25
scroll: deltaY -2 scrollingDeltaY -23
scroll: deltaY -1 scrollingDeltaY -19
scroll: deltaY -1 scrollingDeltaY -15
scroll: deltaY -1 scrollingDeltaY -14
scroll: deltaY 0 scrollingDeltaY -12

where OSX has decided that there's enough movement to report a deltaY change, not just the fine-grained scrollingDeltaY.

Anyway, since the QEMU input layer doesn't provide a mechanism for reporting fine-scrolling I guess this patch is OK. If we get complaints about the trackpad no longer being as responsive as it used to be to scrolls then we can look at doing something more complicated then.

I'll tweak the comments and put this in for 3.0 rc1.

Quoted 44 lines

---
v3 changes:
- Added a comment explaining why we drop scrolling events in both the code and the patch comment.

v2 changes:
- Added a condition that drops scroll events that have a deltaY value of zero.

 ui/cocoa.m | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
ui/cocoa.m +12 -8
index 2991ed4f19..3bae090101 100644
@@ -802,14 +802,19 @@ - (void) handleEvent:(NSEvent *)event
              * This is in-line with standard Mac OS X UI behaviour.
              */
 
+            /*
+             * When deltaY is zero, it means the scrolling device did not move
+             * for this event. So we drop the event.
+             */
+            if ([event deltaY] != 0) {
             /* Determine if this is a scroll up or scroll down event */
-            buttons = ([event scrollingDeltaY] > 0) ?
-                INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
-            qemu_input_queue_btn(dcl->con, buttons, true);
-            qemu_input_event_sync();
-            qemu_input_queue_btn(dcl->con, buttons, false);
-            qemu_input_event_sync();
-
+                buttons = ([event deltaY] > 0) ?
+                    INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
+                qemu_input_queue_btn(dcl->con, buttons, true);
+                qemu_input_event_sync();
+                qemu_input_queue_btn(dcl->con, buttons, false);
+                qemu_input_event_sync();
+            }
             /*
              * Since deltaY also reports scroll wheel events we prevent mouse
              * movement code from executing.
--
Quoted 2 lines

2.14.3 (Apple Git-98)

thanks
-- PMM

author
#2 lore raw
Quoted 119 lines · Peter Maydell
Quoted 10 lines · John Arbuckle

The NSEvent class method scrollingDeltaY is available for Mac OS 10.7 and newer. Since QEMU supports Mac OS 10.5 and up, we need to be using a method that is available on these version of Mac OS X. The deltaY method is a method that does the same thing as scrollingDeltaY and is available on Mac OS 10.5 and up. So we replace scrollingDeltaY with deltaY.

We only check deltaY's value if it is not zero because zero means no scrolling took place.

Not quite -- it means that the scrolling was fine enough that it doesn't add up to a big movement. If you use a fine-scrolling input device (I used the trackpad on my macbook air), then you will get a sequence of events like this for a very slow trackpad scroll:

scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -4
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY 0

(interestingly it does give events with both deltas 0; maybe that event had a horizontal scroll component to it).

Larger scroll gestures get you this sort of thing:

scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY -3 scrollingDeltaY -31
scroll: deltaY -3 scrollingDeltaY -36
scroll: deltaY -2 scrollingDeltaY -30
scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY -1 scrollingDeltaY -16
scroll: deltaY -3 scrollingDeltaY -32
scroll: deltaY -3 scrollingDeltaY -30
scroll: deltaY -2 scrollingDeltaY -28
scroll: deltaY -2 scrollingDeltaY -25
scroll: deltaY -2 scrollingDeltaY -23
scroll: deltaY -1 scrollingDeltaY -19
scroll: deltaY -1 scrollingDeltaY -15
scroll: deltaY -1 scrollingDeltaY -14
scroll: deltaY 0 scrollingDeltaY -12

where OSX has decided that there's enough movement to report a deltaY change, not just the fine-grained scrollingDeltaY.

Anyway, since the QEMU input layer doesn't provide a mechanism for reporting fine-scrolling I guess this patch is OK. If we get complaints about the trackpad no longer being as responsive as it used to be to scrolls then we can look at doing something more complicated then.

I'll tweak the comments and put this in for 3.0 rc1.

Quoted 46 lines

---
v3 changes:
- Added a comment explaining why we drop scrolling events in both the code and
the patch comment.

v2 changes:
- Added a condition that drops scroll events that have a deltaY value of zero.

 ui/cocoa.m | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
ui/cocoa.m +2 -0
index 2991ed4f19..3bae090101 100644
@@ -802,14 +802,19 @@ - (void) handleEvent:(NSEvent *)event
              * This is in-line with standard Mac OS X UI behaviour.
              */
 
+            /*
+             * When deltaY is zero, it means the scrolling device  

did not move

ui/cocoa.m +3 -0
+             * for this event. So we drop the event.
+             */
+            if ([event deltaY] != 0) {
             /* Determine if this is a scroll up or scroll down  

event */

ui/cocoa.m +7 -7
-            buttons = ([event scrollingDeltaY] > 0) ?
-                INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
-            qemu_input_queue_btn(dcl->con, buttons, true);
-            qemu_input_event_sync();
-            qemu_input_queue_btn(dcl->con, buttons, false);
-            qemu_input_event_sync();
-
+                buttons = ([event deltaY] > 0) ?
+                    INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
+                qemu_input_queue_btn(dcl->con, buttons, true);
+                qemu_input_event_sync();
+                qemu_input_queue_btn(dcl->con, buttons, false);
+                qemu_input_event_sync();
+            }
             /*
              * Since deltaY also reports scroll wheel events we  
Quoted 8 lines
Quoted 5 lines

prevent mouse
* movement code from executing.
--
2.14.3 (Apple Git-98)

thanks
-- PMM

Excellent job investing.

#3 lore raw
In reply to Peter Maydell
Quoted 9 lines · Peter Maydell
Quoted 8 lines · John Arbuckle

The NSEvent class method scrollingDeltaY is available for Mac OS 10.7 and newer. Since QEMU supports Mac OS 10.5 and up, we need to be using a method that is available on these version of Mac OS X. The deltaY method is a method that does the same thing as scrollingDeltaY and is available on Mac OS 10.5 and up. So we replace scrollingDeltaY with deltaY.

Quoted 4 lines
Quoted 2 lines

We only check deltaY's value if it is not zero because zero means no scrolling took place.

I'll tweak the comments and put this in for 3.0 rc1.

Now applied to master, thanks.

-- PMM