Modify

Opened 4 months ago

Closed 3 months ago

Last modified 8 weeks ago

#17268 closed enhancement (fixed)

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

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 19.03
Component: Core validator Version:
Keywords: validation, ignore, errors Cc:

Description

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.

Attachments (36)

clear_ignored_errors.patch (4.6 KB) - added by taylor.smock 4 months ago.
clear_ignored_errors_v2.patch (6.8 KB) - added by taylor.smock 4 months ago.
Add additional dialogue options to reset/restore (doesn't ask for confirmation to reset)
clear_ignored_errors_v3.patch (5.8 KB) - added by taylor.smock 4 months ago.
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).
clear_ignored_errors_v4.patch (6.5 KB) - added by taylor.smock 4 months ago.
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.
clear_ignored_errors_v5.patch (6.5 KB) - added by taylor.smock 4 months ago.
Switch from size() > 0 to !<var>.isEmpty() and remove some tree lines.
clear_ignored_errors_v6.patch (7.6 KB) - added by taylor.smock 4 months ago.
Ask if validation should be rerun (only does selection)
rerun.PNG (15.3 KB) - added by GerdP 4 months ago.
clear_ignored_errors_v7.patch (7.7 KB) - added by taylor.smock 4 months ago.
Add check to avoid crash when there is no osm data and change the name of the question window
clear_ignored_errors_v8.patch (7.8 KB) - added by taylor.smock 4 months ago.
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).
clear_ignored_errors_v9.patch (8.3 KB) - added by taylor.smock 4 months ago.
Fix an issue (I was copying between the wrong preference keys)
clear_ignored_errors_v10.patch (8.7 KB) - added by taylor.smock 4 months ago.
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).
clear_ignored_errors_v11.patch (8.9 KB) - added by taylor.smock 4 months ago.
Default to disabled if nothing can be done, add some functionality to the ignoreErrors to call the resetignorelistButton.
clear_ignored_errors_v12.patch (10.1 KB) - added by taylor.smock 3 months ago.
Use ValidatorPrefHelper for preferences, Cancel appears to work properly in the rerunValidator dialogue, still uses a .bak preference file
clear_ignored_errors_v12.2.patch (10.2 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v13.patch (10.2 KB) - added by taylor.smock 3 months ago.
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).
clear_ignored_errors_v14.patch (10.6 KB) - added by taylor.smock 3 months ago.
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)
clear_ignored_errors_v15.patch (10.7 KB) - added by GerdP 3 months ago.
- no need to sort , fix i18n coding
clear_ignored_errors_v16.patch (16.1 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v17.patch (18.2 KB) - added by GerdP 3 months ago.
isBlank() -> isEmpty(), add description in ValidatorDialog
clear_ignored_errors_v18.patch (18.0 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v19.patch (18.2 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v20.patch (18.7 KB) - added by taylor.smock 3 months ago.
Now uses a tree to display the errors, does not allow for individual deletion.
clear_ignored_errors_v21.patch (23.6 KB) - added by taylor.smock 3 months ago.
Build tree, allow deletion from tree, and rebuild error list from that tree.
clear_ignored_errors_v22.patch (25.9 KB) - added by GerdP 3 months ago.
clear_ignored_errors_v23.patch (26.1 KB) - added by GerdP 3 months ago.
have to process both events (mousePressed AND mouseReleased)
clear_ignored_errors_v24.patch (26.4 KB) - added by taylor.smock 3 months ago.
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).
clear_ignored_errors_v24.2.patch (26.4 KB) - added by taylor.smock 3 months ago.
Fixes for ant pmd checkstyle (I forgot to run it before uploading the patch)
clear_ignored_errors_v25.patch (26.9 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v26.patch (26.6 KB) - added by taylor.smock 3 months ago.
Remove some debug statements
clear_ignored_errors_v27.patch (26.5 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v28.patch (27.1 KB) - added by taylor.smock 3 months ago.
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.
ignorelist.PNG (19.1 KB) - added by GerdP 3 months ago.
clear_ignored_errors_v29.patch (27.3 KB) - added by taylor.smock 3 months ago.
Make the restore button the same size as the other buttons (actually, make all the buttons the same size as ok.png).
clear_ignored_errors_v30.patch (27.3 KB) - added by taylor.smock 3 months ago.
Prevent multiple windows from popping up when Manage Ignore is clicked multiple times.
clear_ignored_errors_v31.patch (25.1 KB) - added by taylor.smock 3 months ago.
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.
clear_ignored_errors_v32.patch (25.0 KB) - added by taylor.smock 3 months ago.
Remove TODO save comment from when I wasn't certain how I was going to save the actions in the dialog.

Download all attachments as: .zip

Change History (99)

Changed 4 months ago by taylor.smock

Attachment: clear_ignored_errors.patch added

comment:1 Changed 4 months ago by 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.

comment:2 Changed 4 months ago by 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.

comment:3 Changed 4 months ago by 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.

Changed 4 months ago by taylor.smock

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

Changed 4 months ago by 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).

comment:4 Changed 4 months ago by 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.

Last edited 4 months ago by GerdP (previous) (diff)

Changed 4 months ago by 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.

comment:5 Changed 4 months ago by 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?

comment:6 Changed 4 months ago by 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.

Changed 4 months ago by taylor.smock

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

Changed 4 months ago by taylor.smock

Ask if validation should be rerun (only does selection)

Changed 4 months ago by GerdP

Attachment: rerun.PNG added

comment:7 Changed 4 months ago by 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"?

comment:8 Changed 4 months ago by 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.

Changed 4 months ago by taylor.smock

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

comment:9 Changed 4 months ago by GerdP

Milestone: 19.02

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)?

comment:10 Changed 4 months ago by 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.

comment:11 in reply to:  9 Changed 4 months ago by 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.

comment:12 Changed 4 months ago by 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.

Changed 4 months ago by 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).

Changed 4 months ago by taylor.smock

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

comment:13 Changed 4 months ago by 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.

comment:14 Changed 4 months ago by 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?

comment:15 Changed 4 months ago by 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.

Changed 4 months ago by 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).

