Modify

#20729 closed enhancement (fixed)

Improve validation with data filters enabled

Reported by: ljdelight Owned by: GerdP
Priority: normal Milestone: 21.05
Component: Core validator Version: latest
Keywords: filter Cc:

Description

Users de-clutter their editing area by enabling filters to hide/disable objects, and these objects are sometimes not included when a user performs validation and are often hidden from view after validation actually occurs. This may skew the validation results without the user being aware of the issue.

This enhancement is to warn the user when validation is performed while filters are actively hiding/disabling objects. The dialog allows the user to persist their response as the saved preference key 'validator.vallidate_with_filters_action' (values are 'ask' (default), 'continue', and 'fail').

When the dialog should be shown to the user, here's a screenshot and the 'Help' button opens the JOSM Help Browser Validator page (https://wiki.openstreetmap.org/wiki/JOSM/Validator ):

When the 'error' selection is saved, the next time the user runs validation with active filters this error message is shown:

The 'validator.vallidate_with_filters_action' Preference can be configured by changing the radio selection under Edit > Settings > Data validator:

Attachments (12)

prompt-dialog.png (30.3 KB) - added by ljdelight 14 months ago.
preferences-options.png (4.7 KB) - added by ljdelight 14 months ago.
error-dialog.png (9.4 KB) - added by ljdelight 14 months ago.
josm-validation-with-filters.patch (11.7 KB) - added by ljdelight 14 months ago.
validation-with-filter.png (96.8 KB) - added by ljdelight 14 months ago.
Validation done with the filter enabled, causing the 'way end node near other highway' result to not be identified.
validation-no-filter.png (100.6 KB) - added by ljdelight 14 months ago.
ticket20729-filter-changes-validation-results.osm (1021 bytes) - added by ljdelight 14 months ago.
20729-replace-isDrawable.patch (2.2 KB) - added by GerdP 14 months ago.
My proposal to fix the tests by replacing sDrawable() by more appropriate methods
validation-has-hidden-results.png (12.4 KB) - added by ljdelight 13 months ago.
notification.png (48.7 KB) - added by ljdelight 13 months ago.
Example of the notification
josm-show-notification-on-hidden-objects.patch (1.9 KB) - added by ljdelight 13 months ago.
20729-notification.patch (2.1 KB) - added by GerdP 13 months ago.
sligtly simplified patch with different text

Download all attachments as: .zip

Change History (42)

Changed 14 months ago by ljdelight

Attachment: prompt-dialog.png added

Changed 14 months ago by ljdelight

Attachment: preferences-options.png added

Changed 14 months ago by ljdelight

Attachment: error-dialog.png added

Changed 14 months ago by ljdelight

comment:1 Changed 14 months ago by GerdP

I was not aware that filters have an influence on the results of the validator. I think this should not happen, but maybe it's a wanted feature? I rarely work with filters. Can you give an example to reproduce this problem?

comment:2 Changed 14 months ago by GerdP

Owner: changed from team to ljdelight
Status: newneedinfo

comment:3 Changed 14 months ago by Don-vip

The error message should explain how to disable it in preferences.

comment:4 Changed 14 months ago by Don-vip

Component: CoreCore validator
Keywords: filter added; validator removed
Milestone: 21.04

comment:5 Changed 14 months ago by skyper

What is that special about active filter with the indication in the top right corner of Mapview compared to validation with incomplete data, some selection or on upload?

Changed 14 months ago by ljdelight

Attachment: validation-with-filter.png added

Validation done with the filter enabled, causing the 'way end node near other highway' result to not be identified.

Changed 14 months ago by ljdelight

Attachment: validation-no-filter.png added

comment:6 in reply to:  1 Changed 14 months ago by ljdelight

Replying to GerdP:

I was not aware that filters have an influence on the results of the validator. I think this should not happen, but maybe it's a wanted feature? I rarely work with filters. Can you give an example to reproduce this problem?

Sure, I attached 'ticket20729-filter-changes-validation-results.osm' with two ways that are near but not connected. The filter 'highway: residential' enabled and hidden does change the validation results from 3 warnings to 2 warnings, screenshots follow.
Additionally, I have seen cases where the validation creates objects (in the validation layer) that are not visible to the user and the dialog would help there.

Validation shows 3 warnings without a filter:

Validation shows 2 warnings with a filter (disable checked):
Validation done with the filter enabled, causing the 'way end node near other highway' result to not be identified.

comment:7 Changed 14 months ago by GerdP

OK, I see. The difference in the result is probably caused by the use of IPrimitive.isDrawable(). The implementations for this method return false for the filtered objects and thus some tests don't "see" all data. This is probably not intended, but I have to dig deeper to be sure. I'll open new tickets for those cases which seem wrong.

