Modify

Opened 9 years ago

Last modified 16 months ago

#11153 new enhancement

[WIP PATCH] improve readability of validator warnings

Reported by: Klumbumbus Owned by: team
Priority: normal Milestone:
Component: Core validator Version:
Keywords: Cc:

Description (last modified by Klumbumbus)

example of a validator warning:
tourism=information without information

it would be nice if the keys and tags which are included by {x} are written differently visualized (e.g. italic, other font, other color or highlighting). This way you can better understand which word is a key/tag and which is just a "normal" word of the validator message.

A few validator messages use additional ' characters but this is not optimal and adding this to the other messages would trigger a retranslation of all those messages.

Attachments (8)

11153.patch (12.4 KB ) - added by taylor.smock 16 months ago.
Add hyperlinks to *.tag|*.key in the ValidatorTreePanel
11153.2.patch (16.6 KB ) - added by taylor.smock 16 months ago.
Fix UI not showing selection
11153a.PNG (45.1 KB ) - added by Klumbumbus 16 months ago.
11153b.PNG (6.3 KB ) - added by Klumbumbus 16 months ago.
11153.3.patch (21.8 KB ) - added by taylor.smock 16 months ago.
Fix missing folder icons, add mouse pointer feedback (hand cursor), add proposed method for linking tag/keys/values not in selector
HtmlTreeCellRenderer.java (11.7 KB ) - added by taylor.smock 16 months ago.
src/org/openstreetmap/josm/gui/widgets/HtmlTreeCellRenderer.java from attachment:11153.3.patch
11153c.gif (139.9 KB ) - added by Klumbumbus 16 months ago.
11153.4.patch (36.4 KB ) - added by taylor.smock 16 months ago.
Fix broken tests (add <a href=...) and correctly obtain the ignore group

Download all attachments as: .zip

Change History (29)

comment:1 by Klumbumbus, 6 years ago

Description: modified (diff)

comment:2 by Don-vip, 6 years ago

Ticket #15993 has been marked as a duplicate of this ticket.

comment:3 by Don-vip, 6 years ago

Agreed, we should avoid changing the string to translate. We can surely add quotes ad maybe hyperlinks like suggested in #15993 from Java code.

comment:4 by skyper, 4 years ago

Additionally, I'd like to have an option to add =* to a {*.key}. The problem is that we need both, simply adding it to every {*.key} will not work.

comment:5 by skyper, 17 months ago

I recognized that placeholders, mean-while, sometimes only use the key or value as "normal" text and not in context of tags. Most of the time the placeholder needs to be changed to text in the message but that might not work for all cases if multiple expressions share one message.

Last edited 17 months ago by skyper (previous) (diff)

comment:6 by Klumbumbus, 17 months ago

In the last 8 years since I created this ticket I always had it in mind when adding/editing validator messages ;)
But you are right, we have some of those validator messages and we would need to double check them all.

comment:7 by taylor.smock, 17 months ago

Adding quotes should be trivially easy:

  • src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java

    diff --git a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java b/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java
    a b  
    277277                    Integer.parseInt(m.group(1)), m.group(2), p);
    278278            try {
    279279                // Perform replacement with null-safe + regex-safe handling
    280                 m.appendReplacement(sb, String.valueOf(argument).replace("^(", "").replace(")$", ""));
     280                final String replacement = String.valueOf(argument).replace("^(", "").replace(")$", "");
     281                m.appendReplacement(sb, (replacement.startsWith("\"") ? "" : "\"")
     282                        + replacement
     283                        + (replacement.endsWith("\"") ? "" : "\""));
    281284            } catch (IndexOutOfBoundsException | IllegalArgumentException e) {
    282285                Logging.log(Logging.LEVEL_ERROR, tr("Unable to replace argument {0} in {1}: {2}", argument, sb, e.getMessage()), e);
    283286            }

Adding hyperlinks would be a bit more difficult. And the quote adding is going to miss tags quoted with ', but that is easy enough to fix before applying the patch.

Anyway, I'm looking at doing URLs, and I've been able to get them to display.

Having mouse clicks open hyperlinks is a different problem.

by taylor.smock, 16 months ago

Attachment: 11153.patch added

Add hyperlinks to *.tag|*.key in the ValidatorTreePanel

comment:8 by taylor.smock, 16 months ago

I've managed to make the hyperlinks clickable.

attachment:11153.patch does the following:

  • Add a new TreeCellRenderer class whose focus is on rendering HTML and allowing links to be opened.
  • Modifies MapCSSTagCheckerRule to add links to descriptions
  • Wrap the message with html tags in ValidatorTreePanel

@skyper/Klumbumbus: Let me know if the hyperlinks don't make sense from a UI perspective, or if there is some validation where the key/tag should not be linked.

