[PATCH] There should be a method to clear ignored errors

[taylor.smock]

In addition to the patch, copy the fix.png to images.
cp ./bin/images/dialogs/fix.png ./images/

I've had some people ask me why they don't see anything with the validator. Often, its due to silly things like an ignore list.

So, I would like to have a method to clear the ignore list without telling them to go to the application's preference location.

[GerdP]

I agree that we need a way to reset this list. I tried the patch but it doesn't seem to work.
Without looking at the patch in detail I expected a new button for the action somewhere in the validator panel. So I started an edit session and open the validator dialog. Since there was no new button I searched for it via help. Found it and selected it.
This removes the file ignorederrors, but it doesn't reset the values stored in the validator. So, when I click ignore for another error the file contained again all previously ignored error codes.
Besides that the dialog should wait for an OK before removing the file and I'd prefer to be able to restore it easily.

[taylor.smock]

OK.

To restore the file, should I rename it to ignorederrors.bak? Also, should I mark it as something to be deleted on exit (e.g. ignoredErrors.deleteOnExit()).

I didn't know that the validator stored the ignorederrors list in memory. I'll look into clearing that (I was just checking to see if the file was removed -- I assumed that the validator read from the file each time it was run).

It sounds like you think the reset button should either be in the Validation Results panel or the Data validator preferences pane. I'm not certain which one you were referring to. I would probably put it in the Data validator pane, but I'm not certain where the best location for it would be (I put it under Help initially since I assumed that would be the best place for it). Until I have a better place to put it, I'm going to leave it under Help.

Thank you for telling me that people might accidentally click a button and not want to do the action. I assumed that it would be an intentional action. I'll add a confirmation dialog.

TLDR: I learned what assumption really meant. Again.

[GerdP]

I also thought about adding such a dialog. My use case would be this:
You have executed validator and wonder why an expected error message doesn't appear. In that case it would be great to be able to see the list of errors without the ignorederrors (ignoring the list), maybe executing validator again.
I think the problem with the current file content is that it requires deep knowledge about the source code to be able to understand a list like mine:

3000_*[wood=deciduous]
3000_way[highway=unclassified][!name][noname'NEQ'yes]
303
614

While the upper two are quite self-explaining the other two need some kind of comment. For example

303 /* Unnamed way */
614 /* Crossing building/residential area */

would be easier to understand. With those comments it would be possible to generate a small dialog that could allow to enable/disable each entry with a checkbox.

[taylor.smock]

Add additional dialogue options to reset/restore (doesn't ask for confirmation to reset)

[taylor.smock]

Move the buttons and logic to the Validation Results pane. It (still) does not ask for confirmation, but I don't think it is necessary at this point (it toggles between save/restore until a new ignore is added).

[GerdP]

I think that's already much better :)
A few problems:
1) Crash: Image copy is not in dialogs
2) Crash if there is a directory validator with neither ignorederrors nor ignorederrors.bak or when ignorederrors is empty:

2019-01-29 07:39:59.765 SEVERE: Handled by bug report queue: java.lang.IllegalArgumentException: No icon provided
java.lang.IllegalArgumentException: No icon provided
	at org.openstreetmap.josm.gui.SideButton.<init>(SideButton.java:43)
	at org.openstreetmap.josm.gui.dialogs.ValidatorDialog.<init>(ValidatorDialog.java:186)

3) Please don't System.out.println(..), use e.g. class Logging instead and show a popup with e.g. JOptionPane.showMessageDialog()
4) The file name "ignorederrors" should be stored in a final static field
5) I am searching for a better button text. "Reset" is misleading, I would expect that this restores a previous version of the list. I think we "Clear" something here. So, maybe "Clear ignore" and "Restore ignore" ?
6) It would be great when the button would also change the displayed tree in case that objects were filtered.

General remark: Keep in mind that experienced users know where the ignorederrors file is and that they might rename/remove that file
even while JOSM is running.

[taylor.smock]

Change copy image to not use the dialogs directroy, hopefully fix the crash when ignorederrors files are not present, remove System.out.println() (I was using these as debug statements for "You are here"), change Reset to Clear, and (hopefully) change the displayed tree when switching.

[GerdP]

