Changeset 19643 in josm

Timestamp:

2026-10-06T22:46:46+02:00 (3 days ago)

Author:

gaben

Message:

fix #23002 - keep notifications aligned to the map view

• follow the map view on fullscreen toggle, maximize and other layout changes
• scale down icons larger than ImageSizes.NOTIFICATION (new, 32px)
• fix a possible deadlock when a notification is shown from a worker thread
• drop duplicates anywhere in the queue, not only the last one
• remove the transparent margins that blocked clicks on the map

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/Notification.java (modified) (4 diffs)
• src/org/openstreetmap/josm/gui/NotificationManager.java (modified) (19 diffs)
• src/org/openstreetmap/josm/tools/ImageProvider.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/gui/NotificationManagerTest.java (added)
• test/unit/org/openstreetmap/josm/gui/NotificationTest.java (added)

trunk/src/org/openstreetmap/josm/gui/Notification.java

@@ -7 +7 @@
 
 import javax.swing.Icon;
+import javax.swing.ImageIcon;
 import javax.swing.JEditorPane;
 import javax.swing.JOptionPane;
@@ -14 +15 @@
 import org.openstreetmap.josm.gui.widgets.JMultilineLabel;
 import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.tools.ImageProvider.ImageSizes;
+import org.openstreetmap.josm.tools.ImageResource;
 
 /**
@@ -130 +133 @@
     /**
      * Set an icon to display on the left part of the message window.
+     * <p>
+     * Icons larger than {@link ImageSizes#NOTIFICATION} are scaled down.
      *
      * @param icon the icon (null means no icon is displayed)
@@ -135 +140 @@
      */
     public Notification setIcon(Icon icon) {
-        this.icon = icon;
+        if (icon instanceof ImageIcon && (icon.getIconWidth() > ImageSizes.NOTIFICATION.getAdjustedWidth()
+                || icon.getIconHeight() > ImageSizes.NOTIFICATION.getAdjustedHeight())) {
+            this.icon = new ImageResource(((ImageIcon) icon).getImage()).getImageIconBounded(ImageSizes.NOTIFICATION.getImageDimension());
+        } else {
+            this.icon = icon;
+        }
         return this;
     }

trunk/src/org/openstreetmap/josm/gui/NotificationManager.java

@@ -5 +5 @@
 
 import java.awt.BasicStroke;
+import java.awt.BorderLayout;
 import java.awt.Color;
 import java.awt.Component;
@@ -13 +14 @@
 import java.awt.Insets;
 import java.awt.Point;
+import java.awt.Rectangle;
 import java.awt.RenderingHints;
 import java.awt.Shape;
 import java.awt.event.ActionEvent;
 import java.awt.event.ActionListener;
+import java.awt.event.ComponentAdapter;
+import java.awt.event.ComponentEvent;
+import java.awt.event.ComponentListener;
+import java.awt.event.HierarchyEvent;
+import java.awt.event.HierarchyListener;
 import java.awt.event.MouseAdapter;
 import java.awt.event.MouseEvent;
@@ -26 +33 @@
 
 import javax.swing.AbstractAction;
-import javax.swing.BorderFactory;
 import javax.swing.GroupLayout;
 import javax.swing.JButton;
@@ -41 +47 @@
 import org.openstreetmap.josm.gui.help.HelpUtil;
 import org.openstreetmap.josm.gui.util.GuiHelper;
+import org.openstreetmap.josm.tools.GuiSizesHelper;
 import org.openstreetmap.josm.tools.ImageProvider;
 import org.openstreetmap.josm.tools.Logging;
@@ -46 +53 @@
 /**
  * Manages {@link Notification}s, i.e. displays them on screen.
- *
+ * <p>
  * Don't use this class directly, but use {@link Notification#show()}.
- *
+ * <p>
  * If multiple messages are sent in a short period of time, they are put in
  * a queue and displayed one after the other.
- *
+ * <p>
  * The user can stop the timer (freeze the message) by moving the mouse cursor
  * above the panel. As a visual cue, the background color changes from
@@ -65 +72 @@
     private Notification currentNotification;
     private NotificationPanel currentNotificationPanel;
+
+    /** the component {@link #currentNotificationPanel} is aligned to, {@code null} while nothing is displayed */
+    private Component notificationAnchor;
+
+    /** keeps the displayed notification aligned when the layout around it changes, e.g. in fullscreen mode */
+    private final ComponentListener anchorListener = new ComponentAdapter() {
+        @Override
+        public void componentResized(ComponentEvent e) {
+            updateNotificationPosition();
+        }
+
+        @Override
+        public void componentMoved(ComponentEvent e) {
+            updateNotificationPosition();
+        }
+    };
+
+    /** realigns the displayed notification when toggling fullscreen, before the window is painted with it at its old position */
+    private final HierarchyListener showingListener = e -> {
+        if ((e.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) != 0 && e.getComponent().isShowing()) {
+            updateNotificationPosition();
+        }
+    };
+
+    /** brings the displayed notification over to the map view as soon as one is opened, and back when it is closed */
+    private final MapFrameListener mapFrameListener = (oldFrame, newFrame) -> updateNotificationPosition();
+
     private final Deque<Notification> queue;
 
