Changeset 19085 in josm for trunk

Timestamp:

2024-05-22T21:12:24+02:00 (6 weeks ago)

Author:

taylor.smock

Message:

Fix #23687: Freeze when opening Add Tag dialog

This occurs when the following are true:

• The EDT is in a blocking dialog where a DataSet write lock is held
• The EDT decides to repaint the mapview
• The style caching occurs on a different thread

Why is this a problem?
The style caching waits for a read lock in a different thread, which will never
happen since the EDT has a write lock. The try-catch block that checks to see if
the current thread can get a read lock does not work on a thread that already has
a write lock, since that thread will always be able to obtain a read lock.

There are two solutions to this:

1. Change the style caching code such that it times out after some period of time and then retries in the EDT (to avoid recurrent timeouts)
2. Change TagEditHelper such that the blocking dialog is not in a block which holds a DataSet write lock

Option 2 is better, since it fixes the root cause; option 1 may need to be done
in the future, if only to allow for bug reports.

File:

• trunk/src/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelper.java (modified) (14 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelper.java

@@ -141 +141 @@
     /** The preference storage of recent tags */
     public static final ListProperty PROPERTY_RECENT_TAGS = new ListProperty("properties.recent-tags",
-            Collections.<String>emptyList());
+            Collections.emptyList());
     /** The preference list of tags which should not be remembered, since r9940 */
     public static final StringProperty PROPERTY_TAGS_TO_IGNORE = new StringProperty("properties.recent-tags.ignore",
@@ -181 +181 @@
     /**
      * Copy of recently added tags in sorted from newest to oldest order.
-     *
+     * <p>
      * We store the maximum number of recent tags to allow dynamic change of number of tags shown in the preferences.
      * Used to cache initial status.
@@ -278 +278 @@
         if (activeDataSet == null)
             return;
-        try {
-            activeDataSet.beginUpdate();
-            Collection<OsmPrimitive> selection = OsmDataManager.getInstance().getInProgressSelection();
-            this.sel = selection;
-            if (Utils.isEmpty(selection))
-                return;
-
-            final AddTagsDialog addDialog = getAddTagsDialog();
-
-            addDialog.showDialog();
-
-            addDialog.destroyActions();
+        final Collection<OsmPrimitive> selection = updateSelection();
+
+        if (Utils.isEmpty(selection))
+            return;
+
+        final AddTagsDialog addDialog = getAddTagsDialog();
+
+        addDialog.showDialog();
+
+        addDialog.destroyActions();
+        activeDataSet.update(() -> {
             // Remote control can cause the selection to change, see #23191.
-            if (addDialog.getValue() == 1 && (selection == sel || warnSelectionChanged())) {
+            if (addDialog.getValue() == 1 && (selection.equals(updateSelection()) || warnSelectionChanged())) {
                 addDialog.performTagAdding(selection);
             } else {
                 addDialog.undoAllTagsAdding();
             }
-        } finally {
-            activeDataSet.endUpdate();
-        }
+        });
     }
 
@@ -317 +314 @@
     public void editTag(final int row, boolean focusOnKey) {
         changedKey = null;
-        sel = OsmDataManager.getInstance().getInProgressSelection();
+        updateSelection();
         if (Utils.isEmpty(sel))
             return;
@@ -361 +358 @@
 
     /**
+     * Update the current selection for this editor
+     */
+    private Collection<OsmPrimitive> updateSelection() {
+        final DataSet activeDataSet = OsmDataManager.getInstance().getActiveDataSet();
+        try {
+            activeDataSet.getReadLock().lock();
+            Collection<OsmPrimitive> selection = new ArrayList<>(OsmDataManager.getInstance().getInProgressSelection());
+            this.sel = selection;
+            return selection;
+        } finally {
+            activeDataSet.getReadLock().unlock();
+        }
+    }
+
+    /**
      * For a given key k, return a list of keys which are used as keys for
      * auto-completing values to increase the search space.
@@ -370 +382 @@
             return Arrays.asList("addr:street", "name");
         else
-            return Arrays.asList(key);
+            return Collections.singletonList(key);
     }
 
@@ -552 +564 @@
             p.add(new JLabel(tr("Key")), GBC.std());
             p.add(Box.createHorizontalStrut(10), GBC.std());
-            p.add(keys, GBC.eol().fill(GBC.HORIZONTAL));
+            p.add(keys, GBC.eol().fill(GridBagConstraints.HORIZONTAL));
 
             List<AutoCompletionItem> valueList = autocomplete.getTagValues(getAutocompletionKeys(key), usedValuesAwareComparator);
@@ -569 +581 @@
             p.add(new JLabel(tr("Value")), GBC.std());
             p.add(Box.createHorizontalStrut(10), GBC.std());
-            p.add(values, GBC.eol().fill(GBC.HORIZONTAL));
+            p.add(values, GBC.eol().fill(GridBagConstraints.HORIZONTAL));
             p.add(Box.createVerticalStrut(2), GBC.eol());
 
@@ -829 +841 @@
             mainPanel.add(new JLabel("<html>"+trn("This will change up to {0} object.",
                 "This will change up to {0} objects.", sel.size(), sel.size())
-                +"<br><br>"+tr("Please select a key")), GBC.eol().fill(GBC.HORIZONTAL));
+                +"<br><br>"+tr("Please select a key")), GBC.eol().fill(GridBagConstraints.HORIZONTAL));
 
             keys = new AutoCompComboBox<>();
@@ -837 +849 @@
             keys.setAutocompleteEnabled(AUTOCOMPLETE_KEYS.get());
 
-            mainPanel.add(keys, GBC.eop().fill(GBC.HORIZONTAL));
+            mainPanel.add(keys, GBC.eop().fill(GridBagConstraints.HORIZONTAL));
             mainPanel.add(new JLabel(tr("Choose a value")), GBC.eol());
 
@@ -846 +858 @@
             values.setAutocompleteEnabled(AUTOCOMPLETE_VALUES.get());
 
-            mainPanel.add(values, GBC.eop().fill(GBC.HORIZONTAL));
+            mainPanel.add(values, GBC.eop().fill(GridBagConstraints.HORIZONTAL));
 
             cacheRecentTags();
@@ -978 +990 @@
                     lines.add(sc.getKeyText() + ' ' + tr("to apply first suggestion"))
             );
-            lines.add(Shortcut.getKeyText(KeyStroke.getKeyStroke(KeyEvent.VK_ENTER, KeyEvent.SHIFT_DOWN_MASK)) + ' '
+            lines.add(Shortcut.getKeyText(KeyStroke.getKeyStroke(KeyEvent.VK_ENTER, InputEvent.SHIFT_DOWN_MASK)) + ' '
                     +tr("to add without closing the dialog"));
-            Shortcut.findShortcut(KeyEvent.VK_1, commandDownMask | KeyEvent.SHIFT_DOWN_MASK).ifPresent(sc ->
+            Shortcut.findShortcut(KeyEvent.VK_1, commandDownMask | InputEvent.SHIFT_DOWN_MASK).ifPresent(sc ->
                     lines.add(sc.getKeyText() + ' ' + tr("to add first suggestion without closing the dialog"))
             );
@@ -1013 +1025 @@
                 recentTagsPanel = new JPanel(new GridBagLayout());
                 buildRecentTagsPanel();
-                mainPanel.add(recentTagsPanel, GBC.eol().fill(GBC.HORIZONTAL));
+                mainPanel.add(recentTagsPanel, GBC.eol().fill(GridBagConstraints.HORIZONTAL));
             } else {
                 Dimension panelOldSize = recentTagsPanel.getPreferredSize();
@@ -1132 +1144 @@
                 JPanel tagPanel = new JPanel(new FlowLayout(FlowLayout.LEFT, 0, 0));
                 tagPanel.add(tagLabel);
-                recentTagsPanel.add(tagPanel, GBC.eol().fill(GBC.HORIZONTAL));
+                recentTagsPanel.add(tagPanel, GBC.eol().fill(GridBagConstraints.HORIZONTAL));
             }
             // Clear label if no tags were added