Thanks. The displayed tree doesn't change yet. This needs some more logic because the field Testerror.ignored has to be re-calculated This would be quite complex, as the field is also used to hide "fixed" errors.
A probably better solution might be to show a popup that the validator has to be executed again to see the effect.

BTW: You seem to prefer to use if list.size() > 0 instead of !list.isEmpty(). Why?

[taylor.smock]

OK. I used list.size() > 0 since I didn't know that it had an .isEmpty() method (I pretty much default to only assuming that a length or size method is implemented). I'll upload a patch for that in a minute. I don't know if I want to have a popup asking if the validation errors should be recalculated (I would want to have the option to remember the choice, which I don't think is an option, I'll have to do some digging).

If it helps, I have done most of my "programming" in bash, so I know that some of what I do in java is not necessarily the best thing to do.

[taylor.smock]

Switch from size() > 0 to !<var>.isEmpty() and remove some tree lines.

[taylor.smock]

Ask if validation should be rerun (only does selection)

[GerdP]

Close to perfect in my eyes. The popup needs a bit cosmetics. With german translation it looks like this:

The text "Should the validator be rerun" is too small, the icon with the question mark is somehow hidden, the header "ignorederros list changed" is expecting that the user knows what this file name means.
Maybe "Ignored error filter changed"?

[taylor.smock]

I've changed the ignorederrors list changed to your suggestion (Ignored error filter changed).

