Modify

Opened 5 weeks ago

Closed 3 weeks ago

Last modified 2 weeks ago

#15167 closed enhancement (fixed)

[Patch] Download Dialog: UI cleanup

Reported by: bafonins Owned by: team
Priority: major Milestone: 17.08
Component: Core Version:
Keywords: download dialog overpass osm Cc:

Description

Hi everyone! I would like to share the result of my work on the download dialog. The task was to clean the existing codebase and develop new, generic way to add any download sources to JOSM, as well as collect all download related functionality at one place having only a single entry point.

This is how it looks in the normal mode :


The main features from the old dialog are still present, except for the area check. But this can be added later.

This is the view in the expert mode with the Overpass tab opened :

All features from #15057 are present.

This is the view in the expert mode with the OSM tab opened :

As you can see the pane dynamically changes according to the user selected mode.
The new interface allows to embed any external download source easily, even through plugins.
The code for download dialog is cleaner.

Will add the patch a bit later. I would like to hear thoughts about the new UI from people who use JOSM extensively. Any feedback is appreciated.

Attachments (13)

dd-expert-mode-osm.png (544.6 KB) - added by bafonins 5 weeks ago.
dd-expert-mode-overpass.png (505.6 KB) - added by bafonins 5 weeks ago.
dd-normal-mode.png (786.9 KB) - added by bafonins 5 weeks ago.
merge-download-dialog.patch (80.6 KB) - added by bafonins 4 weeks ago.
Here is the patch.
merge-download-dialog-v2.patch (83.9 KB) - added by bafonins 4 weeks ago.
added label to check download area
space.png (210.4 KB) - added by Don-vip 4 weeks ago.
merge-download-dialog-v4.patch (17.5 KB) - added by bafonins 4 weeks ago.
refactored some code, fixed docs, removed redundant code according to Michaels changes, each download source has its own size
flicker.gif (1.8 MB) - added by Klumbumbus 4 weeks ago.
Download-dlg-before.png (142.7 KB) - added by bastiK 3 weeks ago.
Download-dlg-now.png (102.5 KB) - added by bastiK 3 weeks ago.
merge-download-dialog-v5.patch (18.4 KB) - added by bafonins 3 weeks ago.
moved the info label in non expert mode.
download12690.png (72.6 KB) - added by Klumbumbus 3 weeks ago.
downloading or uploading.PNG (24.2 KB) - added by SanderH 3 weeks ago.
downloading or uploading

Change History (72)

Changed 5 weeks ago by bafonins

Attachment: dd-expert-mode-osm.png added

Changed 5 weeks ago by bafonins

Attachment: dd-expert-mode-overpass.png added

Changed 5 weeks ago by bafonins

Attachment: dd-normal-mode.png added

comment:1 Changed 5 weeks ago by Don-vip

Priority: normalmajor

looks very nice! :)

comment:2 Changed 5 weeks ago by Klumbumbus

I like it too.

Changed 4 weeks ago by bafonins

Attachment: merge-download-dialog.patch added

Here is the patch.

comment:3 Changed 4 weeks ago by bafonins

It would be great to get some feedback, since the download dialog is pretty important component I it might be the case that I am missing something, or removed something that is not supposed to be removed. Also, remarks on the design are highly appreciated.
Some remarks :
OverpassDownloadAction is removed, since within this patch all download sources reside in the same dialog. It can still be added if necessary by simply extending the download dialog with additional methods to set initial opening tab.
As mentioned earlier, the label for area size check is not present.

comment:4 Changed 4 weeks ago by michael2402

I'd like to merge this today, if there are no other objections ;-).

comment:5 Changed 4 weeks ago by stoecker

Fine, if you fix this: "As mentioned earlier, the label for area size check is not present."

comment:6 Changed 4 weeks ago by Don-vip

no objection, go on :)

comment:7 Changed 4 weeks ago by Don-vip

Milestone: 17.08

comment:8 Changed 4 weeks ago by Don-vip

