[PATCH] Redesign of SlippyMapBBoxChooser's SourceButton

[ris]

Here's an implementation of the ideas I was sketching out in #15369. This mostly fell out of the need to be able to extend the source button's capabilities to allow e.g. enabling/disabling of "show downloaded area".

Advantages of this approach:

• Extensibility
• Potential for improved accessibility
• Proper functioning with SlippyMapBBoxChooser widgets too small to show full source list
• Attempts to look a _bit_ like the layer-chooser that might be found in more modern slippy maps like Leaflet

Advantages of old approach:

• Looks like the layer-chooser in old OpenLayers slippy maps. But these are becoming rarer all the time and a user is less likely to have encountered one nowadays.

In places you'll notice I've tried to make decisions that would make it easier for plugin authors to add their own options to the menu, but of course everywhere I've tried to strike a balance between clean design and potential API breakage.

Patches are against r12940.

Can also browse the patches in github: ​https://github.com/risicle/josm/compare/576e806f627c648afdf09a9b04138b109e298164...ris-slippy-map-source-button

Attaching patches...

[ris]

I'll also attach some screenshots...

[ris]

Download dialog with menu open (patch v1)

[ris]

Small minimap with menu closed (patch v1)

[ris]

Small minimap with menu open (patch v1)

[Klumbumbus]

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

[Klumbumbus]

Would be great in 17.10 to not upset users who don't like the display of the downloaded area, if there are any ;)

Also the layer chooser in the minimap is currently pretty much useless in my case:

[ris]

Oh wow that's nasty.

Additions I'm considering but aren't currently included in the patches are:

• Tooltip on button ("Showing 'Bing aerial imagery': click to select other imagery sources" ?)
• Actually disable the "Show downloaded area" checkbox when no appropriate layer is available to remove the apparent contradiction of having the option selected, yet no markings.

and then of course, this all needs tests.

[bastiK]

Looks good, some minor things:

• Please add javadoc to all public methods (including constructors) except for those with @Override annotation. Update javadoc to reflect new method signature.
• No double newline & check unused imports
• clip does not give component dimensions but the part of the component that was obstructed and needs to be repainted.
• You can upload a single patch file (I don't mind).

[Don-vip]

You can run ant checkstyle to check everything's ok before creating the patch :)

[ris]

Please add javadoc to all public methods (including constructors)

For the JButton passthrough ones should I just copy the swing doc strings?

[ris]

Ah ok maybe I can use an @see...

[bastiK]

Replying to ris:

Please add javadoc to all public methods (including constructors)

For the JButton passthrough ones should I just copy the swing doc strings?

The swing doc is copyrighted, so just put something similar.

[ris]

I've kept it as two patches because the addition of PopupMenuButton is quite separate from the SlippyMapBBoxChooser stuff.

[bastiK]

In 12955/josm:

applied #15414 - Redesign of SlippyMapBBoxChooser's SourceButton (patch by ris)

[bastiK]

Please observe Jenkins and fix any warnings that you feel responsible for. :)

[Don-vip]

In 12962/josm:

see #15414 - fix javadoc warnings

[Klumbumbus]

When adding or removing entries in the imagery list in the preferences then the list in the download window automatically updates while the list in the minimap doesn't, even after closing and reopening the minmap. It seems a complete josm restart is required to update the list in the minimap.

[ris]

This is as it already was, no?

Yes, it's something I should take a look at and probably write a test for.

[ris]

(oh and yes, sorry, the trailing underscores are because Java's symbol ambiguity (function arg vs instance field) scares me. Same goes for my use of this. everywhere.

[Klumbumbus]

Replying to ris:

This is as it already was, no?

I don't know. I just noticed the behavior while testing the new feature. However this is a minor problem and should be low priority.