Changeset 9334 in josm for trunk

Timestamp:

2016-01-07T01:53:58+01:00 (9 years ago)

Author:

Don-vip

Message:

see #12282 - handle warnings for mappaint styles:

• Log warnings that occur when loading style
• Display them in Mappaint dialog (Info action)
• Drop redundant error icon
• Ignore XML styles in styles validation test
• Make styles validation test fail if at least a warning occur

Location:

trunk

Files:

• images/dialogs/mappaint/error_small.png (deleted)
• src/org/openstreetmap/josm/gui/dialogs/MapPaintDialog.java (modified) (9 diffs)
• src/org/openstreetmap/josm/gui/mappaint/MapPaintStyles.java (modified) (4 diffs)
• src/org/openstreetmap/josm/gui/mappaint/StyleSource.java (modified) (6 diffs)
• src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/mappaint/styleelement/MapImage.java (modified) (2 diffs)
• src/org/openstreetmap/josm/gui/preferences/SourceEditor.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImageEntryTest.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/gui/preferences/map/MapPaintPreferenceTest.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/MapPaintDialog.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.dialogs;
 
+import static org.openstreetmap.josm.tools.I18n.marktr;
 import static org.openstreetmap.josm.tools.I18n.tr;
 
@@ -26 +27 @@
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.List;
 
@@ -31 +33 @@
 import javax.swing.DefaultButtonModel;
 import javax.swing.DefaultListSelectionModel;
+import javax.swing.ImageIcon;
 import javax.swing.JCheckBox;
 import javax.swing.JFileChooser;
@@ -544 +547 @@
 
         private boolean errorsTabLoaded;
+        private boolean warningsTabLoaded;
         private boolean sourceTabLoaded;
 
@@ -573 +577 @@
             tabs.setTabComponentAt(0, lblInfo);
 
-            final JPanel pErrors = new JPanel(new GridBagLayout());
-            tabs.add("Errors", pErrors);
-            JLabel lblErrors;
-            if (s.getErrors().isEmpty()) {
-                lblErrors = new JLabel(tr("Errors"));
-                lblErrors.setFont(lblInfo.getFont().deriveFont(Font.PLAIN));
-                lblErrors.setEnabled(false);
-                tabs.setTabComponentAt(1, lblErrors);
-                tabs.setEnabledAt(1, false);
-            } else {
-                lblErrors = new JLabel(tr("Errors"), ImageProvider.get("misc", "error"), JLabel.HORIZONTAL);
-                tabs.setTabComponentAt(1, lblErrors);
-            }
+            final JPanel pErrors = addErrorOrWarningTab(tabs, lblInfo,
+                    s.getErrors(), marktr("Errors"), 1, ImageProvider.get("misc", "error"));
+            final JPanel pWarnings = addErrorOrWarningTab(tabs, lblInfo,
+                    s.getWarnings(), marktr("Warnings"), 2, ImageProvider.get("warning-small"));
 
             final JPanel pSource = new JPanel(new GridBagLayout());
@@ -591 +586 @@
             JLabel lblSource = new JLabel(tr("Source"));
             lblSource.setFont(lblSource.getFont().deriveFont(Font.PLAIN));