Summary: Download Dialog: UI cleanup[Patch] Download Dialog: UI cleanup

Changed 4 weeks ago by bafonins

added label to check download area

comment:9 Changed 4 weeks ago by michael2402

Resolution: fixed
Status: newclosed

In 12652/josm:

Apply #15167: Merge OSM and overpass download dialog. Patch by bafonins

comment:10 Changed 4 weeks ago by michael2402

In 12653/josm:

See #15167: Convert to iterator based for loop.

comment:11 Changed 4 weeks ago by michael2402

In 12654/josm:

See #15167: Fix generics.

comment:12 Changed 4 weeks ago by michael2402

In 12655/josm:

See #15167: Add javadoc, improve code style.

comment:13 Changed 4 weeks ago by Don-vip

Two problems:

  1. The vertical size of top panel is initially too important, a lot of space is wasted
  2. Refresh problem when the dialog is resized: the whole dialog blinks, this is very disturbing


Last edited 4 weeks ago by Don-vip (previous) (diff)

Changed 4 weeks ago by Don-vip

Attachment: space.png added

comment:14 Changed 4 weeks ago by Don-vip

Resolution: fixed
Status: closedreopened

comment:15 Changed 4 weeks ago by Klumbumbus

It would be useful if every source tab remebers its own position of the separator bar above the map, so it changes automatically when swithing the source tabs as shown on the screenshots in the ticket description.
Right now when you move the separator bar up in the OSM tab and then switch to the Overpass tab you need to move it down again to see something.

comment:16 Changed 4 weeks ago by Klumbumbus

The new interface allows to embed any external download source easily, even through plugins.

Anyone has an idea which additional sources might be available?

Changed 4 weeks ago by bafonins

refactored some code, fixed docs, removed redundant code according to Michaels changes, each download source has its own size

comment:17 in reply to:  13 Changed 4 weeks ago by bafonins

Hm, thats weird. Does not happen to me. On what operating system does it happen?

Replying to Don-vip:

Two problems:

  1. The vertical size of top panel is initially too important, a lot of space is wasted
  2. Refresh problem when the dialog is resized: the whole dialog blinks, this is very disturbing


comment:18 Changed 4 weeks ago by Don-vip

Windows 10. What's yours?

Changed 4 weeks ago by Klumbumbus

Attachment: flicker.gif added

comment:19 Changed 4 weeks ago by Klumbumbus

Flickering see attachment:flicker.gif

(The flickering happens to me also with the old overpass window.)

comment:20 in reply to:  19 Changed 4 weeks ago by bafonins

Oh, that looks awful. Will check it

Replying to Klumbumbus:

Flickering see attachment:flicker.gif

(The flickering happens to me also with the old overpass window.)

comment:21 in reply to:  18 Changed 4 weeks ago by bafonins

os x. Will have access to windows machine tomorrow, will check it

Replying to Don-vip:

Windows 10. What's yours?

comment:22 in reply to:  19 Changed 4 weeks ago by michael2402

Replying to Klumbumbus:

Flickering see attachment:flicker.gif

This might be a JDK bug. It is not happening on Linux. But I did not find any matching bug report. Which version are you using?

comment:23 Changed 4 weeks ago by Klumbumbus

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-08-26 15:29:22 +0200 (Sat, 26 Aug 2017)
Build-Date:2017-08-26 14:04:15
Revision:12666
Relative:URL: ^/trunk

