[WIP PATCH] improve readability of validator warnings

[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.

[Don-vip]

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

[Don-vip]

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

[skyper]

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.

[skyper]

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.

[Klumbumbus]

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.

[taylor.smock]

Adding quotes should be trivially easy:

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

@@ -277 +277 @@
                     Integer.parseInt(m.group(1)), m.group(2), p);
             try {
                 // Perform replacement with null-safe + regex-safe handling
-                m.appendReplacement(sb, String.valueOf(argument).replace("^(", "").replace(")$", ""));
+                final String replacement = String.valueOf(argument).replace("^(", "").replace(")$", "");
+                m.appendReplacement(sb, (replacement.startsWith("\"") ? "" : "\"")
+                        + replacement
+                        + (replacement.endsWith("\"") ? "" : "\""));
             } catch (IndexOutOfBoundsException | IllegalArgumentException e) {
                 Logging.log(Logging.LEVEL_ERROR, tr("Unable to replace argument {0} in {1}: {2}", argument, sb, e.getMessage()), e);
             }

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.

[taylor.smock]

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

[taylor.smock]

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.

[taylor.smock]

Fix UI not showing selection

[taylor.smock]

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

[Klumbumbus]

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

[Klumbumbus]

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");
}

[taylor.smock]

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

[taylor.smock]

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

[taylor.smock]

@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.

[Klumbumbus]

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?

[taylor.smock]

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.

[taylor.smock]

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

[Klumbumbus]

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.

[taylor.smock]

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?

[Klumbumbus]

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.

[Klumbumbus]

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

[taylor.smock]

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

[taylor.smock]

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

[taylor.smock]

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