comment:8 Changed 14 months ago by simon04

Only LongSegment and UnconnectedWays are affected. This isDrawable checks have been introduced in r5920 and r12032, respectively.

comment:9 Changed 14 months ago by GerdP

The code in Multipolygon also checks isDrawable and some tests use this code.

comment:10 Changed 14 months ago by GerdP

This has some more ugly effects. If you have a filter activated that shows only natural=tree and load a file containing some multipolygons and then deactivate the filter the multipolygons are not rendered properly.

comment:11 Changed 14 months ago by GerdP

I think Multipolygon.load() should not checkisDrawable(). I assume isUsable() was meant here and also in the other tests.

Changed 14 months ago by GerdP

My proposal to fix the tests by replacing sDrawable() by more appropriate methods

comment:12 Changed 14 months ago by simon04

Owner: changed from ljdelight to GerdP
Status: needinfonew

+1. Sounds reasonable. Time will tell if isDrawable was there for a good reason...

comment:13 Changed 14 months ago by GerdP

In 17761/josm:

see #20729:Alert user if validation action is performed with data filters enabled

  • replace isDrawable() by more appropriate methods to make results of validator independent of filter settings

TODO: use MultipolygonCache.get() instead of new Multipolygon() in more places to improve performance.

comment:14 Changed 14 months ago by GerdP

Owner: changed from GerdP to ljdelight
Status: newneedinfo

@ljdelight: I think r17761 solves part of the problem.
Do you see other possible reasons why filters cause different test results? I looked for isDisabled and other methods which depend on the filter but found no more issues in core.

comment:15 Changed 13 months ago by ljdelight

Hey @GerdP, your patch solved what I had noticed with the different test results.
The remaining issue is the case where the validation creates objects that are not visible to the user (comment 6 has the screenshots).
What are your thoughts on the dialog?

comment:16 Changed 13 months ago by GerdP

I see no reason to show a warning before the validation is executed now that the errors are fixed. Maybe just show a hint when the validation found problems with hidden objects? Or show a hint when such a problem is selected?

comment:17 Changed 13 months ago by simon04

Summary: [patch] Alert user if validation action is performed with data filters enabledImprove validation with data filters enabled

Changed 13 months ago by ljdelight

comment:18 Changed 13 months ago by ljdelight

Okay I hacked together a simple dialog that displays after validation is done and a TestError includes primitives that aren't drawable.

I'm not familiar with a hint, do you have an example? Thank you

comment:19 Changed 13 months ago by GerdP

Look for Notification

Changed 13 months ago by ljdelight

Attachment: notification.png added

Example of the notification

Changed 13 months ago by ljdelight

comment:20 Changed 13 months ago by ljdelight

That's slick, here's what it can look like:
Example of the notification

Current patch at this point would be josm-show-notification-on-hidden-objects.patch​

comment:21 Changed 13 months ago by GerdP

I think the text is too complex and missleading as the validation layer doesn't show markers for filtered objects.
What about Validation results contain elements hidden by a filter. for the first line?

This also works for users that deactivate the "Validation errors" layer. Still, they may note problems when the "Zoom to problem" action shows an empty area.

Changed 13 months ago by GerdP

Attachment: 20729-notification.patch added

sligtly simplified patch with different text

comment:22 Changed 13 months ago by ljdelight

thanks @GerdP

comment:23 Changed 13 months ago by simon04

Considering the milestone:21.04 roadmap, we probably should no longer change i18n strings...

comment:24 Changed 13 months ago by GerdP

Yes, I also want to wait.

comment:25 Changed 13 months ago by ljdelight

Hi, is there anything else needed on this ticket? It's in a need info. Thanks

comment:26 Changed 13 months ago by GerdP

Owner: changed from ljdelight to GerdP
Status: needinfoassigned

No, I'll commit my patch after release of 21.04 unless anybody suggests a better text.

comment:27 Changed 13 months ago by skyper

What do you want to say with "review"? Is "disable" better?

Please disable active filters to see the hidden results.

comment:28 Changed 13 months ago by GerdP

My understanding is that the filters may stay activated, only if the checkbox below H is enabled nothing is rendered and "zoom to problem" might show an empty area.

comment:29 Changed 13 months ago by simon04

Milestone: 21.0421.05

comment:30 Changed 13 months ago by GerdP

Resolution: fixed
Status: assignedclosed

In 17852/josm:

fix #20729: Improve validation with data filters enabled

  • show notification when validator was executed with filters and at least one element in the error message is hidden

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.