Identification: JOSM/1.5 (12666 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1703 (15063)
Memory Usage: 1930 MB / 3641 MB (1223 MB allocated, but free)
Java version: 1.8.0_144-b01, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1680x1050
Maximum Screen Size: 1680x1050
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=C:\Program Files (x86)\josm-latest-mehr-RAM.jnlp, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=1024m,4096m, -Djnlpx.splashport=52724, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Plugins:
+ AddrInterpolation (33160)
+ DirectDownload (33160)
+ DirectUpload (33182)
+ FastDraw (33182)
+ HouseNumberTaggingTool (33160)
+ Mapillary (v1.5.5)
+ OpeningHoursEditor (33185)
+ ShapeTools (1220)
+ Tracer2 (33004)
+ alignways (33182)
+ apache-commons (33517)
+ apache-http (32699)
+ buildings_tools (33004)
+ contourmerge (1030)
+ editgpx (33004)
+ imagery-xml-bounds (33496)
+ imagery_offset_db (33316)
+ jogl (1.1.0)
+ log4j (32699)
+ measurement (33088)
+ osm-obj-info (1484152384)
+ photo_geotagging (33494)
+ photoadjust (33303)
+ reltoolbox (33530)
+ reverter (33088)
+ tag2link (33382)
+ tageditor (33021)
+ tagging-preset-tester (33004)
+ terracer (33088)
+ turnlanes-tagging (254)
+ turnrestrictions (33537)
+ undelete (33480)
+ utilsplugin2 (33543)
+ wikipedia (33541)

comment:24 Changed 4 weeks ago by Klumbumbus

I updated the docu: Help/Action/Download

Changed 3 weeks ago by bastiK

Attachment: Download-dlg-before.png added

Changed 3 weeks ago by bastiK

Attachment: Download-dlg-now.png added

comment:25 Changed 3 weeks ago by bastiK

This is with fresh preferences:

Before (12545):

Now (12681):

As you can see, the explanation text is cut off, with strange vertical placement. I would prefer the warning text to go on the bottom (as before), but its no big deal, as long as the top panel is properly sized.

In the "Download from OSM" tab, the JSplitPane is unnecessary, as the size of the top panel is fixed. I would argue, a JTabbedPane with a single tab makes no sense. So in non-expert mode, we could do without it.

The only point that is a blocker for the release is the excessive initial size of the top area. (Sorry for being so critical, it's actually great work.)

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-08-28 00:23:19 +0200 (Mon, 28 Aug 2017)
Revision:12681
Build-Date:2017-08-28 01:33:34
URL:http://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (12681 en) Linux Ubuntu 17.04
Memory Usage: 371 MB / 1984 MB (168 MB allocated, but free)
Java version: 9-Ubuntu+0-9b161-1, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1366x768
Maximum Screen Size: 1366x768
Java package: openjdk-9-jre:amd64-9~b161-1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-13
VM arguments: [-Djosm.restart=true, -Djosm.dir.name=JOSM-latest, -Djava.net.useSystemProxies=true, -Djosm.home=pref-josm-l2]

comment:26 Changed 3 weeks ago by Don-vip

The flickering on Windows is blocking too.

Changed 3 weeks ago by bafonins

moved the info label in non expert mode.

comment:27 in reply to:  25 Changed 3 weeks ago by bafonins

The latest patch solves the issue with separating download sources and download selections. By default each download source uses minimum size, but this can be changed by the user.
I agree that JSplitPane is not really needed in the 'Download from OSM' tab, but it might be needed for other download sources and removing/adding it depending on the mode/tab is not so trivial. Finally, it can be ignored by the user if the dialog is sized properly.

Replying to bastiK:

This is with fresh preferences:

Before (12545):

Now (12681):

As you can see, the explanation text is cut off, with strange vertical placement. I would prefer the warning text to go on the bottom (as before), but its no big deal, as long as the top panel is properly sized.

In the "Download from OSM" tab, the JSplitPane is unnecessary, as the size of the top panel is fixed. I would argue, a JTabbedPane with a single tab makes no sense. So in non-expert mode, we could do without it.

The only point that is a blocker for the release is the excessive initial size of the top area. (Sorry for being so critical, it's actually great work.)

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-08-28 00:23:19 +0200 (Mon, 28 Aug 2017)
Revision:12681
Build-Date:2017-08-28 01:33:34
URL:http://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (12681 en) Linux Ubuntu 17.04
Memory Usage: 371 MB / 1984 MB (168 MB allocated, but free)
Java version: 9-Ubuntu+0-9b161-1, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1366x768
Maximum Screen Size: 1366x768
Java package: openjdk-9-jre:amd64-9~b161-1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-13
VM arguments: [-Djosm.restart=true, -Djosm.dir.name=JOSM-latest, -Djava.net.useSystemProxies=true, -Djosm.home=pref-josm-l2]

comment:28 in reply to:  26 ; Changed 3 weeks ago by bafonins

After some testing on win8 it turned out that the reason that causes flickering is in the overriden paint method in DownloadDialog. The comment for the method is not really descriptive. Any ideas?
If I remove it, the dialog resizes without any problems.

Replying to Don-vip:

The flickering on Windows is blocking too.

comment:29 Changed 3 weeks ago by bastiK

In 12684/josm:

see #15167 - moved the info label in non expert mode (patch by bafonins)

comment:30 Changed 3 weeks ago by bastiK

In 12685/josm:

see #15167, see #6720 - remove workaround that causes flickering on resize (idea by bafonins)

(hopefully, the original problem is no longer present)

comment:31 in reply to:  28 Changed 3 weeks ago by bastiK

Replying to bafonins:

After some testing on win8 it turned out that the reason that causes flickering is in the overriden paint method in DownloadDialog. The comment for the method is not really descriptive. Any ideas?
If I remove it, the dialog resizes without any problems.

We will have to see, if there are now problems as reported in #6720. In a quick test, it was okay.

comment:32 in reply to:  25 Changed 3 weeks ago by Klumbumbus

Replying to bastiK:

I would prefer the warning text to go on the bottom (as before)

I think it is better on top as this warning is specific to the source (i.e. download from osm). When you switch to the download from overpass api tab this warning is not relevant.

Changed 3 weeks ago by Klumbumbus

Attachment: download12690.png added

comment:33 Changed 3 weeks ago by Klumbumbus

(r12690 with fresh preferences)
I think the initial size of the top panel of the download from osm tab is still too high, especially if compared with the old dialog.

comment:34 Changed 3 weeks ago by Don-vip

+1, it could be lowered a little bit. But it's much better now, the flickering issue is gone, and if we manually reduce the size of top panel, it's remembered :)

comment:35 Changed 3 weeks ago by bastiK

Regarding the line

setMinimumSize(new Dimension(450, 115));

in [12684]: I don't think it is a good idea to set the size in pixels. The minimum height should be determined by the layout manager instead.

comment:36 Changed 3 weeks ago by Don-vip

In 12694/josm:

see #15167 - checkstyle

comment:37 Changed 3 weeks ago by michael2402

I was traveling this week so I had not time to pick up with it.

I'll have a look at it tomorrow and fix the sizing of the top panels (so that they use the right initial size).

comment:38 in reply to:  37 ; Changed 3 weeks ago by Don-vip

Replying to michael2402:

I'll have a look at it tomorrow

Is it ok for tonight? Only a few minutes remain for August :)

comment:39 in reply to:  38 Changed 3 weeks ago by michael2402

Replying to Don-vip:

Replying to michael2402:

I'll have a look at it tomorrow

Is it ok for tonight? Only a few minutes remain for August :)