I think the hyperlinks make sense, and I haven't seen any validations where having the key/tag linked does not make sense.

by taylor.smock, 16 months ago

Attachment: 11153.2.patch added

Fix UI not showing selection

comment:9 by taylor.smock, 16 months ago

Milestone: 23.01
Summary: improve readability of validator warnings[PATCH] improve readability of validator warnings

I'll go ahead and apply the patch next week, absent feedback.

comment:10 by Klumbumbus, 16 months ago

What I noticed with the patch (in the screenshot on the right):

  • folder icons are partly missing
  • on these lines with missing folder icons also the mouse over popup is broken
  • text becomes bigger so lower parts of letters are cut off (e.g.: g, y, p, _)
  • on hyperlinks I would expect the mouse pointer to change to the hand with pointing index finger, but it stays as default arrow
  • it doesn't work with java coded tests(?)


Revision:18622
Is-Local-Build:true
Build-Date:2023-01-11 23:13:26

Identification: JOSM/1.5 (18622 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19045)
Memory Usage: 404 MB / 4058 MB (66 MB allocated, but free)
Java version: 17.0.5+8-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680×1050 (scaling 1.00×1.00)
Maximum Screen Size: 1680×1050
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: en_DE
Numbers with default locale: 1234567890 -> 1234567890
Dataset consistency test: No problems found
Last edited 16 months ago by Klumbumbus (previous) (diff)

by Klumbumbus, 16 months ago

Attachment: 11153a.PNG added

comment:11 by Klumbumbus, 16 months ago

And it would be nice if we could deal with things like (in combinations.mapcss), where half of the tags get a link and the other half not.

way[waterway=weir][bridge=yes][highway] {
  throwWarning: tr("{0} together with {1}", "{0.tag}", "{1.tag}");
  suggestAlternative: "waterway=weir + ford=yes";
  suggestAlternative: "waterway=dam";
  suggestAlternative: tr("two objects, one with {0} and one with {1} + {2} + {3}", "{0.tag}", "{2.key}", "{1.tag}", "layer");
  group: tr("suspicious tag combination");
}


by Klumbumbus, 16 months ago

Attachment: 11153b.PNG added

in reply to:  10 ; comment:12 by taylor.smock, 16 months ago

Replying to Klumbumbus:

What I noticed with the patch (in the screenshot on the right):

  • folder icons are partly missing

I noticed this yesterday (I was comparing old to new, and went "wait, there are folder icons?"), and was testing a fix out -- essentially, I just add the panel to the JLabel. It looks like it works, and I'll upload that later today once I have fiddled with it a bit more.
EDIT: Doing this made the hyperlinks unclickable.

  • on these lines with missing folder icons also the mouse over popup is broken
  • text becomes bigger so lower parts of letters are cut off (e.g.: g, y, p, _)

I'm not seeing either of those problems right now (either with the fix for your first problem or or without that fix). I'm hoping this isn't a platform-specific thing.

  • on hyperlinks I would expect the mouse pointer to change to the hand with pointing index finger, but it stays as default arrow

I'd like to do this too, but we don't do this in some other locations, so it wasn't top of mind. I think I just have to add a mouse motion listener, and if there is a link, change the cursor.

  • it doesn't work with java coded tests(?)

Of course not. Each test will have to be adapted. Right now, it depends upon two things:

  1. <html> at the beginning of the string and </html> at the end of the string. This is done in ValidatorTreePanel, so tests don't need to do this.
  2. The check itself has to add the <a href="url">text</a>.

    And it would be nice if we could deal with things like (in combinations.mapcss), where half of the tags get a link and the other half not.

The code relies upon the regex in MapCSSTagCheckerRule, which looks for patterns matching "\\{(\\d+)\\.(key|value|tag)\\}. Inserting specific key/tags does not work right now. We'd have to change something such that we know that a pattern is a tag/key/value. We could try guessing, but then we start running into problems with keys versus values. Example: building. Are we looking at osmwiki:Key:building or osmwiki:Tag:historic=building?

The new regex should be something like \\{(\\S+)\\.(key|value|tag)\\}. We'll then have to check and see if group 1 is all numbers, and if so, follow the old code path. Else follow a new code path that just uses group 1.

So the rule would look something like this:

