Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18340 closed enhancement (fixed)

[RFC PATCH] Allow additional sources of data to be downloaded at the same time as OpenStreetMap data sources

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 20.05
Component: Core Version:
Keywords: download Cc:

Description

In the event that there is a plugin with additional data (e.g., MapWithAI), it would be nice if the plugin could register a download task so that the user can more easily download the additional data.

The attached patch does the following in trunk/src/org/openstreetmap/josm/gui/download/OSMDownloadSource.java:

  • Add an enclosed class (DataDownloadType) that holds a boolean property (is it enabled?), the download task class, and the checkbox to be added to the Download UI.
    • Add a function to determine if the download area is too large (the bound is passed instead of an area, in case a service has max lengths for sides instead of max areas)
  • Add a getter, adder, and remover for the List that holds DataDownloadType. The built in types can never be removed or added (after initialization)
  • Add download tasks to worker asynchronously (probably no real difference)

I still need to write tests (it actually looks like there are not any tests explicitly looking at the file).

Attachments (5)

18340.patch (21.5 KB ) - added by taylor.smock 4 years ago.
18340.1.patch (21.4 KB ) - added by taylor.smock 4 years ago.
Update patch from r15528 to r16500. This is just so the patch applies cleanly. No other changes have been made. Renaming/translations/etc. will be in the next patch.
18340.2.patch (22.9 KB ) - added by taylor.smock 4 years ago.
Replace OSMDownloadData with List<IDownloadSourceType> (IDownloadSourceType was the DataDownloadType interface, and is now in its own file), rename DOWNLOAD_POSSIBILITIES to DOWNLOAD_SOURCES, change text to use None of, add a method to get a specific instance of a download type (so people can ask for an instance and check if it is enabled), use stream instead of parallelStream.
18340.4.patch (24.4 KB ) - added by taylor.smock 4 years ago.
Public interface, make some methods static, new JPanel that is updated every time the window is opened (to account for new/removed plugins)
popup.PNG (7.7 KB ) - added by GerdP 4 years ago.
popup with data sources

Download all attachments as: .zip

Change History (23)

by taylor.smock, 4 years ago

Attachment: 18340.patch added

comment:1 by taylor.smock, 4 years ago

Is there any interest in this?

Last edited 4 years ago by taylor.smock (previous) (diff)

comment:2 by simon04, 4 years ago

