Modify

Opened 11 months ago

Closed 11 months ago

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

Download all attachments as: .zip

Change History (27)

comment:1 Changed 11 months ago by ris

I'll also attach some screenshots...

Changed 11 months ago by ris

Attachment: v1_download.png added

Download dialog with menu open (patch v1)

Changed 11 months ago by ris

Attachment: v1_minimap_closed.png added

Small minimap with menu closed (patch v1)

Changed 11 months ago by ris

Attachment: v1_minimap_open.png added

Small minimap with menu open (patch v1)

comment:2 Changed 11 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 11 months ago by Klumbumbus

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

comment:4 Changed 11 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 11 months ago by Klumbumbus (previous) (diff)

Changed 11 months ago by Klumbumbus

Attachment: minimap_plus_button.png added

comment:5 Changed 11 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 11 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 11 months ago by Don-vip

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

comment:8 Changed 11 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 11 months ago by ris

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

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

Resolution: fixed
Status: newclosed

In 12955/josm:

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

comment:13 Changed 11 months ago by bastiK

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

comment:14 Changed 11 months ago by Don-vip

In 12962/josm:

see #15414 - fix javadoc warnings

comment:15 Changed 10 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 10 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 10 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 10 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.