Modify

Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#15414 closed enhancement (fixed)

[PATCH] Redesign of SlippyMapBBoxChooser's SourceButton

Reported by: ris Owned by: team
Priority: normal Milestone: 17.10
Component: Core Version:
Keywords: jmapviewer slippy source button Cc:

Description

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

Attachments (9)

v1-0001-add-PopupMenuButton-a-button-for-triggering-the-appe.patch (3.3 KB) - added by ris 2 months ago.
v1-0002-SlippyMapBBoxChooser-redesign-SourceButton-using-a-r.patch (15.6 KB) - added by ris 2 months ago.
v1-0003-SlippyMapBBoxChooser-maintain-state-of-showDownloadA.patch (3.0 KB) - added by ris 2 months ago.
v1_download.png (29.0 KB) - added by ris 2 months ago.
Download dialog with menu open (patch v1)
v1_minimap_closed.png (54.1 KB) - added by ris 2 months ago.
Small minimap with menu closed (patch v1)
v1_minimap_open.png (52.3 KB) - added by ris 2 months ago.
Small minimap with menu open (patch v1)
minimap_plus_button.png (19.6 KB) - added by Klumbumbus 2 months ago.
v2-0001-add-PopupMenuButton-a-button-for-triggering-the-appe.patch (4.4 KB) - added by ris 2 months ago.
v2-0002-SlippyMapBBoxChooser-redesign-SourceButton-using-a-r.patch (17.0 KB) - added by ris 2 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 2 months ago by ris

I'll also attach some screenshots...

Changed 2 months ago by ris

Attachment: v1_download.png added

Download dialog with menu open (patch v1)

Changed 2 months ago by ris

Attachment: v1_minimap_closed.png added

Small minimap with menu closed (patch v1)

Changed 2 months ago by ris

Attachment: v1_minimap_open.png added

Small minimap with menu open (patch v1)

comment:2 Changed 2 months ago by ris

Download dialog with menu open (patch v1) Small minimap with menu closed (patch v1) Small minimap with menu open (patch v1)

comment:3 Changed 2 months ago by Klumbumbus

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

comment:4 Changed 2 months ago by Klumbumbus

Milestone: 17.10

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:


Last edited 2 months ago by Klumbumbus (previous) (diff)

Changed 2 months ago by Klumbumbus

Attachment: minimap_plus_button.png added

comment:5 Changed 2 months ago by 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.

comment:6 Changed 2 months ago by 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).

comment:7 Changed 2 months ago by Don-vip

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

comment:8 Changed 2 months ago by ris

Please add javadoc to all public methods (including constructors)

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

comment:9 Changed 2 months ago by ris

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

comment:10 in reply to:  8 Changed 2 months ago by 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.

comment:11 Changed 2 months ago by ris

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

comment:12 Changed 2 months ago by bastiK

Resolution: fixed
Status: newclosed

In 12955/josm:

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

comment:13 Changed 2 months ago by bastiK

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

comment:14 Changed 2 months ago by Don-vip

In 12962/josm:

see #15414 - fix javadoc warnings

comment:15 Changed 2 months ago by 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.

comment:16 Changed 2 months ago by ris

This is as it already was, no?

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

comment:17 Changed 2 months ago by 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.

comment:18 in reply to:  16 Changed 2 months ago by 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.

Modify Ticket

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