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

[taylor.smock]

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

[taylor.smock]

Is there any interest in this?

[simon04]

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.

[taylor.smock]

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

[taylor.smock]

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.

[taylor.smock]

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.

[taylor.smock]

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)

[taylor.smock]

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

[taylor.smock]

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

[simon04]

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)

[simon04]

In 16504/josm:

see #18340 - OSMDownloadSource: refactoring, improve i18n

[simon04]

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

[Klumbumbus]

There are some "since xxx" left.

[taylor.smock]

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

[GerdP]

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.

[GerdP]

popup with data sources

[GerdP]

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

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

[skyper]

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

[taylor.smock]

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!";
  }

[simon04]

In 16517/josm:

see #18340 - fix @since xxx

[simon04]

In 16518/josm:

see #18340 - OSMDownloadSource: improve i18n

[simon04]

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

[Klumbumbus]

German is now complete too.