Modify

Opened 10 months ago

Last modified 9 months ago

#23102 new enhancement

[RFC PATCH] Poor map navigation on macbooks

Reported by: delusional Owned by: team
Priority: normal Milestone:
Component: Core Version: latest
Keywords: macosx Cc:

Description (last modified by taylor.smock)

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.

Attachments (0)

Change History (4)

comment:1 by delusional, 10 months ago

Keywords: macosx added

comment:2 by taylor.smock, 9 months ago

Description: modified (diff)
Summary: Poor map navigation on macbooks[RFC PATCH] Poor map navigation on macbooks

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()).

comment:3 by delusional, 9 months ago

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

     
    254254              <compilerarg value="-Xplugin:semanticdb -sourceroot:@{srcdir} -targetroot:${build.dir}/semanticdb" if:set="lsif" />
    255255              <classpath>
    256256                  <path refid="runtime.path"/>
     257                  <path refid="compile.path"/>
    257258                  <cp-elements/>
    258259              </classpath>
    259260          </javac>
  • ivy.xml

     
    1919        <conf name="sources" description="Source code of used libraries"/>
    2020    </configurations>
    2121    <dependencies>
     22        <dependency conf="compile->default" org="com.apple" name="AppleJavaExtensions" rev="1.4"/>
    2223        <!-- api->default -->
    2324        <dependency conf="api->default" org="org.openstreetmap.jmapviewer" name="jmapviewer" rev="2.16"/>
    2425        <!-- 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

     
    106115        }
    107116    }
    108117
     118    private double scalef = 0.0;
     119
     120    public final class MagnifyHandler implements com.apple.eawt.event.MagnificationListener {
     121        @Override
     122        public void magnify(com.apple.eawt.event.MagnificationEvent e) {
     123            scalef -= e.getMagnification();
     124
     125            Point mouse = Optional.ofNullable(nc.getMousePosition()).orElseGet(
     126                    () -> new Point((int) nc.getBounds().getCenterX(), (int) nc.getBounds().getCenterY()));
     127            nc.zoomManyTimes(mouse.x, mouse.y, (int)scalef);
     128
     129            scalef -= (int)scalef;
     130        }
     131    }
     132
    109133    /**
    110134     * The point in the map that was the under the mouse point
    111135     * when moving around started.
     
    150174
    151175            registerActionShortcut(new ZoomerAction(".", "MapMover.Zoomer.out"),
    152176                    Shortcut.registerShortcut("view:zoomoutalternate", tr("Map: {0}", tr("Zoom Out")), KeyEvent.VK_PERIOD, Shortcut.CTRL));
     177        } else {
     178            GestureUtilities.addGestureListenerTo(nc, new MagnifyHandler());
    153179        }
    154180    }

comment:4 by anonymous, 9 months ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to delusional.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.