I have no clue why there is a question mark icon (I'm seeing the JOSM icon). I'm not setting an icon for the window, so that might be why.

I have no clue how to fix the text size (I can use <h1> but not <h2> or <h3> (neither of the latter actually change the text size), and I think <h1> makes the text far too large.

I've also added a check so that the window doesn't pop up and cause a potential error when there is no real data. That will also be in the next patch I upload.

[taylor.smock]

Add check to avoid crash when there is no osm data and change the name of the question window

[GerdP]

Hmm, thought about the problem again. I don't like all the IO operations in ValidatorDialog. My first idea was to move the two methods to OSMValidator but now I wonder why we do not use the preferences.xml to store the content of OSMValidator.ignoredErrors. It seems the right place to use for me.

@all:
If I got that right the only reason for this special file is that the validator code was in a plugin many years ago (see r3669)?

[GerdP]

There is probably another conflict here :(
We have a preference validator.ignore, see ValidatorPrefHelper.PREF_USE_IGNORE. Somehow the patch works around this or maybe implements a parallel control flow.

[stoecker]

Replying to GerdP:

Hmm, thought about the problem again. I don't like all the IO operations in ValidatorDialog. My first idea was to move the two methods to OSMValidator but now I wonder why we do not use the preferences.xml to store the content of OSMValidator.ignoredErrors. It seems the right place to use for me.

I think so.

@all:
If I got that right the only reason for this special file is that the validator code was in a plugin many years ago (see r3669)?

No. Mainly that our preferences file wasn't so sophisticated at that time. Storing in individual files was state-of-the-art then.

[taylor.smock]

I wasn't intentionally working around ValidatorPrefHelper.PREF_USE_IGNORE. I'll modify my code to check for that and I'll move the IO operations to OSMValidator. This will also make it easier for a future refactor to use the preferences file (however that will be done) since they will only need to modify one file.

As far as storing the ignorelist in the preferences file, I don't know how to go about that right now, and that should probably be a different bug. Especially since I think it will be a lot more involved, and I don't think I am the right person (yet) to do something like that.

Something that I noticed during testing is that when the PREF_USE_IGNORE is toggled, it appears to need a restart of JOSM.

[taylor.smock]

Most File operations moved to OsmValidator.java and the clear/restore/save ignore list button no longer shows up when the ValidatorPrefHelper.PREF_USE_IGNORE is set (under the same conditions as the Ignore button).

[taylor.smock]

Fix an issue (I was copying between the wrong preference keys)

[taylor.smock]

I removed (most) file operations and started converting the ignorederrors file to a preference key (validator.ignorederrors).
This permanently deletes the ignorederrors file it reads from!

The section that reads from the ignorederrors file should probably remain in place for at least a few months.

[GerdP]

I think the popup "Should the validation be rerun?" should only appear when the validator panel actually was executed before.
Not sure if that is easy to determine? In my configuration the validator panel is always open. Maybe that is unusual?

[anonymous]

It looks like I can do something like the following:

    /**
     * Prompt to rerun the validator when the ignore list changes
     */
    public void rerunValidatorPrompt() {
        MapFrame map = MainApplication.getMap();
        List<TestError> errors = map.validatorDialog.tree.getErrors();
        if (!validateAction.isEnabled() || errors == null || errors.isEmpty()) return;
        final int answer = ConditionalOptionPaneUtil.showOptionDialog(
                "rerun_validation_when_ignorelist_changed",
                MainApplication.getMainFrame(),
                "<hmtl><h3>" + tr("Should the validation be rerun?") + "</h3></html>",
                tr("Ignored error filter changed"),
                JOptionPane.YES_NO_CANCEL_OPTION,
                JOptionPane.QUESTION_MESSAGE,
                null,
                null);
        if (answer == JOptionPane.YES_OPTION) {
            validateAction.doValidate(true);
        }
    }

It probably won't run when there are no errors (e.g., when the ignorelist is full), but at least it won't prompt to run if the validator hasn't been run.

I'll upload the patch in a minute.

[taylor.smock]

Add a check to see if the validator has been run (does not detect when the error list has been cleared, and assumes that the validator has not been run in that case).

[GerdP]

OK, I'll give it a try tomorrow. What about this line?

//this.setEnabled(false); // TODO enable when I figure out how to call from ignore

[taylor.smock]

I was looking at changing the button text (e.g., calling toggle()) when the ignore button was pressed, so that Save Ignore appeared as the button text.

I think I would have to do some somewhat invasive edits to the Ignore button before I could call toggle() from it.

The intent was to disable the button when nothing could be done, and enable it when something could be done.

Example:
Initial start with no information in the ignorelist

The button should be disabled (since it shouldn't do anything)

Ignore an error

The button should be enabled and say Save Ignore

At this point, everything works, but from a UI standpoint, I think it would be best if I could call toggle() from the Ignore button.

[taylor.smock]

Default to disabled if nothing can be done, add some functionality to the ignoreErrors to call the resetignorelistButton.

[GerdP]

Some observations:

• I wonder why you don't use ValidatorPrefHelper to store the new settings?
• The Cancel button in the "Should the validation be rerun" popup seems to have no effect with v11?
• I don't understand the need for the *.bak list. I think you need a list and a (new) flag that says if it is in use or not. I see the new button (Clear/Restore) as a toggle for this flag, and of course the flag should be evaluated in methods like OsmValidator.hasIgnoredError(String s)? Since we already have the preference validator.ignore I think validator.ignorelist.active would be OK.

That said I still search for better button texts. Maybe "Enable/Disable ignore"? With button tooltip "Use ignore list to filter Validation results" and "Don't use ..."
Once this is completed we have to make sure that the help explains the difference between the validator.ignore switch used in the preferences dialog and this new button.

[taylor.smock]

1) I didn't think to use ValidatorPrefHelper, since I assumed that it didn't have a method to store lists. I'll modify it to have a variable for the ignorederrors and ignorederrors.bak.
2) I'll double check that. It should only be running when JOptionPane.YES_OPTION is selected (this is the only section of code that I have that would rerun validation):

if (answer == JOptionPane.YES_OPTION) {
    validateAction.doValidate(true);
}

3) I was creating the .bak list explicitly to clear the ignorelist while still being able to undo clearing the ignorelist. I suppose I can check for a key on startup and then completely remove the ignorelist, e.g.

if (ValidatorPrefHelper.PREF_RESETIGNORE.get()) {
    ValidatorPrefHelper.PREF_IGNORELIST.put(null);
    ValidatorPrefHelper.PREF_RESETIGNORE.put(false);
}

I would have to do something similar when the Ignore button is clicked after the ignorelist is reset.
I was initially writing this for people to clear their ignorelists without having to do anything complicated, i.e., Joe User selects Ignore on a group and then ignores the whole group accidentally. Sure, this is a nuclear option, but that was (somewhat) deliberate.

You are probably right that there are better button texts. But we should probably decide what it actually does before looking for better button texts. Will it clear the ignorelist? Or will it just hide the ignorelist?

[taylor.smock]

Use ValidatorPrefHelper for preferences, Cancel appears to work properly in the rerunValidator dialogue, still uses a .bak preference file

[taylor.smock]

Use ValidatorPrefHelper for preferences, Cancel appears to work properly in the rerunValidator dialogue, still uses a .bak preference key, and fixes a crash when the ignorelist is empty.

[GerdP]

Replying to taylor.smock:

2) I'll double check that. It should only be running when JOptionPane.YES_OPTION is selected (this is the only section of code that I have that would rerun validation):