@@ -74 +108 @@
     private static NotificationManager instance;
 
+    /** margin between the notification panel and the borders of its anchor, in unscaled pixels */
+    private static final int MARGIN = 10;
+
     private static final Color PANEL_SEMITRANSPARENT = new Color(224, 236, 249, 230);
     private static final Color PANEL_OPAQUE = new Color(224, 236, 249);
@@ -79 +116 @@
     NotificationManager() {
         queue = new LinkedList<>();
-        hideTimer = new Timer(Notification.TIME_DEFAULT, e -> this.stopHideTimer());
+        hideTimer = new Timer(Notification.TIME_DEFAULT, e -> this.stopHideTimer(null));
         hideTimer.setRepeats(false);
         pauseTimer = new Timer(pauseTime.get(), new PauseFinishedEvent());
@@ -88 +125 @@
 
     /**
-     * Show the given notification (unless a duplicate notification is being shown at the moment or at the end of the queue)
+     * Show the given notification (unless a duplicate notification is being shown at the moment or is already queued)
      * @param note The note to show.
      * @see Notification#show()
@@ -94 +131 @@
     void showNotification(Notification note) {
         synchronized (queue) {
-            if (Objects.equals(note, currentNotification) || Objects.equals(note, queue.peekLast())) {
+            if (Objects.equals(note, currentNotification) || queue.contains(note)) {
                 Logging.debug("Dropping duplicate notification {0}", note);
                 return;
             }
             queue.add(note);
-            processQueue();
-        }
+        }
+        // must not run while the monitor is held, see processQueue()
+        processQueue();
     }
 
@@ -109 +147 @@
      */
     void replaceExistingNotification(Notification oldNotification, Notification newNotification) {
+        boolean isDisplayed;
         synchronized (queue) {
-            if (Objects.equals(oldNotification, currentNotification)) {
-                stopHideTimer();
-            } else {
+            isDisplayed = Objects.equals(oldNotification, currentNotification);
+            if (!isDisplayed) {
                 queue.remove(oldNotification);
             }
-            showNotification(newNotification);
-            processQueue();
-        }
-    }
-
+        }
+        if (isDisplayed) {
+            // must not run while the monitor is held either, it waits for the EDT as well
+            stopHideTimer(oldNotification);
+        }
+        showNotification(newNotification);
+    }
+
+    /**
+     * Displays the next queued notification, unless one is being displayed already or the queue is empty.
+     * <p>
+     * Only the state transition is guarded by the monitor of {@link #queue}. The rest waits for the EDT, and the EDT
+     * takes that very monitor as well, so holding it any longer would deadlock every caller
+     * that is not the EDT itself.
+     */
     private void processQueue() {
-        if (running) return;
-
-        currentNotification = queue.poll();
-        if (currentNotification == null) return;
+        synchronized (queue) {
+            if (running) return;
+
+            currentNotification = queue.poll();
+            if (currentNotification == null) return;
+
+            // claim the slot before releasing the monitor, so that no concurrent call displays a second notification
+            running = true;
+        }
 
         GuiHelper.runInEDTAndWait(() -> {
-            currentNotificationPanel = new NotificationPanel(currentNotification, new FreezeMouseListener(), e -> this.stopHideTimer());
+            currentNotificationPanel = new NotificationPanel(currentNotification, new FreezeMouseListener(), e -> this.stopHideTimer(null));
+            currentNotificationPanel.addHierarchyListener(showingListener);
             currentNotificationPanel.validate();
 
-            int margin = 5;
+            currentNotificationPanel.setSize(currentNotificationPanel.getPreferredSize());
+
             JFrame parentWindow = MainApplication.getMainFrame();
-            Dimension size = currentNotificationPanel.getPreferredSize();
             if (parentWindow != null) {
-                int x;
-                int y;
-                MapFrame map = MainApplication.getMap();
-                if (MainApplication.isDisplayingMapView() && map.mapView.getHeight() > 0) {
-                    MapView mv = map.mapView;
-                    Point mapViewPos = SwingUtilities.convertPoint(mv.getParent(), mv.getX(), mv.getY(), MainApplication.getMainFrame());
-                    x = mapViewPos.x + margin;
-                    y = mapViewPos.y + mv.getHeight() - map.statusLine.getHeight() - size.height - margin;
-                } else {
-                    x = margin;
-                    y = parentWindow.getHeight() - MainApplication.getToolbar().control.getSize().height - size.height - margin;
-                }
                 parentWindow.getLayeredPane().add(currentNotificationPanel, JLayeredPane.POPUP_LAYER, 0);
-
-                currentNotificationPanel.setLocation(x, y);
-            }
-            currentNotificationPanel.setSize(size);
+                MainApplication.addMapFrameListener(mapFrameListener);
+                updateNotificationPosition();
+            }
             currentNotificationPanel.setVisible(true);
         });
 
