#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 )
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...
Attachments (7)
Change History (38)
by , 6 years ago
Attachment: | 0001-SlippyMapBBoxChooser-should-probably-paint-through-p.patch added |
---|
by , 6 years ago
Attachment: | 0002-OsmDataLayer-add-getHatchedTexture-accessor-method.patch added |
---|
by , 6 years ago
Attachment: | 0003-SlippyMapBBoxChooser-paint-extents-of-downloaded-are.patch added |
---|
comment:1 by , 6 years ago
Description: | modified (diff) |
---|
comment:2 by , 6 years ago
comment:4 by , 6 years ago
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 by , 6 years ago
Must it use a new design. Can't it simply use boxes like the imagery preview map uses?
comment:6 by , 6 years ago
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.
follow-ups: 8 24 comment:7 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
That would be a solution that works for both the mini map and the download dialog.
comment:12 by , 6 years ago
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.
by , 6 years ago
Attachment: | v2-0001-SlippyMapBBoxChooser-should-probably-paint-through-p.patch added |
---|
by , 6 years ago
Attachment: | v2-0002-SlippyMapBBoxChooser-paint-extents-of-downloaded-are.patch added |
---|
follow-up: 21 comment:15 by , 6 years ago
Ok, posted a new patch series. What do people think of the opacity value chosen (64)?
by , 6 years ago
Attachment: | v2_download_dialog.png added |
---|
by , 6 years ago
Attachment: | v3_minimap.png added |
---|
comment:16 by , 6 years ago
(damn, both of those screenshots are from the v2 patches - my screenshot application auto-incremented the number. annoying.)
comment:18 by , 6 years ago
comment:19 by , 6 years ago
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 ButtonGroup
s to take a list of AbstractButton
s instead of a list of ButtonModel
s?)
comment:20 by , 6 years ago
Milestone: | → 17.10 |
---|
comment:21 by , 6 years ago
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:24 by , 6 years ago
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.
follow-up: 28 comment:25 by , 6 years ago
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.
follow-up: 30 comment:27 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
I have started to use WireMock in #15102
Excellent. I'll be right on it.
comment:30 by , 6 years ago
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 by , 6 years ago
All possibilities - at the moment I'm diving deep into the testing infrastructure and WireMock...
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