if (answer == JOptionPane.YES_OPTION) {
    validateAction.doValidate(true);
}

I meant the Cancel button doesn't cancel the action connected to the button. The toggle button should remain as before and no rerun be executed.

You are probably right that there are better button texts. But we should probably decide what it actually does before looking for better button texts. Will it clear the ignorelist? Or will it just hide the ignorelist?

Since with v11 the "cleared" list still exists (in *.bak) and JOSM allows to restore it after a restart I think 'hide' describes it better. I like to be able to disable (or hide) the ignore list without completely removing (clearing) it. No idea how many other users see it that way.

[taylor.smock]

OK. I get what you are saying about the Cancel button and what the button in the panel does.

I wasn't intentionally keeping the cleared list indefinitely, I just didn't have anything implemented to clear/remove it.

Also, I've got a bunch of java.lang.NullPointerException ever since I switched to the ValidatorPrefHelper methods. I'm not certain how to work around those yet besides adding a bunch of != null checks. (I discovered this after I cleared the ignorelist and restarted JOSM).

I'll modify it so that Cancel works as people would expect.

[taylor.smock]

Now follows the expected behavior when clicking cancel in the dialogue window, clears ignorederrors.bak on startup. The preference values now default to empty arrays instead of null (so I don't have to add null checks all over the place).

[taylor.smock]

Add a pref for whether or not the ignorederror.bak list is cleared on startup (defaults to cleared, advanced users can change it by changing validator.ignorelist.bak.keep to true)

[GerdP]

• no need to sort , fix i18n coding

[GerdP]

I'd still prefer to have only one list and a flag.
At the moment, after some experimenting, I see quite a lot of preference settings when I search for 'validator.ignore':

validator.ignore	true
validator.ignorederrors	null
validator.ignorederrors.bak	null
validator.ignorelist	[3000_*[amenity=place_of_worship][!religion]:w_494800356]
validator.ignorelist.bak	[]
validator.ignorelist.bak.keep	false

Are they all in needed?
The longer I think about this the more I come to the conclusion that we need an editor showing the list and that we should change the code so that we don't just see simple, meaningless numbers. No idea how much work this is. With such an editor the button would be either obsolete or could be a "Edit Ignores".
There are more problems in the validator dialog, e.g. a right click on the title bar always shows all buttons enabled, the popup that appears after clicking "Ignore" asks "Ignore whole group or individual elements?" and I am always unsure what to select :(
For example: When I select the group "warnings" and click on Ignore. What will that do?
Only after looking at the effect on the ignore list I learned that "elements" means OSM elements" and not "all different warnings"
Let me know if you want to dig into this, else I might try myself.

[taylor.smock]

I don't think that

validator.ignorederrors
validator.ignorederrors.bak

are created anymore. On a clean profile.xml, they shouldn't be appearing.

As far as validator.ignore, that is needed (it determines whether or not the ignorelist is used), and so are the validator.ignorelist/validator.ignorelist.bak (for clearing/restoring the ignorelist), and validator.ignorelist.bak.keep controls whether or not the .bak preference is saved after restarting JOSM.

I have changed the code to use Config.getPref().putListOfMaps(), and modified the OsmValidator.ignoredErrors to be a HashMap<String, String>.

I also overloaded the method to add the error, so a description can be added.

For now, I think I'll modify the button to be Ignore Settings, create a popup that lists all of the errors (and their descriptions, if stored) and then have (for now) Cancel, Clear, Restore, and OK.

The Clear and Restore will do what they currently do, while OK will exist for when someone else implements a selection method.

I'm working on the patch now, but I'm running into issues getting a dialog to popup when the button is selected.

[GerdP]

Please double check the null values. I think I cleaned my preferences before. Can't test right now.

[taylor.smock]

Change to a pop-up window that asks for "OK", "Reset", "Clear", and "Cancel". The ignorelist can be directly modified in this pane, and the method to add errors is now overloaded with the option to add a description.

[GerdP]

isBlank() -> isEmpty(), add description in ValidatorDialog

[GerdP]

We are getting better and better :)
I couldn't compile v16:

compile:
    [javac] Compiling 126 source files to C:\josm\core\build
    [javac] C:\josm\core\src\org\openstreetmap\josm\gui\dialogs\IgnoreListManagementDialog.java:154: error: cannot find symbol
    [javac]             if (map.get(key) != null && !map.get(key).isBlank()) {
    [javac]                                                      ^
    [javac]   symbol:   method isBlank()
    [javac]   location: class String

I changed isBlank() to isEmpty() for testing.
You also seem to have a special (smaller) version of the copy icon in the dialogs directory?

I've attached v17 which contains above change and uses the existing copy.svg for now.
I've also added code so that OsmValidator.addIgnoredError() is always called with a description.

I am not sure if we get problems when you show the list in a simple text editor window (which BTW lacks undo/redo). I think it would be better to show the information in a tree structure similar to that in the validator tree so that the user can select groups.

[GerdP]

See also #17295. I we go that way we might not need the ignore list?

[taylor.smock]

Now uses a table to display the errors. The copy image is a bit large, renamed a file so it is a bit more generic. The table is currently editable.

[taylor.smock]

I think that it would be better to have two lists. One for errors that you explicitly don't want to see, and one for errors that you want to see.

I'm not certain about the tree structure. Probably not the worst of ideas, but I don't know how to implement it yet.

[GerdP]

With v18 I see only one entry where I should have three. The table has three rows but only the first is filled.

[GerdP]

A few more things to watch:

• Please don't use string concatenation with i18n methods (tr,trn etc) or logger like this:

tr("Validator " + type + " List Management") or Logging.error(tr("We don't understand the following type: ") + type);. It will either not work or might produce overhead on runtime. For Logging use eg.Logging.error(tr("We don't understand the following type: {0}"), type);

• If I got that right most existing JOSM messages use something like "Cannot ..." instead of "We don't ..."
• SonarLint complains when you use type.equals("ignore") instead of "ignore".equals(type). The latter will never produce a NPE

[GerdP]

Thanks, I'll try it. Any ideas regarding the obsolete entries in preferences? This is what I see when I rename the JOSM directory and start JOSM:
validator.ignore true
validator.ignorederrors null
validator.ignorederrors.bak null
validator.ignorelist []
validator.ignorelist.bak []
validator.ignorelist.bak.keep false

[GerdP]

• When I click again on the Manage Ignore button while the popup is open a 2nd popup is opened. That should be avoided.
• I found no way to remove an entry from the list. I can set the fields to blank but the entry does not disappear
• The entries in the list are not sorted. No problem unless you have clicked on "ignore single elements" with hundrets of wrong ways. See ValidatorTreePanel.sortErrors(List<TestError> errors)
• We'll need a lot of code to make this dialog "normal", e.g. clicking on the title should sort, right click on an entry might show

"delete" etc. I think the tree view is much more intuitive for this.

[taylor.smock]

I'm not getting where the validator.ignorederrors and validator.ignorederrors.bak are coming from.
I did a search for .bak and ignorederrors in the patch, and I only saw .bak in three places,

        50          /** The preferences key for the ignorelist backup */
        51          public static final String PREF_IGNORELIST_BACKUP = PREFIX + ".ignorelist.bak";
        52      
        53          /** The preferences key for whether or not the ignorelist backup should be cleared on start */
        54          public static final BooleanProperty PREF_IGNORELIST_KEEP_BACKUP = new BooleanProperty(PREFIX + ".ignorelist.bak.keep", false);
/*---------------------*/
        280         /**
        281          * Restore the error list by copying ignorederrors.bak to ignorederrors
        282          */

There are quite a few other locations with ignorederrors, but those seem to be method calls.

Just to be certain, I ran

$ grep -r ".bak" src/org/openstreetmap/josm
src/org/openstreetmap/josm/update/UpdateCommand.java:        File backup = new File(file.toString() + ".bak");
src/org/openstreetmap/josm/data/Preferences.java:                File backupFile = new File(prefDir, "preferences.xml.bak");
src/org/openstreetmap/josm/data/Preferences.java:            File backupFile = new File(prefDir, "preferences.xml.bak");
src/org/openstreetmap/josm/data/preferences/sources/ValidatorPrefHelper.java:    public static final String PREF_IGNORELIST_BACKUP = PREFIX + ".ignorelist.bak";
src/org/openstreetmap/josm/data/preferences/sources/ValidatorPrefHelper.java:    public static final BooleanProperty PREF_IGNORELIST_KEEP_BACKUP = new BooleanProperty(PREFIX + ".ignorelist.bak.keep", false);
src/org/openstreetmap/josm/data/validation/OsmValidator.java.svnpatch.rej:+     * Restore the error list by copying ignorederrors.bak to ignorederrors
src/org/openstreetmap/josm/data/validation/OsmValidator.java:     * Restore the error list by copying ignorederrors.bak to ignorederrors

As you can see, I have quite a few instances of .bak in my JOSM directory. That said, none of them are used to create anything like validator.ignorederrors.bak.

That does remind me that I need to change some javadocs.

---

As far as the tree view goes, can you post a screenshot so I know what you are talking about? If it is like the list in the Validator dialog, I'm not certain how to do that yet. I'll look into it.

I have not implemented a method to remove items from the list.

I'll look into sorting the entries, but that is dependent upon keeping the same style. If I were to switch to something like the ValidatorPanel dialog, then that might be a separate issue.

[GerdP]

OK, I'll try to find out where those preference entries are created. And yes, I'd like to have a dialog similar to that in ValidatorTreePanel.

[taylor.smock]

I'll see what I can do about that.
I would have to reverse the ignorederror to common groups or something.

Probably something like:
Description -> Message -> individual elements (if applicable). It looks like I might be able to split on :, but then I might run into other issues. I might want to split on :w_, :r_, :n_ instead, but if someone writes a test that looks like 3000_way[highway'REGEX'^(:w_|:r_|:n_)] then I've got a problem. I should probably check that the remaining characters are numbers to avoid that.

If no Description, then probably bump everything up a level.

I don't think I'll be able to get the warning level though (Error, Warning, Other).

[GerdP]

I would simply group by description, I think that's what the validator dialog does, too.

[GerdP]

Replying to GerdP:

OK, I'll try to find out where those preference entries are created. And yes, I'd like to have a dialog similar to that in ValidatorTreePanel.

My fault. I did not recognize that JOSM keeps a copy of the preferences in the cache folder.
Now, after clearning both I only see these entries:
validator.ignore true
validator.ignorelist null
validator.ignorelist.bak.keep false

[taylor.smock]

No longer using string concanation with tr, logging, etc., fixes the bug where not all errors were populated in the dialog (I forgot to increment the index), and fixes a possible NPE found by SonarLint (GerdP), and use "Cannot" in error messages.

[taylor.smock]

Now uses a tree to display the errors, does not allow for individual deletion.

[taylor.smock]

Build tree, allow deletion from tree, and rebuild error list from that tree.

[GerdP]

Getting closer :)
Problems:

• With v21 on my windows system nothing happens when I right click on the items. I had to change the code to overwrite mouseReleased() instead of mousePressed(). Seems to be a known problem, see class org.openstreetmap.josm.gui.widgets.PopupMenuLauncher.
• When I select multiple entries I only see the "delete" button when I right click on the first selected element

I am not sure if it is good idea to remove parent nodes.
Scenario:

• Run validator on an area in Japan (most roads have no name here but also don't have a noname=yes tag)
• click ignore for "Unnamed ways (1,843)" and press Enter
• download more data and rerun Validator
• You'll find a few new entries, e.g. "Unnamed ways (14)"

Now you wonder why those are not ignored and what you ignored before. If you click again on ignore and select "whole group"
you'll end up with a single "303" entry + 1843 "303:w_x" entries for the single ways. The unpatched code also works like that, so it is an old problem.
If we don't fix that your dialog will only show the entries for the "single elements". If I delete by selecting all ways and clicking "delete" the parent node for "whole group" is also removed.

Not sure where to handle this. I think we should change the code so that we never have such a mixture of "whole group" and "single elements" entries. I tried to implement that but got stuck because it seems that we cannot use a TreeMap for ignoredErrors as it causes a NPE in preferences.MapListSetting.consistencyTest()

[GerdP]

See also #17327 reg. NPE with TreeMap.

[GerdP]

v22 fixes the mousePressed() problem. I've also fixed some more problems reported by SonarLint and implemented new method cleanupIgnoredErrors() which requires a change in MapListSetting to fix #17327.
Please allow also to delete selected entries with the DEL key.

[GerdP]

have to process both events (mousePressed AND mouseReleased)

[taylor.smock]

I was removing the parent nodes since I rebuild the ignorelist based off of what is left.
For example, if you have ignored single elements w_42, n_42, and r_42, and then delete them, you wouldn't expect the ignorelist to completely ignore the whole group.

I'll look into seeing if it is feasible to iterate through the entire ignorelist everytime an ignore is added.
Maybe something like:

public void dedupignorelist() {
    ignorelist.forEach((ignore, description) -> {
        ignorelist.forEach((ignore2, description) -> {
            if (ignore2.contains(ignore) ignorelist.remove(ignore2);
        });
    });
}

I'll have to clean it up a bit, and probably convert ignorelist to a different type (I think it is currently a HashMap) that I can iterate through with a for loop instead of a forEach loop. I should probably only run it when a group is added to the ignore list OR when initially reading an ignorelist from disk.

I don't know why it has to be the first selected element, but I did disable Delete from popping up without anything selected. I'll look into it. Maybe I should just check to see if something is selected?

[GerdP]

Please check the attached v23 first.

[taylor.smock]

I just did. It (currently) doesn't detect whether or not it should be run after a new ignore is added. I probably should have read your comment for v22 before replying.

I was thinking of also collapsing multiple entries into one, but I think the rebuilt tree would do that automatically anyway.

I still need to get the right-click/delete button working.

[taylor.smock]

Modify code calling cleanupIgnoredErrors() -- move call in loadIgnoredErrors() to be dependent upon loading an older ignorelist file, add a call in addIgnoredError() that runs when there are no single osm elements in the new ignore, enable delete on keypress (no shortcut for that).

[taylor.smock]

Fixes for ant pmd checkstyle (I forgot to run it before uploading the patch)

[GerdP]

Works quite well now. I don't think that the restore button is still useful in that dialog as it is not clear what it does when you delete an entry and click on restore.

[taylor.smock]

I know. But I don't know where else to put it or how to better describe it (it restores the last used configuration, but how would that be worded for a user?). The mouseover could be Restore Previous Ignore List or something, but the text on the button still needs to reflect that. Honestly, it is mostly there if someone accidentally deletes their ignorelist and wants it back.

[taylor.smock]

Move the methods to build the JTree and ignorelist to OsmValidator, since those help clean up errors, e.g. #!text *[highway]:r_42 *[highway]:n_42 to #!text *[highway]:r_42:n_42 and fix a bug that occurs when a child and parent are selected.

[GerdP]

Something is wrong now. The method buildIgnore seems to remove entries. I cannot add more than entry to the list.

[taylor.smock]

I was able to reproduce...
And then I wasn't.

The only changes I made to the code were debug statements in between. And I don't see why that would make a difference.

[taylor.smock]

Remove some debug statements

[GerdP]

I am still able to reproduce it with v26.

[taylor.smock]

OK. I think I have some steps to reproduce.

1) Run validator with clean ignorelist
2) Ignore a group without a subgroup
3) Ignore a group with a subgroup