-        running = true;
         elapsedTime = 0;
-
         startHideTimer();
+    }
+
+    /**
+     * Aligns the displayed notification to the map view, or to the content pane while no map view is displayed,
+     * and keeps listening to that anchor for as long as the notification is displayed.
+     */
+    private void updateNotificationPosition() {
+        JFrame parentWindow = MainApplication.getMainFrame();
+        if (currentNotificationPanel == null || parentWindow == null) {
+            return;
+        }
+        Component anchor = MainApplication.isDisplayingMapView() ? MainApplication.getMap().mapView : parentWindow.getContentPane();
+        if (anchor != notificationAnchor) {
+            // the map view is created and destroyed along with the layers, so the anchor may change while displaying
+            detachAnchorListener();
+            notificationAnchor = anchor;
+            anchor.addComponentListener(anchorListener);
+        }
+        // a map view that has just been created is not laid out yet; its first resize event brings the notification over
+        Component target = anchor.getHeight() > 0 ? anchor : parentWindow.getContentPane();
+        currentNotificationPanel.setLocation(getNotificationPosition(target, parentWindow.getLayeredPane(),
+                currentNotificationPanel.getSize(), GuiSizesHelper.getSizeDpiAdjusted(MARGIN)));
+    }
+
+    /**
+     * Stops listening to the anchor of the notification that is no longer displayed.
+     */
+    private void detachAnchorListener() {
+        if (notificationAnchor != null) {
+            notificationAnchor.removeComponentListener(anchorListener);
+            notificationAnchor = null;
+        }
+    }
+
+    /**
+     * Computes the position of a notification panel in the coordinate system of {@code container}, which unlike
+     * the one of the main window does not depend on the window decorations, i.e. on fullscreen mode.
+     * <p>
+     * The panel is aligned to the bottom left corner of {@code anchor}, or to its top left corner if it is taller,
+     * so that the beginning of a long message stays readable.
+     *
+     * @param anchor the component the notification is aligned to, e.g. the map view
+     * @param container the container the notification panel is added to
+     * @param size the size of the notification panel
+     * @param margin the margin to keep between the notification panel and the borders of {@code anchor}
+     * @return the location of the upper left corner of the notification panel
+     */
+    static Point getNotificationPosition(Component anchor, Container container, Dimension size, int margin) {
+        Rectangle bounds = SwingUtilities.convertRectangle(anchor.getParent(), anchor.getBounds(), container);
+        int y = Math.max(bounds.y + margin, bounds.y + bounds.height - size.height - margin);
+        return new Point(bounds.x + margin, y);
     }
 
@@ -170 +260 @@
     }
 
-    private void stopHideTimer() {
-        hideTimer.stop();
-        if (currentNotificationPanel != null) {
+    /**
+     * Hides the displayed notification and starts the pause before the next one.
+     *
+     * @param expected the notification to hide, or {@code null} for whichever is displayed. Nothing happens if
+     *                 another notification is displayed by now.
+     */
+    private void stopHideTimer(Notification expected) {
+        // may be reached from any thread through replaceExistingNotification()
+        GuiHelper.runInEDTAndWait(() -> {
+            if (currentNotificationPanel == null || (expected != null && !Objects.equals(expected, currentNotification))) {
+                return;
+            }
+            hideTimer.stop();
+            detachAnchorListener();
+            MainApplication.removeMapFrameListener(mapFrameListener);
             currentNotificationPanel.setVisible(false);
             JFrame parent = MainApplication.getMainFrame();
@@ -179 +281 @@
             }
             currentNotificationPanel = null;
-        }
-        pauseTimer.restart();
+            synchronized (queue) {
+                // forget it, or an identical notification shown during the pause is dropped as a duplicate
+                currentNotification = null;
+            }
+            pauseTimer.restart();
+        });
     }
 