way[waterway=weir][bridge=yes][highway] {
  throwWarning: tr("{0} together with {1}", "{0.tag}", "{1.tag}");
  suggestAlternative: tr("{0} + {1}", "{waterway=weir.tag}", "{ford=yes.tag}");
  suggestAlternative: tr("{0}", "{waterway=dam.tag}";
  suggestAlternative: tr("two objects, one with {0} and one with {1} + {2} + {3}", "{0.tag}", "{2.key}", "{1.tag}", "{layer.key}");
  group: tr("suspicious tag combination");
}

If we wanted to link values in the future, {building=retail.value} would be an example (we would link to osmwiki:Tag:building=retail).

As a side note: if we are changing the way we parse messages, such that {building=yes.tag} is something we support, we should probably ping Osmose (I think they use our mapcss rules).

Last edited 16 months ago by taylor.smock (previous) (diff)

by taylor.smock, 16 months ago

Attachment: 11153.3.patch added

Fix missing folder icons, add mouse pointer feedback (hand cursor), add proposed method for linking tag/keys/values not in selector

comment:13 by taylor.smock, 16 months ago

@Klumbumbus: Let me know if text rendering is still broken on your system. It probably won't be, since I am no longer returning the JosmEditorPane anymore, but since I didn't see that problem on my system, it might be something related to rendering HTML in a JTree on Windows.

in reply to:  12 comment:14 by Klumbumbus, 16 months ago

Replying to taylor.smock:

Replying to Klumbumbus:

  • on hyperlinks I would expect the mouse pointer to change to the hand with pointing index finger, but it stays as default arrow

I'd like to do this too, but we don't do this in some other locations

We already have this feature e.g. at the startup page, in the Imagery preferences ("New default entries can be added in the Wiki.") or Notes in the mapview.

When I tried patch .3 I noticed that it didn't add the new file HtmlTreeCellRenderer.java while applying the patch. I don't know why. It turns out the same happend also when I tried patch .2. So comment:10 is without that file ...

Could you please add this file (from .3) here seperataly so I could test .3 properly?

comment:15 by taylor.smock, 16 months ago

Erm. Can you double-check that? I'm seeing the new file on L198 in attachment:11153.3.patch.

Just to make certain, I ran curl https://josm.openstreetmap.de/raw-attachment/ticket/11153/11153.3.patch | patch -p1 && ant clean dist on a clean svn checkout, and it compiled and ran properly.

If you are applying the patch from an IDE, I noticed that IntelliJ IDEA seems to be having some issues creating new files when I was looking at someone else's patch.

I'll go ahead and upload the file separately, just in case something is borked with the patch implementation you are using.

by taylor.smock, 16 months ago

Attachment: HtmlTreeCellRenderer.java added

src/org/openstreetmap/josm/gui/widgets/HtmlTreeCellRenderer.java from attachment:11153.3.patch

comment:16 by Klumbumbus, 16 months ago

Ah, now I see that the file was added during applying the patch, it just was not indicated at all that it added a file. It showed only the modified files.
So I did a clean test again (resetted svn, applied patch .3 and did ant clean dist). I see the same problems as with patch .2 in comment:10.

comment:17 by taylor.smock, 16 months ago

OK. I'm now confused. Why are we seeing different behavior? I'm seeing the folders where they are supposed to be.

I'll upload a JAR file from a clean SVN checkout with the patch applied. Is it the build environment? Or is it something to do with the LaF?

comment:18 by Klumbumbus, 16 months ago

OK, I don't know what was broken with my svn, but now I finally get the same behavior with my build than with your build. Sorry for the trouble.

comment:19 by Klumbumbus, 16 months ago

The finger mousepointer sometimes works on the whole tag, sometimes only on the value and sometimes not at all. The following screencast was made with your josm-custom.jar, just to be safe ;)

by Klumbumbus, 16 months ago

Attachment: 11153c.gif added

comment:20 by taylor.smock, 16 months ago

FML. I'll see if I can borrow a Windows machine so I can see if I can figure out what is going on.

comment:21 by taylor.smock, 16 months ago

Milestone: 23.01
Summary: [PATCH] improve readability of validator warnings[WIP PATCH] improve readability of validator warnings

I'm going to have to figure out how to deal with changing validator warnings (for MapCSS rules) anyway. It wasn't something I originally considered.

From MapCSSTagCheckerTest:
Expected :3000_footway used with foot=no
Actual :3000_<a href="https://wiki.openstreetmap.org/wiki/Tag:highway=footway">footway</a> used with <a href="https://wiki.openstreetmap.org/wiki/Tag:foot=no">foot=no</a>
I can either try removing all html tags (e.g. <.*?>) or figure something else out.

Anyway, I was able to reproduce your issue. It looks like it is a LaF thing. I was unable to reproduce with Metal LaF, but was able to reproduce with Windows LaF. It might be related to #20850, except the font is probably ~1.1x the size it should be (larger instead of smaller).

by taylor.smock, 16 months ago

Attachment: 11153.4.patch added

Fix broken tests (add <a href=...) and correctly obtain the ignore group

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to Klumbumbus.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.