-            tabs.setTabComponentAt(2, lblSource);
+            tabs.setTabComponentAt(3, lblSource);
 
             tabs.getModel().addChangeListener(new ChangeListener() {
@@ -598 +593 @@
                     if (!errorsTabLoaded && ((SingleSelectionModel) e.getSource()).getSelectedIndex() == 1) {
                         errorsTabLoaded = true;
-                        buildErrorsPanel(s, pErrors);
+                        buildErrorsOrWarningPanel(s.getErrors(), pErrors);
                     }
-                    if (!sourceTabLoaded && ((SingleSelectionModel) e.getSource()).getSelectedIndex() == 2) {
+                    if (!warningsTabLoaded && ((SingleSelectionModel) e.getSource()).getSelectedIndex() == 2) {
+                        warningsTabLoaded = true;
+                        buildErrorsOrWarningPanel(s.getWarnings(), pWarnings);
+                    }
+                    if (!sourceTabLoaded && ((SingleSelectionModel) e.getSource()).getSelectedIndex() == 3) {
                         sourceTabLoaded = true;
                         buildSourcePanel(s, pSource);
@@ -608 +607 @@
             info.setContent(tabs, false);
             info.showDialog();
+        }
+
+        private JPanel addErrorOrWarningTab(final JTabbedPane tabs, JLabel lblInfo,
+                Collection<?> items, String title, int pos, ImageIcon icon) {
+            final JPanel pErrors = new JPanel(new GridBagLayout());
+            tabs.add(title, pErrors);
+            if (items.isEmpty()) {
+                JLabel lblErrors = new JLabel(tr(title));
+                lblErrors.setFont(lblInfo.getFont().deriveFont(Font.PLAIN));
+                lblErrors.setEnabled(false);
+                tabs.setTabComponentAt(pos, lblErrors);
+                tabs.setEnabledAt(pos, false);
+            } else {
+                JLabel lblErrors = new JLabel(tr(title), icon, JLabel.HORIZONTAL);
+                tabs.setTabComponentAt(pos, lblErrors);
+            }
+            return pErrors;
         }
 
@@ -658 +674 @@
         }
 
-        private void buildErrorsPanel(StyleSource s, JPanel p) {
+        private <T> void buildErrorsOrWarningPanel(Collection<T> items, JPanel p) {
             JosmTextArea txtErrors = new JosmTextArea();
             txtErrors.setFont(GuiHelper.getMonospacedFont(txtErrors));
             txtErrors.setEditable(false);
             p.add(new JScrollPane(txtErrors), GBC.std().fill());
-            for (Throwable t : s.getErrors()) {
+            for (T t : items) {
                 txtErrors.append(t + "\n");
             }

trunk/src/org/openstreetmap/josm/gui/mappaint/MapPaintStyles.java

@@ -11 +11 @@
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -270 +269 @@
             }
         }
-        if (Main.isDebugEnabled() || !source.getErrors().isEmpty()) {
+        if (Main.isDebugEnabled() || !source.isValid()) {
             final long elapsedTime = System.currentTimeMillis() - startTime;
             String message = "Initializing map style " + source.url + " completed in " + Utils.getDurationString(elapsedTime);
-            if (!source.getErrors().isEmpty()) {
-                Main.warn(message + " (" + source.getErrors().size() + " errors)");
+            if (!source.isValid()) {
+                Main.warn(message + " (" + source.getErrors().size() + " errors, " + source.getWarnings().size() + " warnings)");
             } else {
                 Main.debug(message);
@@ -445 +444 @@
      * Add a new map paint style.
      * @param entry map paint style
-     * @return list of errors that occured during loading
-     */
-    public static Collection<Throwable> addStyle(SourceEntry entry) {
+     * @return loaded style source, or {@code null}
+     */
+    public static StyleSource addStyle(SourceEntry entry) {
         StyleSource source = fromSourceEntry(entry);
         if (source != null) {
@@ -458 +457 @@
                 Main.map.mapView.repaint();
             }
-            return source.getErrors();
-        }
-        return Collections.emptyList();
+        }
+        return source;
     }
 

trunk/src/org/openstreetmap/josm/gui/mappaint/StyleSource.java

@@ -14 +14 @@
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CopyOnWriteArraySet;
 
 import javax.swing.ImageIcon;
@@ -33 +36 @@
 public abstract class StyleSource extends SourceEntry {
 
-    private final List<Throwable> errors = new ArrayList<>();
+    private final List<Throwable> errors = new CopyOnWriteArrayList<>();
+    private final Set<String> warnings = new CopyOnWriteArraySet<>();
     public File zipIcons;
 
@@ -42 +46 @@
     private static ImageProvider defaultIconProvider;
 
-    /******
-     * The following fields is additional information found in the header
-     * of the source file.
+    /**
+     * The following fields is additional information found in the header of the source file.
      */
     public String icon;
@@ -121 +124 @@
     }
 
+    /**
+     * Log an error that occured with this style.
+     * @param e error
+     */
     public void logError(Throwable e) {
         errors.add(e);
     }
 
+    /**
+     * Log a warning that occured with this style.
+     * @param w warnings
+     */
+    public void logWarning(String w) {
+        warnings.add(w);
+    }
+
+    /**
+     * Replies the collection of errors that occured with this style.
+     * @return collection of errors
+     */
     public Collection<Throwable> getErrors() {
         return Collections.unmodifiableCollection(errors);
+    }
+
+    /**
+     * Replies the collection of warnings that occured with this style.
+     * @return collection of warnings
+     */
+    public Collection<String> getWarnings() {
+        return Collections.unmodifiableCollection(warnings);
+    }
+
+    /**
+     * Determines if this style is valid (no error, no warning).
+     * @return {@code true} if this style has 0 errors and 0 warnings
+     */
+    public boolean isValid() {
+        return errors.isEmpty() && warnings.isEmpty();
     }
 
@@ -180 +215 @@
         ImageProvider i = getSourceIconProvider();
         if (!getErrors().isEmpty()) {
-            i = new ImageProvider(i).addOverlay(new ImageOverlay(new ImageProvider("dialogs/mappaint/error_small")));
+            i = new ImageProvider(i).addOverlay(new ImageOverlay(new ImageProvider("misc", "error"), 0.5, 0.5, 1, 1));
+        } else if (!getWarnings().isEmpty()) {
+            i = new ImageProvider(i).addOverlay(new ImageOverlay(new ImageProvider("warning-small"), 0.5, 0.5, 1, 1));
         }
         return i;
@@ -200 +237 @@
      */
     public String getToolTipText() {
-        if (errors.isEmpty())
+        if (errors.isEmpty() && warnings.isEmpty())
             return null;
-        else
-            return trn("There was an error when loading this style. Select ''Info'' from the right click menu for details.",
-                    "There were {0} errors when loading this style. Select ''Info'' from the right click menu for details.",
-                    errors.size(), errors.size());
+        int n = errors.size() + warnings.size();
+        return trn("There was an error when loading this style. Select ''Info'' from the right click menu for details.",
+                "There were {0} errors when loading this style. Select ''Info'' from the right click menu for details.",
+                n, n);
     }
 

trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java

@@ -535 +535 @@
             backgroundColorOverride = c.get("background-color", null, Color.class);
             if (backgroundColorOverride != null) {
-                Main.warn(tr("Detected deprecated ''{0}'' in ''{1}'' which will be removed shortly. Use ''{2}'' instead.",
-                        "canvas{background-color}", url, "fill-color"));
+                String msg = tr("Detected deprecated ''{0}'' in ''{1}'' which will be removed shortly. Use ''{2}'' instead.",
+                        "canvas{background-color}", url, "fill-color");
+                logWarning(msg);
+                Main.warn(msg);
             }
         }

trunk/src/org/openstreetmap/josm/gui/mappaint/styleelement/MapImage.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.gui.mappaint.styleelement;
+
+import static org.openstreetmap.josm.tools.I18n.tr;
 
 import java.awt.Graphics;
@@ -102 +104 @@
                         synchronized (MapImage.this) {
                             if (result == null) {
+                                source.logWarning(tr("Failed to locate image ''{0}''", name));
                                 ImageIcon noIcon = MapPaintStyles.getNoIcon_Icon(source);
                                 img = noIcon == null ? null : (BufferedImage) noIcon.getImage();

trunk/src/org/openstreetmap/josm/gui/preferences/SourceEditor.java

@@ -701 +701 @@
         public String link;
         public String description;
+        /** Style type: can only have one value: "xml". Used to filter out old XML styles. For MapCSS styles, the value is not set. */
+        public String styleType;
         public Integer minJosmVersion;
 
@@ -1418 +1420 @@
                                     }
                                 }
+                            } else if ("style-type".equals(key)) {
+                                last.styleType = value;
                             }
                         }

trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImageEntryTest.java

@@ -5 +5 @@
 
 import java.io.File;
-import java.io.IOException;
 
 import org.junit.Test;
 import org.openstreetmap.josm.TestUtils;
-import org.openstreetmap.josm.io.IllegalDataException;
-import org.xml.sax.SAXException;
 
 /**
@@ -19 +16 @@
     /**
      * Non-regression test for ticket <a href="https://josm.openstreetmap.de/ticket/12255">#12255</a>.
-     * @throws IllegalDataException if an error was found while parsing the data from the source
-     * @throws IOException if any I/O error occurs
-     * @throws SAXException if any XML error occurs
      */
     @Test
-    public void testTicket12255() throws IllegalDataException, IOException, SAXException {
+    public void testTicket12255() {
         ImageEntry e = new ImageEntry(new File(TestUtils.getRegressionDataFile(12255, "G0016941.JPG")));
         e.extractExif();

trunk/test/unit/org/openstreetmap/josm/gui/preferences/map/MapPaintPreferenceTest.java

@@ -14 +14 @@
 import org.openstreetmap.josm.JOSMFixture;
 import org.openstreetmap.josm.gui.mappaint.MapPaintStyles;
+import org.openstreetmap.josm.gui.mappaint.StyleSource;
 import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.ParseException;
 import org.openstreetmap.josm.gui.preferences.SourceEditor.ExtendedSourceEntry;
@@ -41 +42 @@
         assertFalse(sources.isEmpty());
         Map<String, Collection<Throwable>> allErrors = new HashMap<>();
+        Map<String, Collection<String>> allWarnings = new HashMap<>();
         for (ExtendedSourceEntry source : sources) {
-            System.out.println(source.url);
-            Collection<Throwable> errors = MapPaintStyles.addStyle(source);
-            System.out.println(errors.isEmpty() ? " => OK" : " => KO");
-            if (!errors.isEmpty()) {
-                allErrors.put(source.url, errors);
+            // Do not validate XML styles
+            if (!"xml".equalsIgnoreCase(source.styleType)) {
+                System.out.println(source.url);
+                StyleSource style = MapPaintStyles.addStyle(source);
+                System.out.println(style.isValid() ? " => OK" : " => KO");
+                Collection<Throwable> errors = style.getErrors();
+                Collection<String> warnings = style.getWarnings();
+                if (!errors.isEmpty()) {
+                    allErrors.put(source.url, errors);
+                }
+                if (!warnings.isEmpty()) {
+                    allWarnings.put(source.url, warnings);
+                }
             }
         }
-        assertTrue(allErrors.toString(), allErrors.isEmpty());
+        assertTrue(allErrors.toString()+"\n"+allWarnings.toString(), allErrors.isEmpty() && allWarnings.isEmpty());
     }
 }