Osm license #52

brunesto · 2020-10-21T08:00:45Z

Hello this is to resolve #51 - the osm tile usage policy . it does not remove the app, just adds copyright label and changes the way TileRetriever is instanciated

Adding the copyright label is just a couple of lines in MapView

For beeing able to easily use own tile provider (e.g. in the MobileSample class) it required more changes: Basically TileProviderRetriever is gone, instead the TileRetriever has to passed as a constructor arg to MapView and BaseMap. Hence CachedOsmTileRetriever does not have static fields methods anymore.

I wanted to keep the changes small, so 2 further changes are probably needed:

Instead of moving CachedOsmTileRetriever around I just modified module-info.java
It breaks compatibility, so an empty MapView constructor would be needed

samples/mobile/src/main/java/com/gluonhq/maps/samples/mobile/MobileSample.java

maps/src/main/java/com/gluonhq/maps/MapView.java

abhinayagarwal · 2020-10-21T14:34:27Z

maps/src/main/java/com/gluonhq/impl/maps/tile/osm/CachedOsmTileRetriever.java


-    static {
+     {


Not a huge fan of double brace initialization. May be add a constructor instead?

I am not sure if the logic was appropriate before you made the change., but now we have different instances of CacheThread writing to the same cache directory.

You mean there were 2 instances of CacheThread because the sample created 2 CachedOsmTileRetriever, right?
If this was the issue, I just added a String parameter to the CachedOsmTileRetriever constructor and it is used as suffix to build the cache root path cacheRoot.

I am not a big fan of the implementation in CachedOsmTileRetriever. If you check CacheThread, you will find that it has an indefinite while loop and it keep on running. The deactivate() which is supposed to break the while loop is never called. The whole class needs to be re-written IMO.

Can you please remove the change where you added a string name as suffix for cache directory? It is better to keep the class as it is. I will log an issue with the shortcomings on the class.

I have reverted the commit that added suffix to cache

samples/mobile/src/main/java/com/gluonhq/maps/samples/mobile/MobileSample.java

abhinayagarwal · 2020-10-21T14:53:39Z

maps/src/main/java/com/gluonhq/maps/tile/TileRetriever.java

@@ -41,4 +41,5 @@
     * @return an image representing the tile
     */
    Image loadTile(int zoom, long i, long j);
+    String copyright();


This method makes more sense in OsmTileRetriever.

Are you sure? In this case MapView would have to accept a OsmTileRetriever instead of TileRetriever as its constructor argument.

Apologies for the delay. I somehow missed the notifications.

I am not sure. @jperedadnr do you have a preference here? Will all types of TileRetriever need a copyright license and it make sense to add "copyright" to TileRetriever interface?

I've done a full review of the PR, looks good to me, with some comments that should be addressed.

About the copyright, yes, it might not be mandatory, but I'd say it can stay with TileRetriever.

In my comments, I suggest to use CSS for the label, in which case you can set its visibility to false. Even setting the copyright text to null won't show it. (We don't really have a way to enforce the developer to use/show the license).

abhinayagarwal · 2020-10-21T15:58:58Z

Hi @brunesto ,

Thank you for the PR. Please sign the Gluon CLA as per the Gluon contribution requirement.

jperedadnr · 2020-10-28T20:16:50Z

@brunesto We have just merged a pending PR, can you fix the conflicts?

brunesto · 2020-10-29T16:00:01Z

@jperedadnr I think it's done

This reverts commit f6c7127.

maps/src/main/java/com/gluonhq/impl/maps/BaseMap.java

samples/mobile/src/main/java/com/gluonhq/maps/samples/mobile/MobileSample.java

maps/src/main/java/com/gluonhq/maps/MapView.java

jperedadnr · 2020-12-02T13:36:24Z

maps/src/main/java/module-info.java

@@ -37,4 +37,5 @@

    exports com.gluonhq.maps;
    exports com.gluonhq.maps.tile;
+    exports com.gluonhq.impl.maps.tile.osm;


if we are going to export this package, it shouldn't be under impl, so osm should be under com.gluonhq.maps.tile ?

Please see the original comment: "...Instead of moving CachedOsmTileRetriever around I just modified module-info.java ..."

I had a look at it but I don't see a simple way to split the CachedOsmTileRetriever between impl and exported packaged, so I prefer not to make any changes for that in the PR.

jperedadnr · 2020-12-02T15:42:02Z

samples/mobile/src/main/java/com/gluonhq/maps/samples/mobile/MobileSample.java

@@ -68,7 +70,28 @@

    @Override
    public void start(Stage stage) {
-        MapView view = new MapView();
+        TileRetriever osm=new CachedOsmTileRetriever() {


If this for demo only, why don't we already provide this implementation as part of com.gluonhq.maps.tile.osm, exactly as is? Or with a name like DefaultCachedOsmTileRetriever ?

Well I think it is better if tile provider is explicit, i.e. the developer should know where the tiles are coming from. Also like this it is easy to modify the url to switch to a different provider.

samples/mobile/src/main/java/com/gluonhq/maps/samples/mobile/MobileSample.java

jperedadnr · 2020-12-02T15:45:34Z

maps/src/main/java/com/gluonhq/maps/MapView.java

    /**
     * Create a MapView component.
     */
-    public MapView() {
-        baseMap = new BaseMap();
+    public MapView(TileRetriever tileRetriever) {


For the sake of compatibility, we could keep the no-args constructor, and default to a possible implementation of CachedOsmTileRetriever (see comment below)?

Thinking about it... I would prefer to not have a no-args constructor actually, for the reasons in comment above.

brunesto added 2 commits October 21, 2020 09:39

added copyright label + sample mapbox provider

0899898

rm

a411ebd

abhinayagarwal reviewed Oct 21, 2020

View reviewed changes

brunesto added 2 commits October 21, 2020 22:50

format code + L&F

a05df7d

CachedOsmTileRetriever uses name as suffix for cache directory

f6c7127

brunesto added 3 commits October 29, 2020 16:53

merged

7b25538

merged

b66d923

Merge branch 'gluonhq-master' into osm-license

ee68d3a

jperedadnr added the CLA signed Author has signed the Gluon CLA label Nov 3, 2020

abhinayagarwal mentioned this pull request Nov 30, 2020

Rewrite CachedOsmTileRetriever #53

Open

Revert "CachedOsmTileRetriever uses name as suffix for cache directory"

347599a

This reverts commit f6c7127.

jperedadnr reviewed Dec 2, 2020

View reviewed changes

brunesto added 5 commits December 4, 2020 19:50

spacing + javadoc

ee59b35

map label uses css

417e9c9

comments + added my own token

5a01dc3

spacing

17398a9

javadoc

9d1c40d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Osm license #52

Osm license #52

brunesto commented Oct 21, 2020 •

edited

Loading

abhinayagarwal Oct 21, 2020

abhinayagarwal Oct 21, 2020

brunesto Oct 21, 2020 •

edited

Loading

abhinayagarwal Nov 30, 2020

abhinayagarwal Nov 30, 2020

brunesto Nov 30, 2020

abhinayagarwal Oct 21, 2020

brunesto Oct 21, 2020 •

edited

Loading

abhinayagarwal Nov 30, 2020

jperedadnr Dec 2, 2020

abhinayagarwal commented Oct 21, 2020

jperedadnr commented Oct 28, 2020

brunesto commented Oct 29, 2020 •

edited

Loading

jperedadnr Dec 2, 2020

brunesto Dec 5, 2020

brunesto Dec 5, 2020

jperedadnr Dec 2, 2020

brunesto Dec 5, 2020

jperedadnr Dec 2, 2020

brunesto Dec 5, 2020

Osm license #52

Are you sure you want to change the base?

Osm license #52

Conversation

brunesto commented Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunesto Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunesto Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinayagarwal commented Oct 21, 2020

jperedadnr commented Oct 28, 2020

brunesto commented Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunesto commented Oct 21, 2020 •

edited

Loading

brunesto Oct 21, 2020 •

edited

Loading

brunesto Oct 21, 2020 •

edited

Loading

brunesto commented Oct 29, 2020 •

edited

Loading