comment:16 Changed 4 months ago by 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

comment:17 Changed 4 months ago by 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.

Changed 4 months ago by taylor.smock

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

comment:18 Changed 3 months ago by 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.

comment:19 Changed 3 months ago by 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?

Changed 3 months ago by taylor.smock

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

Changed 3 months ago by 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.

comment:20 in reply to:  19 Changed 3 months ago by 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.

comment:21 Changed 3 months ago by 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.

Changed 3 months ago by 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).

Changed 3 months ago by 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)

Changed 3 months ago by GerdP

  • no need to sort , fix i18n coding

comment:22 Changed 3 months ago by 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.

comment:23 Changed 3 months ago by 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.

comment:24 Changed 3 months ago by GerdP

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

Changed 3 months ago by 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.

Changed 3 months ago by GerdP

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

comment:25 Changed 3 months ago by 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.

comment:26 Changed 3 months ago by GerdP

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

Changed 3 months ago by 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.

comment:27 Changed 3 months ago by 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.

comment:28 Changed 3 months ago by GerdP

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

comment:29 Changed 3 months ago by 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

comment:30 Changed 3 months ago by 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

comment:31 Changed 3 months ago by 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.

comment:32 Changed 3 months ago by 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.

comment:33 Changed 3 months ago by 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.

comment:34 Changed 3 months ago by 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).

comment:35 Changed 3 months ago by GerdP

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

comment:36 in reply to:  33 Changed 3 months ago by 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

Changed 3 months ago by 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.

Changed 3 months ago by taylor.smock

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

Changed 3 months ago by taylor.smock

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

comment:37 Changed 3 months ago by 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()

comment:38 Changed 3 months ago by GerdP

See also #17327 reg. NPE with TreeMap.

Changed 3 months ago by GerdP

comment:39 Changed 3 months ago by 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.

Changed 3 months ago by GerdP

have to process both events (mousePressed AND mouseReleased)

comment:40 Changed 3 months ago by 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?

comment:41 Changed 3 months ago by GerdP

Please check the attached v23 first.

comment:42 Changed 3 months ago by 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.

Changed 3 months ago by 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).

Changed 3 months ago by taylor.smock

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

comment:43 Changed 3 months ago by 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.

comment:44 Changed 3 months ago by 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.

Changed 3 months ago by 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.

comment:45 Changed 3 months ago by GerdP

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

comment:46 Changed 3 months ago by 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.

Changed 3 months ago by taylor.smock

Remove some debug statements

comment:47 Changed 3 months ago by GerdP

I am still able to reproduce it with v26.

comment:48 Changed 3 months ago by 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).

comment:49 Changed 3 months ago by GerdP

I simply try to ignore two groups without subgroup.

Changed 3 months ago by 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.

comment:50 Changed 3 months ago by 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?

Changed 3 months ago by 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.

Changed 3 months ago by GerdP

Attachment: ignorelist.PNG added

comment:51 Changed 3 months ago by GerdP

Hm, this still doesn't look correct:

comment:52 Changed 3 months ago by 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.

Changed 3 months ago by taylor.smock

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

comment:53 in reply to:  52 Changed 3 months ago by 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.

Changed 3 months ago by taylor.smock

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

comment:54 Changed 3 months ago by 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.

Changed 3 months ago by 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.

comment:55 Changed 3 months ago by 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.

Changed 3 months ago by taylor.smock

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

comment:56 Changed 3 months ago by 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.

comment:57 Changed 3 months ago by Don-vip

Milestone: 19.0219.03

comment:58 Changed 3 months ago by GerdP

Resolution: fixed
Status: newclosed

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

comment:59 in reply to:  58 Changed 3 months ago by Don-vip

Replying to GerdP:

In 14828/josm:

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

comment:60 Changed 3 months ago by GerdP

In 14842/josm:

see #17268: fix invalid i18n string

comment:61 Changed 2 months ago by GerdP

In 14851/josm:

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

comment:62 Changed 2 months ago by GerdP

In 14852/josm:

see #17268: fix more javadoc issues

comment:63 Changed 8 weeks ago by Don-vip

In 14939/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.