Modify

Opened 3 months ago

Closed 2 months ago

Last modified 8 weeks ago

#15369 closed enhancement (fixed)

[PATCH] download area display in SlippyMapBBoxChooser

Reported by: ris Owned by: team
Priority: normal Milestone: 17.10
Component: Core Version: latest
Keywords: SlippyMapBBoxChooser download area OsmDataLayer paint Cc:

Description (last modified by ris)

There are a good number of reasons it's useful to be able to see the extents of the downloaded area in a SlippyMapBBoxChooser. In the download dialog it makes it possible to line up a new download with the existing downloaded area. The MiniMap does a much better job of giving the user an idea of where they are in their larger work area if they can see the download bounds.

Here I've implemented it deliberately in a near-identical way to how OsmDataLayer performs the painting. Why haven't I made an effort to refactor & share the implementation? It turns out it's just different enough to make that useful. The differences mostly come from JMapViewer's different idea of projections & geometry to that of MapView's.

The hatched texture is fetched from the layer itself via a new accessor method getHatchedTexture(). Why did I not make this a static method when the underlying field is static? Because I can foresee a time when it becomes more common to work with multiple downloaded osm layers (especially now we have overpass support) and hatching colour or style could conceivably be used to differentiate between them (?).

Reasons you may not want to apply this as-is:

It is currently not optional. I was planning to add a control for this to the bottom of the "source button", similar to how openlayers maps (used to) allow optional layers to be controlled. But then I looked at the source of the "source button", and it became clear that this is a horse that has already been flogged a bit far as it is.

So... I propose that before this gets considered for inclusion I give the source button a bit of an overhaul. It's trying to mimic a pattern found in old openlayers maps which will be becoming less and less familiar to users over time and does so in quite an ad-hoc and accessibility-troubled way. My suggestion would be to replace it with a fairly normal-looking JButton showing the fairly widely recognized "layers" symbol which, when pressed, opens a bog-standard JMenu which can expose tile sources through radio menu items or boolean options through toggle menu items. This completely avoids the "your panel isn't very big and you have more than 4 tile sources? good luck choosing the bottom ones." problem by deferring it to swing.

Thoughts?

Attaching patches...

Change History (38)

comment:1 Changed 3 months ago by ris

Description: modified (diff)

comment:2 Changed 3 months ago by ris

Of course, I'm actually using git to manage this so you may prefer to look at the patches in the github interface https://github.com/risicle/josm/compare/22fb521836246588145e80d03fc9b0f9ba1342ff...ris-slippy-map-bbox-dl-area

comment:3 Changed 3 months ago by Don-vip

Interesting idea, could you please attach a screenshot of the result?

comment:4 Changed 3 months ago by bastiK

I think it is good as is and the source button overhaul can be done afterwards.

Just a suggestion on the visual presentation: In the OsmDataLayer, the yellow hatching is like a warning label: "Data in this area is incomplete, don't edit here!". In the download dialog on the other hand, we invite the user to explore the unknown areas rather than keep them out. So it would make sense to have a different visual style.

comment:5 Changed 3 months ago by stoecker

Must it use a new design. Can't it simply use boxes like the imagery preview map uses?

comment:6 Changed 3 months ago by bastiK

Imagery preview is yet another slightly different concept as it shows the finite area of imagery coverage. In contrast, there is no limit to what the user can download successively from the server. This is like wiping a misty window, so I would like a semi-transparent filling of the area that is not downloaded, just drop the yellow hatching.

ris, also have a look at the Mini map toggle dialog and see if it works as intended.

Last edited 3 months ago by bastiK (previous) (diff)

comment:7 Changed 3 months ago by ris

I'll post more later when I'm back at my machine, but the main reason I'm developing this is so that it works better with my markseen plugin, which is largely based on the mini map.

The reason I think it's important to keep the yellow hatching is to maintain consistency between the main map and the mini maps. It's not necessarily obvious what a general darkened area means. It's also hard to tell that an area is shaded at all of an unshaded portion is not visible - it's just a darker map. See the familiar hatching and a user immediately knows what that signifies.

The problem with merging this as-is is that it gives no option to disable the shading, which some users may fond annoying and a regression.

Another subject I meant to mention was tests. I have found it tricky to mock tile fetching out convincingly: see my approach in my markseen plugin - I'm not sure how amenable people would be to adding a jmockit dependency to the tests.

comment:8 in reply to:  7 Changed 3 months ago by bastiK

Replying to ris:

The reason I think it's important to keep the yellow hatching is to maintain consistency between the main map and the mini maps. It's not necessarily obvious what a general darkened area means. It's also hard to tell that an area is shaded at all of an unshaded portion is not visible - it's just a darker map. See the familiar hatching and a user immediately knows what that signifies.

It is not really an improvement for the mini map. What is the point of an overview map, when it is obstructed at all times?

The problem with merging this as-is is that it gives no option to disable the shading, which some users may fond annoying and a regression.

Okay, but marking the downloaded areas is an obvious improvement. It should be possible to find a solution so users don't immediately have the urge to turn off the new feature.

comment:9 Changed 3 months ago by ris

It is not really an improvement for the mini map. What is the point of an overview map, when it is obstructed at all times?

Ah right we're assuming different models of working. You're assuming the method is "download a small area, work on it, then extend it to work on an adjacent area". I'm coming at this having watched many HOT users using josm with the task manager, where the user is handed a large(ish) task area and their job is to fix/map all the stuff in that area. Most of the mapping time is spent zoomed way in in their area and the use of the mini map would be to see where they are within their task tile.