I'm already on it. I'm glad we did not agree on a time zone :D

comment:40 Changed 3 weeks ago by bastiK

Today officially ends 3:30 CEST, when cron kicks off the nightly build.

comment:41 Changed 3 weeks ago by michael2402

In 12705/josm:

See #15167: Make size of the OSM download panel fixed, only allow resizing for overpass.

comment:42 Changed 3 weeks ago by michael2402

In 12706/josm:

See #15167: Store the name of the current selected tab.

This also makes the expert mode tab selection more universal

comment:43 Changed 3 weeks ago by stoecker

Remembering the translated name is not correct. It should remember the original English name instead.

P.S. Our release dates are not made of stone. They are recommendations...

comment:44 in reply to:  43 ; Changed 3 weeks ago by anonymous

Simple name of a download source is not translated and is not visible to the user.

Replying to stoecker:

Remembering the translated name is not correct. It should remember the original English name instead.

P.S. Our release dates are not made of stone. They are recommendations...

comment:45 Changed 3 weeks ago by stoecker

Ah ok.

comment:46 in reply to:  44 Changed 3 weeks ago by bafonins

sorry, forgot to login

Replying to anonymous:

Simple name of a download source is not translated and is not visible to the user.

Replying to stoecker:

Remembering the translated name is not correct. It should remember the original English name instead.

