Modify

Opened 7 years ago

Closed 7 years ago

Last modified 7 years 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 7 years ago.
dd-expert-mode-overpass.png (505.6 KB ) - added by bafonins 7 years ago.
dd-normal-mode.png (786.9 KB ) - added by bafonins 7 years ago.
merge-download-dialog.patch (80.6 KB ) - added by bafonins 7 years ago.
Here is the patch.
merge-download-dialog-v2.patch (83.9 KB ) - added by bafonins 7 years ago.
added label to check download area
space.png (210.4 KB ) - added by Don-vip 7 years ago.
merge-download-dialog-v4.patch (17.5 KB ) - added by bafonins 7 years 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 7 years ago.
Download-dlg-before.png (142.7 KB ) - added by bastiK 7 years ago.
Download-dlg-now.png (102.5 KB ) - added by bastiK 7 years ago.
merge-download-dialog-v5.patch (18.4 KB ) - added by bafonins 7 years ago.
moved the info label in non expert mode.
download12690.png (72.6 KB ) - added by Klumbumbus 7 years ago.
downloading or uploading.PNG (24.2 KB ) - added by SanderH 7 years ago.
downloading or uploading

Change History (72)

by bafonins, 7 years ago

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

by bafonins, 7 years ago

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

by bafonins, 7 years ago

Attachment: dd-normal-mode.png added

comment:1 by Don-vip, 7 years ago

Priority: normalmajor

looks very nice! :)

comment:2 by Klumbumbus, 7 years ago

I like it too.

by bafonins, 7 years ago

Attachment: merge-download-dialog.patch added

Here is the patch.

comment:3 by bafonins, 7 years ago

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 by michael2402, 7 years ago

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

comment:5 by stoecker, 7 years ago

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

comment:6 by Don-vip, 7 years ago

no objection, go on :)

comment:7 by Don-vip, 7 years ago

Milestone: 17.08

comment:8 by Don-vip, 7 years ago

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

by bafonins, 7 years ago

added label to check download area

comment:9 by michael2402, 7 years ago

Resolution: fixed
Status: newclosed

In 12652/josm:

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

comment:10 by michael2402, 7 years ago

In 12653/josm:

See #15167: Convert to iterator based for loop.

comment:11 by michael2402, 7 years ago

In 12654/josm:

See #15167: Fix generics.

comment:12 by michael2402, 7 years ago

In 12655/josm:

See #15167: Add javadoc, improve code style.

comment:13 by Don-vip, 7 years ago

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 7 years ago by Don-vip (previous) (diff)

by Don-vip, 7 years ago

Attachment: space.png added

comment:14 by Don-vip, 7 years ago

Resolution: fixed
Status: closedreopened

comment:15 by Klumbumbus, 7 years ago

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 by Klumbumbus, 7 years ago

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

Anyone has an idea which additional sources might be available?

by bafonins, 7 years ago

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

in reply to:  13 comment:17 by bafonins, 7 years ago

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 by Don-vip, 7 years ago

Windows 10. What's yours?

by Klumbumbus, 7 years ago

Attachment: flicker.gif added

comment:19 by Klumbumbus, 7 years ago

Flickering see attachment:flicker.gif

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

in reply to:  19 comment:20 by bafonins, 7 years ago

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

in reply to:  18 comment:21 by bafonins, 7 years ago

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

Replying to Don-vip:

Windows 10. What's yours?

in reply to:  19 comment:22 by michael2402, 7 years ago

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 by Klumbumbus, 7 years ago

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 by Klumbumbus, 7 years ago

I updated the docu: Help/Action/Download

by bastiK, 7 years ago

Attachment: Download-dlg-before.png added

by bastiK, 7 years ago

Attachment: Download-dlg-now.png added

comment:25 by bastiK, 7 years ago

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 by Don-vip, 7 years ago

The flickering on Windows is blocking too.

by bafonins, 7 years ago

moved the info label in non expert mode.

in reply to:  25 comment:27 by bafonins, 7 years ago

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]

in reply to:  26 ; comment:28 by bafonins, 7 years ago

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 by bastiK, 7 years ago

In 12684/josm:

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

comment:30 by bastiK, 7 years ago

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)

in reply to:  28 comment:31 by bastiK, 7 years ago

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.

in reply to:  25 comment:32 by Klumbumbus, 7 years ago

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.

by Klumbumbus, 7 years ago

Attachment: download12690.png added

comment:33 by Klumbumbus, 7 years ago

(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 by Don-vip, 7 years ago

+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 by bastiK, 7 years ago

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 by Don-vip, 7 years ago

In 12694/josm:

see #15167 - checkstyle

comment:37 by michael2402, 7 years ago

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

in reply to:  37 ; comment:38 by Don-vip, 7 years ago

Replying to michael2402:

I'll have a look at it tomorrow

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

in reply to:  38 comment:39 by michael2402, 7 years ago

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 by bastiK, 7 years ago

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

comment:41 by michael2402, 7 years ago

In 12705/josm:

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

comment:42 by michael2402, 7 years ago

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 by stoecker, 7 years ago

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

in reply to:  43 ; comment:44 by anonymous, 7 years ago

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 by stoecker, 7 years ago

Ah ok.

in reply to:  44 comment:46 by bafonins, 7 years ago

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

by SanderH, 7 years ago

downloading or uploading

comment:47 by SanderH, 7 years ago

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 7 years ago by Don-vip (previous) (diff)

in reply to:  47 comment:48 by michael2402, 7 years ago

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 by Don-vip, 7 years ago

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 by Don-vip, 7 years ago

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 by Don-vip, 7 years ago

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 by bastiK, 7 years ago

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

comment:53 by michael2402, 7 years ago

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 ;-)

in reply to:  53 ; comment:54 by SanderH, 7 years ago

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.

in reply to:  53 comment:55 by Klumbumbus, 7 years ago

Replying to michael2402:

of what sources to add ;-)

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

in reply to:  54 comment:56 by michael2402, 7 years ago

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

in reply to:  52 comment:57 by Klumbumbus, 7 years ago

Replying to bastiK:

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

12712 is a nice number for releasing :)

in reply to:  53 comment:58 by bastiK, 7 years ago

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 by Don-vip, 7 years ago

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.