At this point, I now have a list that has Ignore list at the top.

Is this (roughly) what is happening with your tests? (I'll be trying to fix this one for awhile).

[GerdP]

I simply try to ignore two groups without subgroup.

[taylor.smock]

Fix issue with some groups not being added (it looks like it was due to the group not following the pattern ^[0-9]+_.*$, so I now look for ^[0-9]+_?.*). It isn't using the description of the error at this time for some reason. I will investigate that tomorrow.

[GerdP]

Yes, you have to be very careful with these patterns. The tests from MapCSSTagChecker all seem to start with 3000_ followed by a more or less unpredictable sequence of characters, maybe followed by the element pattern. So your should always assume that the element pattern appears also in the CSS rule itself. AFAIK all other tests just produce a number like 303 or 3701 (no underscore) or num + _ + element pattern. Because of plugins it is not even granted that the numbers are unique, but that's a different story.
Maybe it would be a good idea to define the patterns in TestError?

[taylor.smock]

Get groups working properly and checks that :(r|n|w)_[0-9]+ is after the last ]. Does not touch TestError yet -- if we were to implement something like TestError.parse(String error), then we would need to have some good way to send that information back, possibly as a TestErrorInformation class or something.

[GerdP]

Hm, this still doesn't look correct:

[GerdP]

