[PATCH] Extends GPS legend for time information, improve design

[Pauline]

I added the legend for the time information and improved the general design of the legend, including a transparent background and outline for the color bar texts.

This is the before:

This is the after:

[taylor.smock]

Can you document the magic numbers?
Example: w+fw+37 -- what is the 37 for? I assume it is "just" a padding, but what is the padding for?

Also, having a comparison of the the same legend type before/after would help. It looks like you did the following:

1. Added a common background (defaults to a transparent light grey colour)
2. Modified the title output for some legends

Also, can you modify the equation formatting to have spaces (e.g. w+fw+37 -> w + fw + 37) so that it is stylistically consistent with the rest of codebase.

Additional notes:

• Instead of passing 0.0 in for minVal and maxVal, why not use Double.NaN? You can then use Double.isNaN for the checks, and it will allow someone to use the (probably very rare) gpx points from epoch. I can see someone using survey data from around the epoch though.
• Some functions should be split out. For stuff like this, the body of if statements is convenient since those are usually discrete chunks of functionality. This makes the code easier to understand long term, especially if the new functions are well documented.

[Pauline]

Replying to taylor.smock:

Can you document the magic numbers?
Example: w+fw+37 -- what is the 37 for? I assume it is "just" a padding, but what is the padding for?

I created a new variable padding, which is the distance from the color bar texts to the edge of the grey rectangle. I then used this value to create different paddings. Means I deleted all the magic numbers and replaced them with either padding or padding / 2 or something like that.

Also, having a comparison of the the same legend type before/after would help. It looks like you did the following:

1. Added a common background (defaults to a transparent light grey colour)
2. Modified the title output for some legends

Yes, I did the two things you mentioned. I also added an outline for the texts next to the color bar (light or dark - depends on the color of the texts). I also looked at the position of title, texts and color bar to make it more beautiful.

This is the legend before I edited it:

This is the legend after I edited it:

Also, can you modify the equation formatting to have spaces (e.g. w+fw+37 -> w + fw + 37) so that it is stylistically consistent with the rest of codebase.

Yes, I changed that.

Additional notes:

• Instead of passing 0.0 in for minVal and maxVal, why not use Double.NaN? You can then use Double.isNaN for the checks, and it will allow someone to use the (probably very rare) gpx points from epoch. I can see someone using survey data from around the epoch though.
• Some functions should be split out. For stuff like this, the body of if statements is convenient since those are usually discrete chunks of functionality. This makes the code easier to understand long term, especially if the new functions are well documented.

I split the function DrawColorBar into two functions:

• DrawColorBar for all the scales except the time scale and
• DrawColorBarTime for the time scale.

This has the advantage, that I don't have the 0.0 and I also don't have the Double.NaN.

You can see all my changes in the file josm_legend_2.patch.

[Pauline]

Are the changes I made enough or should I change something else as well? Is there anything I should explain to you in more detail?

[taylor.smock]

Thanks for poking this ticket -- I'll try to pull down the patch for local testing today.

[taylor.smock]

In 19236/josm:

Fix #23926: Extend GPS legend for time information, improve design (patch by Pauline, modified)

Modifications are as follows:

• Reduction of code duplication
• Addition of functions in ColorHelper to calculate contrast ratios

[taylor.smock]

If we ever switch to git, do you have a preferred name and email to properly attribute the patch?

[stoecker]

Coverity complains. When I understand this correctly it says the null checks can be removed, as they will never be true. Though why that should be an issue... And additional check does no harm, or does it?

** CID 1563450:  Null pointer dereferences  (REVERSE_INULL)
/src/org/openstreetmap/josm/tools/ColorScale.java: 327 in org.openstreetmap.josm.tools.ColorScale.drawColorBar(java.awt.Graphics2D, int, int, int, int, double)()


________________________________________________________________________________________________________
*** CID 1563450:  Null pointer dereferences  (REVERSE_INULL)
/src/org/openstreetmap/josm/tools/ColorScale.java: 327 in org.openstreetmap.josm.tools.ColorScale.drawColorBar(java.awt.Graphics2D, int, int, int, int, double)()
321                 } else {
322                     g.fillRect(xText + fw + 7 + i * w / n, y, w / n, h + 1);
323                 }
324             }
325    
326             // legend title

                CID 1563450:  Null pointer dereferences  (REVERSE_INULL)
                Null-checking "title" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

327             if (title != null) {
328                 g.setColor(LEGEND_TITLE);
329                 g.drawString(title, xRect + rectWidth / 2 - titleWidth / 2, y - fh * 3 / 2 - 10);
330             }
331    
332             // legend texts

** CID 1563449:  Null pointer dereferences  (REVERSE_INULL)
/src/org/openstreetmap/josm/tools/ColorScale.java: 387 in org.openstreetmap.josm.tools.ColorScale.drawColorBarTime(java.awt.Graphics2D, int, int, int, int, double, double)()


________________________________________________________________________________________________________
*** CID 1563449:  Null pointer dereferences  (REVERSE_INULL)
/src/org/openstreetmap/josm/tools/ColorScale.java: 387 in org.openstreetmap.josm.tools.ColorScale.drawColorBarTime(java.awt.Graphics2D, int, int, int, int, double, double)()
381                 } else {
382                     g.fillRect(xText + fw + padding / 3 + i * w / n, y, w / n + 1, h);
383                 }
384             }
385    
386             // legend title

                CID 1563449:  Null pointer dereferences  (REVERSE_INULL)
                Null-checking "title" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

387             if (title != null) {
388                 g.setColor(LEGEND_TITLE);
389                 g.drawString(title, xRect + rectWidth / 2 - titleWidth / 2, y - fh * 3 / 2 - padding / 2);
390             }
391    
392             // legend texts

[taylor.smock]

It looks like it is due to the fm.stringWidth(title) on L304/L370. That will throw an NPE, so the null check on L327/L387 is useless. I'm surprised that my IDE didn't complain about it though.

[taylor.smock]

I'm going to add a null check in addTitle and remove the two "problematic" null checks.

[taylor.smock]

In 19238/josm:

See #23926: Fix new coverity issues

Coverity doesn't like null checks when a previous statement would have thrown
a NullPointerException on the object being checked for null. This fixes the
"defect" by removing the null checks and adding a null check in
ColorScale#addTitle.

[stoecker]

Thanks BTW for the review process. Pauline is my intern and I wanted to give her a real-life experience with OpenSource bug fixing. It's her first time in Java programming (counting the livegps changes as the same task). The request for the changes came from one of my colleagues. I did only help a little bit ;-)

[taylor.smock]

Good to know; if I'd known she was an intern, I would have handled this a bit differently (as in, I probably would have treated this as more of a learning opportunity for her).

I don't know if she has already done this, but she should probably go through what I actually applied and figure out what I changed and why.

Of specific note, she might want to look into why using the green color channel as the only method for determining contrast for outline colors isn't the wisest decision. I don't like my solution either, but it should produce the best results for a significantly wider range of colors.

And who knows, she might tell me I did something stupid. :)