[RFC PATCH] Poor map navigation on macbooks

[delusional]

I'm using an M2 macbook for on-the-go mapping, and I've found that moving around the map is cumbersome.

From a user perspective I see two problems. The first is a mismatch between the platform native gestures and JOSM. In macos two finger dragging is generally panning, but because it's still presented to the application as scrolling, JOSM interprets it as zooming. This forces panning to be a two finger click-drag which doesn't feel good (pushing down and dragging at the same time creates too much friction to feel good). The second problem is around integer input handling. Zooming, currently scrolling but ideally a multitouch gesture, feels clunky and, overly sensitive, and doesn't react well to the magnitude of the movement. Looking at the code this seems to be caused by zooming being an integer operation, and macos generally relying on many subinteger (fractional) events to smoothly scroll.

I've been looking through the code, and it looks like we can get closer to something good with some changes to src/org/openstreetmap/josm/gui/MapMover.java. Scrolling can be turned into (very nice and smooth) panning by changing mouseWheelMoved:

@@ -252,8 +317,19 @@
      */
     @Override
     public void mouseWheelMoved(MouseWheelEvent e) {
-        int rotation = PROP_ZOOM_REVERSE_WHEEL.get() ? -e.getWheelRotation() : e.getWheelRotation();
-        nc.zoomManyTimes(e.getX(), e.getY(), rotation);
+
+        float fracx = 0.0f;
+        float fracy = 0.0f;
+
+        if((e.getModifiersEx() & MouseEvent.SHIFT_DOWN_MASK) == 0) {
+            fracy -= e.getPreciseWheelRotation() * 8; // 8 feels good
+        } else {
+            fracx += e.getPreciseWheelRotation() * 8;
+        }
+
+        nc.zoomTo(nc.getCenter().add(fracx * nc.getScale(), fracy * nc.getScale()));
     }

This leaves us without a zoom gesture. On macos the multitouch pinch gesture is accessed through the apple java specific GestureUtilities, We can use reflection to dynamically link against it (this was taken from ​https://gist.github.com/alanwhite/42502f20390baf879d093691ebb72066). Note that I'm manually keeping track of the fractional part of the zoom to make it feel a little better. Ideally the mapview should support fractional zooming internally, but I like this first change being single file:

@@ -106,6 +114,33 @@
         }
     }