Maybe a problem created by the earlier patch. I've now cleared the list and readded the three items and that looks better.
So I cannot reproduce the above picture now.

[taylor.smock]

Make the restore button the same size as the other buttons (actually, make all the buttons the same size as ok.png).

[taylor.smock]

Replying to GerdP:

Maybe a problem created by the earlier patch. I've now cleared the list and readded the three items and that looks better.
So I cannot reproduce the above picture now.

Don't worry about it. I spent quite some time hunting down the Ignore list bug when it I first saw it occur (I couldn't remember which change caused it), and I just fixed the Restore button size.

[taylor.smock]

Prevent multiple windows from popping up when Manage Ignore is clicked multiple times.

[GerdP]

• I still think that we should remove the Restore button, maybe also the Clear button. This of this situation: Start JOSM with a clear ignore list, add two or more items by clicking on the Ignore button and then click on "Manage Ignore" and remove one of the entries. Now, what do you expect to get with Restore? With v30 it "restores" a list which contains only the entry which I just removed, and it closes the dialog. I wouldn't expect any of that. The same with "Clear all". I would not expect that this button implies "OK". One should still be able to click on Cancel. So, let's keep it simple: Remove the special buttons. I think this will greatly reduce the size of the patch without loosing anything important.

• A better text for the right click button might be "Don't ignore" instead of "Delete".