Though I do think I see your point about this having the effect of making the whole bbox selector look like it's "disabled". Hmm.

comment:10 Changed 3 months ago by stoecker

Another idea. Maybe we reverse the effect. A slightly colored shade for the downloaded areas - in another color than yellow, blue for example (I'd wanted to say green, but that's probably no good idea :-)?

comment:11 Changed 3 months ago by bastiK

That would be a solution that works for both the mini map and the download dialog.

comment:12 Changed 3 months ago by ris

Actually I was coming over to your way of thinking and was going to just remove the hatching. I think it would be good to not obstruct the "working area" and have the non-downloaded part shaded.

comment:13 Changed 3 months ago by bastiK

We have to see how it looks in practice.

comment:14 Changed 3 months ago by ris

Watch this space.

comment:15 Changed 3 months ago by ris

Ok, posted a new patch series. What do people think of the opacity value chosen (64)?

Changed 3 months ago by ris

Attachment: v2_download_dialog.png added

Changed 3 months ago by ris

Attachment: v3_minimap.png added

comment:16 Changed 3 months ago by ris

(damn, both of those screenshots are from the v2 patches - my screenshot application auto-incremented the number. annoying.)

comment:17 Changed 3 months ago by Don-vip

I like it :)

comment:18 Changed 3 months ago by stoecker

Screenshot looks good to me as well. We should test it and check what users think. It is not disturbing as far as I can see.

Images for reference:


comment:19 Changed 3 months ago by ris

Ah didn't know we could embed images in the wikitext. Cool.

Yeah, looks ok to me - I'm looking deeper into the source button now. If I'm going to start writing tests for the component, it'd be better to do them once I've finished fiddling around with things.

(now why in god's name did they design ButtonGroups to take a list of AbstractButtons instead of a list of ButtonModels?)

comment:20 Changed 3 months ago by Don-vip

Milestone: 17.10

comment:21 in reply to:  15 Changed 3 months ago by bastiK

We are all set then...

Replying to ris:

Ok, posted a new patch series. What do people think of the opacity value chosen (64)?

It's okay. I wouldn't mind a lower value (32) - still enough contrast, but not as dark.

comment:22 Changed 3 months ago by ris

Cool - I'll just let you do that before final merge.

comment:23 Changed 2 months ago by bastiK

Resolution: fixed
Status: newclosed

In 12927/josm:

applied #15369 - download area display in SlippyMapBBoxChooser (patch by ris)

comment:24 in reply to:  7 Changed 2 months ago by bastiK

Replying to ris:

Another subject I meant to mention was tests. I have found it tricky to mock tile fetching out convincingly: see my approach in my markseen plugin - I'm not sure how amenable people would be to adding a jmockit dependency to the tests.

I have not worked with a Java mocking framework so far, but from what I can tell, it is mostly useful for testing legacy code that must not be touched. Rather than mocking it all out, it should be better to refactor the original code to allow for better testing. If you have a case, where this is impossible or detrimental to the code quality, then we can talk about adding JMockIt or a contender.

comment:25 Changed 2 months ago by ris

Myself I have most of my experience in the python world, but I've found that most things related to mocking are broadly similar. The alternative to using a mocking framework I guess is design of all of the components of an application to be easily interchangeable with dummy components. I think that ends up being quite a burden and there are inevitably areas that don't get designed to be able to handle quite the right mocking out that the tester wants at any time.

There's also a degree to which good mocking can provide an amount of sandboxing of the testing process. I've got to say I was surprised when I saw the josm tests making both deliberate and not-so-deliberate network requests (e.g. trying to retrieve the start page from the wiki). I would normally not allow a test suite anywhere near a real network. There do exist solutions such as WireMock that can be used to emulate specific behaviours of a web server.

This all said, even with a mocking framework it can sometimes be tough to mock some things out due to their design. I had quite a difficult time trying to mock out JMapViewer's tile fetching with plain white tiles in some tests here https://github.com/risicle/josm-markseen/blob/867d1d5468d8b9d8cfa82a20ac62e47393aae47f/test/unit/org/openstreetmap/josm/plugins/markseen/MarkSeenRootTest.java

Anyway, no tests are perfect and some tests are better than no tests.

comment:26 Changed 2 months ago by Klumbumbus

I like this new feature :)

comment:27 Changed 2 months ago by ris

After using it a bit more, I'm wondering again whether a small amount of hatching might make its appearance clearer. Possibly just a limited hatching immediately around the border of the area. The (un)shaded box blends in very well with some backgrounds.

comment:28 in reply to:  25 Changed 2 months ago by Don-vip

Replying to ris:

I've got to say I was surprised when I saw the josm tests making both deliberate and not-so-deliberate network requests (e.g. trying to retrieve the start page from the wiki). I would normally not allow a test suite anywhere near a real network. There do exist solutions such as WireMock that can be used to emulate specific behaviours of a web server.

I have started to use WireMock in #15102. If you see more tests that should be converted, I'd be happy to accept some patches :)

comment:29 Changed 2 months ago by ris

I have started to use WireMock in #15102

Excellent. I'll be right on it.

comment:30 in reply to:  27 Changed 8 weeks ago by Klumbumbus

Replying to ris:

After using it a bit more, I'm wondering again whether a small amount of hatching might make its appearance clearer. Possibly just a limited hatching immediately around the border of the area.

I don't think a hatching would look good. What about a thin grey border similar to the download bbox but less obtrusive. If its color would be adjustable by the user, he could adjust the visibility of the border.

comment:31 Changed 8 weeks ago by ris

All possibilities - at the moment I'm diving deep into the testing infrastructure and WireMock...

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.