Modify

Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#16850 closed enhancement (fixed)

[PATCH] Make overriding `DownloadOsmTask.AbstractInternalTask.createNewLayer()` methods easier

Reported by: floscher Owned by: Don-vip
Priority: normal Milestone: 18.10
Component: Core Version: latest
Keywords: Cc:

Description

The issue

At JOSM/geojson issue #24 (comment) the method DownloadOsmTask.AbstractInternalTask.createNewLayer(String) is overridden and returns a subclass of OsmDataLayer. To do this, the whole logic had to be duplicated from the "super" method.

What the patch changes

My patch creates a new method (createNewLayer(DataSet, String)), which only instantiates the OsmDataLayer object. This can be easily overridden by plugins to return instances of any subclass.

I also added //TODO comments to make createNewLayer() and createNewLayer(String) final (currently these are overridden by plugins cadastre-fr, geojson, opendata). Since these are only convenience methods that eventually call createNewLayer(DataSet, String), they should not be overridable. When plugins no longer override the methods, they should be made final to enforce this.

The new method getNonBlankLayerName(String) is created so subclasses can change how the layer name is determined. It takes a nullable string and returns a non-blank (not null, length >= 1 and at least one non-whitespace char) layer name.

Attachments (2)

DownloadOsmTask.patch (3.9 KB) - added by floscher 7 months ago.
Another update
DownloadOsmTask.2.patch (4.7 KB) - added by floscher 7 months ago.
Instead of TODO comments, deprecate old API and already add new API

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 months ago by michael2402

getNonBlankLayerName has a very bad logic.

I would prefer to remove the parameter and then use the following logic:

protected OsmDataLayer createNewLayer(final DataSet dataset, final Optional<String> layerName) {
     Objects.requireNonNull(dataset, "dataset");
     Objects.requireNonNull(layerName, "layerName");
     return new OsmDataLayer(dataset, layerName.orElseGet(this::getNonBlankLayerName), null);
}

... createNewLayer(String layerName) {
    var optionalLayerName = Optional.ofNullable(layerName).filter(name -> !name.isEmpty())
    ...
}

comment:2 Changed 7 months ago by Don-vip

Milestone: 18.10

comment:3 Changed 7 months ago by floscher

@michael2402: Ah yes, thanks. I'll have another look how I can improve the patch.

comment:4 Changed 7 months ago by floscher

I now modified the patch to work with Optionals in getNonBlankLayerName(), also I remembered now that .trim().isEmpty() is inefficient. The parameter of createNewLayer() is still a nullable String, there it shouldn't matter much if an Optional is passed through the convenience methods or a nullable string.

By the way: I have patches for the plugins, which I'll propose after this is merged.

Last edited 7 months ago by floscher (previous) (diff)

comment:5 Changed 7 months ago by michael2402

@floscher

The thing we should change in my opinion is the signature of the method createNewLayer. I know that the old code passes a string that can be null. We use nullable/empty Strings a lot in JOSM since there was no other way to mark optional parameters in pre-Java8 code.

In newer code, we should try to move away from the implicit "if null or empty do..." semantics and explicitly ask the users of our methods to specify wether they want to use their own value or they want us to get a default one.

I personally would write:

/**
 * @param dataset The dataset for the new layer
 * @param layerName The name of the new layer. If empty, a default name will be generated
 */
protected final OsmDataLayer createNewLayer(final DataSet dataset, final Optional<String> layerName) {
// if layerName is null: Throw NPE
// if layerName is present and empty: Throw IllegalArgumentException
//...

   return createNewLayer(dataset, computedLayerName);
}

/**
 * Create a new layer
 * @param dataset The dataset for the new layer
 * @param layerName The name of the new layer, never null
 */
protected OsmDataLayer createNewLayer(final DataSet dataset, final String layerName) {
   // ...
}

Using those semantics, it is clear that getNonBlankLayerName should not be called if the layerName was explicitly set and that if a layerName was explicitly set, we can assume that the new layer has that name.

getNonBlankLayerName can then be renamed to "generateLayerName()"

comment:6 Changed 7 months ago by floscher

Ah, now I think I understand. And thanks for the name suggestion, I was searching for a better name for that method but couldn't think of one.

But I'd change the parameter type of createNewLayer(String) to createNewLayer(Optional<String>) and remove createNewLayer() later when making that method also final. That way the plugins using them don't break.

Changed 7 months ago by floscher

Attachment: DownloadOsmTask.patch added

Another update

Changed 7 months ago by floscher

Attachment: DownloadOsmTask.2.patch added

Instead of TODO comments, deprecate old API and already add new API

comment:7 Changed 7 months ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 14347/josm:

fix #16850 - Make overriding DownloadOsmTask.AbstractInternalTask.createNewLayer() methods easier (patch by floscher, modified)

comment:8 Changed 7 months ago by Don-vip

In 14350/josm:

see #16850 - fix error_prone warning

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
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.