I like the abstraction. The patch does not apply cleanly any more. Some remarks/thoughts:

  • Naming: a priori I have troubles to tell what DownloadSource, OSMDownloadData, DataDownloadType are exactly about. Can we merge OSMDownloadData, DataDownloadType? Do we find a better name for DataDownloadType – is it a Download(Sub)Source(Part/Fragment/Type)?
  • The constant DOWNLOAD_POSSIBILITIES should be renamed accordingly.
  • Should we put the interface DataDownloadType (possibly with a different name) in a separate file? Now, the class is >500 lines.
  • I18n: the neither/nor translation (from OSMDownloadSourcePanel#checkDownload) will not work for many languages. Can we simply use "None of"?
  • Maybe we should add getDownloadType(DataDownloadType type) for a clearer implementation of isDownloadOsmData()
  • removeDownloadType, addDownloadType – we should throw an IllegalArgumentException instead of silently ignoring
  • Currently we've rarely been using parallelStream() in the codebase. I don't see a need for the presumably very low number of registered DOWNLOAD_POSSIBILITIES.

in reply to:  2 comment:3 by taylor.smock, 4 years ago

Replying to simon04:

I like the abstraction. The patch does not apply cleanly any more. Some remarks/thoughts:

I kind of figured it wouldn't apply cleanly anymore (it has been six months). I just wanted to make certain that it was something that might be wanted, before I dusted it off.

  • Naming: a priori I have troubles to tell what DownloadSource, OSMDownloadData, DataDownloadType are exactly about. Can we merge OSMDownloadData, DataDownloadType? Do we find a better name for DataDownloadType – is it a Download(Sub)Source(Part/Fragment/Type)?

I'll look into that.

  • The constant DOWNLOAD_POSSIBILITIES should be renamed accordingly.
  • Should we put the interface DataDownloadType (possibly with a different name) in a separate file? Now, the class is >500 lines.
  • I18n: the neither/nor translation (from OSMDownloadSourcePanel#checkDownload) will not work for many languages. Can we simply use "None of"?

I have no problem changing stuff to make it easier for translaters.

  • Maybe we should add getDownloadType(DataDownloadType type) for a clearer implementation of isDownloadOsmData()
  • removeDownloadType, addDownloadType – we should throw an IllegalArgumentException instead of silently ignoring
  • Currently we've rarely been using parallelStream() in the codebase. I don't see a need for the presumably very low number of registered DOWNLOAD_POSSIBILITIES.

Fair enough. I like parallelStream since it can be faster, but for a low number (say < 100), probably not worth it. :)

by taylor.smock, 4 years ago

Attachment: 18340.1.patch added

Update patch from r15528 to r16500. This is just so the patch applies cleanly. No other changes have been made. Renaming/translations/etc. will be in the next patch.

by taylor.smock, 4 years ago

Attachment: 18340.2.patch added

Replace OSMDownloadData with List<IDownloadSourceType> (IDownloadSourceType was the DataDownloadType interface, and is now in its own file), rename DOWNLOAD_POSSIBILITIES to DOWNLOAD_SOURCES, change text to use None of, add a method to get a specific instance of a download type (so people can ask for an instance and check if it is enabled), use stream instead of parallelStream.

in reply to:  2 comment:4 by taylor.smock, 4 years ago

Replying to simon04:

I like the abstraction. The patch does not apply cleanly any more. Some remarks/thoughts:
[...]

  • removeDownloadType, addDownloadType – we should throw an IllegalArgumentException instead of silently ignoring

[...]

So you feel that checking the return value (true if added/removed from the list) is not enough, and we should instead be throwing an explicit exception?

So, it would be

    public boolean removeDownloadType(IDownloadSourceType type) {
        boolean modified = false;
        if (!(type instanceof OsmDataDownloadType) && !(type instanceof GpsDataDownloadType)
                && !(type instanceof NotesDataDownloadType)) {
            modified = DOWNLOAD_SOURCES.remove(type);
        } else {
            throw new IllegalArgumentException("You cannot use a JOSM core class OR a class that was previously added."); // Needs better wording.
        }
        return modified;
    }

instead of

    public boolean removeDownloadType(IDownloadSourceType type) {
        boolean modified = false;
        if (!(type instanceof OsmDataDownloadType) && !(type instanceof GpsDataDownloadType)
                && !(type instanceof NotesDataDownloadType)) {
            modified = DOWNLOAD_SOURCES.remove(type);
        }
        return modified;
    }

(There would be some additional modifications besides the else, but this has minimal code changes for comparison)

Last edited 4 years ago by taylor.smock (previous) (diff)

by taylor.smock, 4 years ago

Attachment: 18340.4.patch added

Public interface, make some methods static, new JPanel that is updated every time the window is opened (to account for new/removed plugins)

comment:5 by taylor.smock, 4 years ago

I've got a partial implementation I'm working on in the MapWithAI plugin. I think the 18340.4.patch is complete, except for (possibly) the IllegalArgumentException.

Everything is being called correctly, the additional checkbox shows up/disappears appropriately (i.e., plugin added/removed).

comment:6 by simon04, 4 years ago

Resolution: fixed
Status: newclosed

In 16503/josm:

fix #18340 - Allow additional sources of data to be downloaded at the same time as OpenStreetMap data sources (patch by taylor.smock)

comment:7 by simon04, 4 years ago

In 16504/josm:

see #18340 - OSMDownloadSource: refactoring, improve i18n

comment:8 by simon04, 4 years ago

Milestone: 20.05

I applied your patch verbatim and did a few changes in the separate commit r16504. Thanks for your work!

comment:9 by Klumbumbus, 4 years ago

There are some "since xxx" left.

in reply to:  8 comment:10 by taylor.smock, 4 years ago

Replying to simon04:

I applied your patch verbatim and did a few changes in the separate commit r16504. Thanks for your work!

Thanks. I took a look, and I should have made some of those changes when I was modifying things (specifically, the isDownloadXXX methods).

comment:11 by GerdP, 4 years ago

These I18N strings are difficult to translate:
None of {0} is enabled!
Please select at least one of {0}.
I think they are too generic.

Last edited 4 years ago by GerdP (previous) (diff)

by GerdP, 4 years ago

Attachment: popup.PNG added

popup with data sources

comment:12 by GerdP, 4 years ago

The corresponding popup is also difficult to read even in English:

popup with data sources
I suggest No data source was selected. Please enable at least one!
or maybe No download source was selected. Please enable at least one!

Last edited 4 years ago by GerdP (previous) (diff)

comment:13 by skyper, 4 years ago

Is there a release planned for this weekend, as i18 changes usually need some days until translation is up to date.

comment:14 by taylor.smock, 4 years ago

We could add the modified message with a marktr somewhere, and make the decision the day of the release?

Alternatively, we could do something like this:

  if ("en".equalsIgnoreCase(LanguageInfo.getJOSMLocaleCode()) || !"text".equals(tr("text"))) {
    message = "No data source was selected. Please enable at least one!";
  } else {
    message = "None of {0} is enabled!";
  }

comment:15 by simon04, 4 years ago

In 16517/josm:

see #18340 - fix @since xxx

comment:16 by simon04, 4 years ago

In 16518/josm:

see #18340 - OSMDownloadSource: improve i18n

in reply to:  13 comment:17 by simon04, 4 years ago

Replying to skyper:

Is there a release planned for this weekend, as i18 changes usually need some days until translation is up to date.

Yes, I'd like to get a release out. However, i18n is far from complete (only one language is 100% at the moment) – https://translations.launchpad.net/josm/trunk/+pots/josm

comment:18 by Klumbumbus, 4 years ago

German is now complete too.

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.