P.S. Our release dates are not made of stone. They are recommendations...

Changed 3 weeks ago by SanderH

downloading or uploading

comment:47 Changed 3 weeks ago by SanderH

In version 12707 (and a few versions before) I noticed that the download dialog mentions 'Uploading' instead of 'Downloading'.
Don't know if this deserves a separate ticket or if it's related to this one.

downloading or uploading

Last edited 3 weeks ago by Don-vip (previous) (diff)

comment:48 in reply to:  47 Changed 3 weeks ago by michael2402

Replying to SanderH:

In version 12707 (and a few versions before) I noticed that the download dialog mentions 'Uploading' instead of 'Downloading'.
Don't know if this deserves a separate ticket or if it's related to this one.

This message is not really correct, we should change it to Requesting.

comment:49 Changed 3 weeks ago by Don-vip

Where does the string come from? I can find only two occurrences in OsmServerWriter and ProgressOutputStream. Nothing related to download at first glance.

comment:50 Changed 3 weeks ago by Don-vip

OK got it: unexpected side effect of the switch from GET to POST method for Overpass API download in r12596 (#15141). I look into it, thank you for reporting this issue.

comment:51 Changed 3 weeks ago by Don-vip

In 12711/josm:

see #15141, see #15167 - use correct message ("Downloading data" instead of "Uploading data") when downloading data from Overpass API / POST

comment:52 Changed 3 weeks ago by bastiK

Are there any outstanding issues in this ticket (i.e. release critical)?

comment:53 Changed 3 weeks ago by michael2402

None that I have encountered. There is no Plugin API yet to register own sources, but this can wait until someone has a nice idea of what sources to add ;-)

comment:54 in reply to:  53 ; Changed 3 weeks ago by SanderH

Replying to michael2402:

There is no Plugin API yet to register own sources, but this can wait until someone has a nice idea of what sources to add ;-)

In The Netherlands we have a plugin (https://github.com/gidema/josm-ods-bag/tree/0.6-dev) to download building and address information from the government and others may use similar plugins.
I think it would be nice if this kind of plugins could be integrated in the standard download dialog. At least that would give a consistent way of downloading data from multiple sources from one central starting point.

comment:55 in reply to:  53 Changed 3 weeks ago by Klumbumbus

Replying to michael2402:

of what sources to add ;-)

I don't know if http://overpass.osm.rambler.ru/cgi/xapi? is still active (MirroredDownloadInfo)

comment:56 in reply to:  54 Changed 3 weeks ago by michael2402

Replying to SanderH:

In The Netherlands we have a plugin (https://github.com/gidema/josm-ods-bag/tree/0.6-dev) to download building and address information from the government and others may use similar plugins.

That's a nice one to get started.

I posted them an issue. https://github.com/gidema/josm-ods-bag/issues/26

comment:57 in reply to:  52 Changed 3 weeks ago by Klumbumbus

Replying to bastiK:

Are there any outstanding issues in this ticket (i.e. release critical)?

12712 is a nice number for releasing :)

comment:58 in reply to:  53 Changed 3 weeks ago by bastiK

Resolution: fixed
Status: reopenedclosed

Replying to michael2402:

None that I have encountered. There is no Plugin API yet to register own sources, but this can wait until someone has a nice idea of what sources to add ;-)

Can be done in a new ticket.

comment:59 Changed 2 weeks ago by Don-vip

In 12733/josm:

see #15167 - sonar - pmd:ImmutableField - Immutable Field

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.