@@ -189 +295 @@
             synchronized (queue) {
                 running = false;
-                processQueue();
-            }
+            }
+            processQueue();
         }
     }
@@ -198 +304 @@
         @Override
         public void actionPerformed(ActionEvent e) {
+            // AWT still delivers the mouse exit event of a panel that has just been removed
             if (currentNotificationPanel != null) {
                 currentNotificationPanel.setNotificationBackground(PANEL_SEMITRANSPARENT);
                 currentNotificationPanel.repaint();
-            }
-            startHideTimer();
+                startHideTimer();
+            }
         }
     }
@@ -236 +343 @@
 
         private void build(final Notification note, MouseListener freeze, ActionListener hideListener) {
+            // the default FlowLayout would add gaps around the visible notification
+            setLayout(new BorderLayout());
             JButton btnClose = new JButton();
             btnClose.addActionListener(hideListener);
             btnClose.setIcon(ImageProvider.get("misc", "grey_x"));
-            btnClose.setPreferredSize(new Dimension(50, 50));
+            btnClose.setPreferredSize(GuiSizesHelper.getDimensionDpiAdjusted(new Dimension(50, 50)));
             btnClose.setMargin(new Insets(0, 0, 1, 1));
             btnClose.setContentAreaFilled(false);
@@ -271 +380 @@
             layout.setAutoCreateContainerGaps(true);
 
-            innerPanel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));
-            add(innerPanel);
+            add(innerPanel, BorderLayout.CENTER);
 
             JLabel icon = null;
@@ -342 +450 @@
             if (unfreezeDelayTimer.isRunning()) {
                 unfreezeDelayTimer.stop();
-            } else {
+            } else if (currentNotificationPanel != null) {
+                // AWT still delivers events for a panel that has just been removed
                 hideTimer.stop();
                 elapsedTime += System.currentTimeMillis() - displayTimeStart;
@@ -368 +477 @@
         @Override
         protected void paintComponent(Graphics graphics) {
-            Graphics2D g = (Graphics2D) graphics;
-            g.setRenderingHint(
-                    RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);
-            g.setColor(getBackground());
-            float lineWidth = 1.4f;
-            Shape rect = new RoundRectangle2D.Double(
-                    lineWidth/2d + getInsets().left,
-                    lineWidth/2d + getInsets().top,
-                    getWidth() - lineWidth/2d - getInsets().left - getInsets().right,
-                    getHeight() - lineWidth/2d - getInsets().top - getInsets().bottom,
-                    20, 20);
-
-            g.fill(rect);
-            g.setColor(getForeground());
-            g.setStroke(new BasicStroke(lineWidth));
-            g.draw(rect);
+            // paint on a copy, so that neither the antialiasing hint nor the stroke leak into the given context
+            Graphics2D g = (Graphics2D) graphics.create();
+            try {
+                g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);
+                g.setColor(getBackground());
+                float lineWidth = 1.4f;
+                // the outline goes on the content box, not around the empty border
+                Insets insets = getInsets();
+                Shape rect = new RoundRectangle2D.Double(
+                        insets.left + lineWidth/2d,
+                        insets.top + lineWidth/2d,
+                        getWidth() - insets.left - insets.right - lineWidth,
+                        getHeight() - insets.top - insets.bottom - lineWidth,
+                        20, 20);
+
+                g.fill(rect);
+                g.setColor(getForeground());
+                g.setStroke(new BasicStroke(lineWidth));
+                g.draw(rect);
+            } finally {
+                g.dispose();
+            }
             super.paintComponent(graphics);
         }

trunk/src/org/openstreetmap/josm/tools/ImageProvider.java

@@ -183 +183 @@
          * @since 16872
          */
-        HTMLINLINE(24, 24);
+        HTMLINLINE(24, 24),
+        /**
+         * Notification icon maximum size
+         * @since 19643
+         */
+        NOTIFICATION(Config.getPref().getInt("iconsize.notification", 32));
 
         private final int virtualWidth;