Maybe we don't have to rerun Validator. I think about introducing a new boolean in TestError so that we can separate "fixed" errors from "ignored". I'll have a look at this today.

[taylor.smock]

Remove the Clear and Restore buttons and most attendant methods. The same effect as Clear can now be done without a specific button (select all, delete). There is no longer a restore, and changed the text on the popup from Delete to Don't ignore as suggested -- it still deletes from the list, but should be clearer to an end user.

[GerdP]

OK, I think it's ready for commit besides the "TODO save" comment. What would you want to save here?
Also, we have to wait for Michael to fix #17327 as my minimal patch is to simple.

[taylor.smock]

Remove TODO save comment from when I wasn't certain how I was going to save the actions in the dialog.

[taylor.smock]

As far as #17327 goes, I don't think it is an issue for this patch, since when we build the ignorelist, we always add "" instead of null to the list when there is no description. I think I did that to avoid NPE's somewhere.

That said, I might not have sufficiently tested those particular conditions.

[GerdP]

In 14828/josm:

fix #17268: There should be a method to clear ignored errors

patch clear_ignored_errors_v32.patch by taylor.smock adapted to r14827

[Don-vip]

Replying to GerdP:

In 14828/josm:

This commit added an invalid i18n string: Don't ignore. Quotes must be doubled or avoided.

[GerdP]

In 14842/josm:

see #17268: fix invalid i18n string

[GerdP]

In 14851/josm:

see #17268: fix some findbugs/sonar/javadoc issues

[GerdP]

In 14852/josm:

see #17268: fix more javadoc issues

[Don-vip]

In 14939/josm:

see #17268, see #17516 - add missing semicolons (issue reported by javadoc 13)

[GerdP]

See #17837: Probably a regression of the changes done here.

[mdk]

See #18223: Probably an other regression.