+    private double scalef = 0.0;
+
+    public final class MagnifyHandler implements InvocationHandler {
+        public Object invoke(Object proxy, Method method, Object[] args) {
+            // method.getName() should always return "magnify" as that's all we subscribed to
+            // production code may wish to double check this.
+            try {
+                for(Object o: args) {
+                    Object mag = o.getClass()
+                        .getMethod("getMagnification")
+                        .invoke(o);
+
+                    scalef -= ((double)mag);
+
+                    Point mouse = Optional.ofNullable(nc.getMousePosition()).orElseGet(
+                        () -> new Point((int) nc.getBounds().getCenterX(), (int) nc.getBounds().getCenterY()));
+                    nc.zoomManyTimes(mouse.x, mouse.y, (int)scalef);
+
+                    scalef -= (int)scalef;
+                }
+            } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+                throw new AssertionError(e);
+            }
+            return null;
+        }
+    }
+
     /**
      * The point in the map that was the under the mouse point
      * when moving around started.
@@ -150,6 +185,36 @@

             registerActionShortcut(new ZoomerAction(".", "MapMover.Zoomer.out"),
                     Shortcut.registerShortcut("view:zoomoutalternate", tr("Map: {0}", tr("Zoom Out")), KeyEvent.VK_PERIOD, Shortcut.CTRL));
+        } else {
+            try {
+                // Run with --add-opens java.desktop/com.apple.eawt.event=ALL-UNNAMED
+                Constructor[] constructors = Class.forName("com.apple.eawt.event.GestureUtilities")
+                    .getDeclaredConstructors();
+
+                Object gu=null;
+
+                for (Constructor constructor : constructors)
+                {
+                    constructor.setAccessible(true);
+                    gu = constructor.newInstance();
+                    break;
+                }
+
+                Object mh = Proxy.newProxyInstance(
+                        Class.forName("com.apple.eawt.event.MagnificationListener").getClassLoader(),
+                        new Class[]{Class.forName("com.apple.eawt.event.MagnificationListener")},
+                        new MagnifyHandler()
+                        );
+
+                gu.getClass()
+                    .getMethod("addGestureListenerTo",
+                            Class.forName("javax.swing.JComponent"),
+                            Class.forName("com.apple.eawt.event.GestureListener"))
+                    .invoke(gu, nc, mh);
+            }
+            catch (Exception e) {
+                throw new AssertionError(e);
+            }
         }
     }

I'm aware that this isn't currently production quality. Right now I'm focused on opening up a discussion around how the maintainers think about this problem, and my proposed solution. The proper patch will have error handling, more specific exception handling, and possibly include configuration knobs for which codepath the user wants.

[taylor.smock]

For the first one, I don't see a problem, but I would want to test it on different systems. With that said, we (kind of) already support that, via the JOSM Preferences -> Display -> Zoom steps to get double scale setting. So I don't know if we really need it.

For the second patch, I'm really not big fan. I don't know what version of Java you are using, but newer versions of Java have been cracking down on reflection, so I'd really like to avoid new (non-test) usage of reflection.

That is probably why you have the Run with --add-opens java.desktop/com.apple.eawt.event=ALL-UNNAMED comment. Realistically, that else statement should be checking to see if we are on mac (PlatformManager.isPlatformOsx()).

[delusional]

Thanks for the comments :)

I think I may have confused the discussion by saying there were two classes of issues and then presenting two patches. The patches do not solve an issue each, but are both part of solving both observed issues affecting map navigation on macbooks.

The first patch is about changing the behavior of the scroll wheel from zooming to panning, since that's the behavior a macbook user would expect. This would have to be guarded by some option to maintain the current zoom behavior on other systems/formfactors. That guard is left unimplemented to highlight the core logic change, and because I don't know if just doing an if on a property in that method would be idiomatic.

To be completely clear, Zoom steps to get double scale doesn't affect the first patch since it's not about zooming.

On the second patch, I agree that use of the reflection is poor, mostly it's to avoid taking on an external dependency for stubbing out the GestureUtilities during compile (the else is already a macos guard, the if it's attached to reads if(!isPlatformOsx())). I don't know what your stance is on external dependencies, but I've never used ant/ivy. If you are ok with taking on an external dependency to a compile time stub that is no longer maintained, we can remove the reflection. The change requires applying this patch to add the dependency, and to include the compile conf dependencies in the compile classpath (I believe the fact that they weren't may have been a bug).

build.xml

@@ -254 +254 @@
               <compilerarg value="-Xplugin:semanticdb -sourceroot:@{srcdir} -targetroot:${build.dir}/semanticdb" if:set="lsif" />
               <classpath>
                   <path refid="runtime.path"/>
+                  <path refid="compile.path"/>
                   <cp-elements/>
               </classpath>
           </javac>

ivy.xml

@@ -19 +19 @@
         <conf name="sources" description="Source code of used libraries"/>
     </configurations>
     <dependencies>
+        <dependency conf="compile->default" org="com.apple" name="AppleJavaExtensions" rev="1.4"/>
         <!-- api->default -->
         <dependency conf="api->default" org="org.openstreetmap.jmapviewer" name="jmapviewer" rev="2.16"/>
         <!-- The javax json dependencies should be removed sometime in 2024 -->

With this the second patch becomes a lot cleaner. I've kept the manual scale fraction handling, but this is a place where we could arguably use Zoom steps to get double scale by multiplying the (sub integer) magnitude with like 10 and tell mac users to set Zoom steps to get double scale to 10 to get good zoom behavior. I'm not against that.

src/org/openstreetmap/josm/gui/MapMover.java

@@ -106 +115 @@
         }
     }
 
+    private double scalef = 0.0;
+
+    public final class MagnifyHandler implements com.apple.eawt.event.MagnificationListener {
+        @Override
+        public void magnify(com.apple.eawt.event.MagnificationEvent e) {
+            scalef -= e.getMagnification();
+
+            Point mouse = Optional.ofNullable(nc.getMousePosition()).orElseGet(
+                    () -> new Point((int) nc.getBounds().getCenterX(), (int) nc.getBounds().getCenterY()));
+            nc.zoomManyTimes(mouse.x, mouse.y, (int)scalef);
+
+            scalef -= (int)scalef;
+        }
+    }
+
     /**
      * The point in the map that was the under the mouse point
      * when moving around started.
@@ -150 +174 @@
 
             registerActionShortcut(new ZoomerAction(".", "MapMover.Zoomer.out"),
                     Shortcut.registerShortcut("view:zoomoutalternate", tr("Map: {0}", tr("Zoom Out")), KeyEvent.VK_PERIOD, Shortcut.CTRL));
+        } else {
+            GestureUtilities.addGestureListenerTo(nc, new MagnifyHandler());
         }
     }

[anonymous]

It turns out I can't exclude the dependency at runtime. The JVM tries to load the class when it sees the reference to it (even if the code path is never taken) and fails with a ClassNotFoundException. If I include the AppleJavaExtensions at runtime it works. I don't know if a stub dependency is preferable to the reflection stuff necessary to avoid it, you'll have to provide me some guidance there.

Instead of bloating this thread with additional patches I've forked the josm mirror on github and added my changes[1]. This seems to be the minimal set of changes needed to make the feature work. I've been using the patch on my macbook while out mapping and it's a much more enjoyable experience than the current behavior. Note that I've added a property to chose what behavior you want. The name is admittedly bad.

[1] ​https://github.com/DelusionalLogic/josm