[WIP patch] Download Dialog incorrectly reports note area rejection

[gaben]

What steps will reproduce the problem?

1. Open download dialog
2. Play with the bounding box while only the OSM data download checked
3. Repeat step 2 with notes checked only

What is the expected result?

For note download the allowed bounding box is 100x greater than data, so the message should reflect that.

What happens instead?

The note's allowed bounding box size (25) ignored, the data restrictions (0.25) used instead.

Please provide any additional information below. Attach a screenshot if possible.

This patch fixes the issue, but the existing code not working as expected. See the comment.

src/org/openstreetmap/josm/gui/download/OSMDownloadSource.java

@@ -340 +340 @@
             }
 
             displaySizeCheckResult(DOWNLOAD_SOURCES.stream()
+                    // this line doesn't work, but should: .filter(IDownloadSourceType::isEnabled)
+                    .filter(type -> type.getCheckBox().isSelected())    //this works
                     .anyMatch(type -> type.isDownloadAreaTooLarge(bbox)));
         }
 
@@ -414 +416 @@
         @Override
         public boolean isDownloadAreaTooLarge(Bounds bound) {
             // see max_request_area in
-            // https://github.com/openstreetmap/openstreetmap-website/blob/master/config/example.application.yml
+            // https://github.com/openstreetmap/openstreetmap-website/blob/master/config/settings.yml
             return bound.getArea() > Config.getPref().getDouble("osm-server.max-request-area", 0.25);
         }
     }
@@ -492 +494 @@
         @Override
         public boolean isDownloadAreaTooLarge(Bounds bound) {
             // see max_note_request_area in
-            // https://github.com/openstreetmap/openstreetmap-website/blob/master/config/example.application.yml
+            // https://github.com/openstreetmap/openstreetmap-website/blob/master/config/settings.yml
             return bound.getArea() > Config.getPref().getDouble("osm-server.max-request-area-notes", 25);
         }
     }

[gaben]

Ping/up/whatever. I have no idea why IDownloadSourceType::isEnabled isn't working.

[taylor.smock]

It looks like it is due to the preferences not being updated when the checkbox state changes.

isEnabled() calls getBooleanProperty().get().

EDIT: And the answer is in the javadoc

Determines the last state of the download type

[gaben]

Is it desired? Because we are now dealing with two states. One for live and the other for stored checkbox (BooleanProperty) state.

For this reason, the javadoc is misleading.

[taylor.smock]

I cannot remember exactly why I did that, but most likely due to having a Cancel button (AKA don't persist changed state when user has canceled), which would kind of argue against live updates to the property.

[gaben]

Checkbox states saved on the Download window close, doesn't matter if you cancel or download. See here and here.

[taylor.smock]

In that case, we might as well add a new checkbox listener that updates the preferences. Which means we can remove the call in rememberSettings. But only if we notify plugins that use it.

[gaben]

I'm not familiar with Java GUI programming, but I see listeners for download source checkboxes e.g. here.

[taylor.smock]

Yes. That listener is defined here​.

[gaben]

What could be the solution, a new listener you mentioned in comment:8?

[gaben]

Replying to taylor.smock:

In that case, we might as well add a new checkbox listener that updates the preferences. Which means we can remove the call in rememberSettings. But only if we notify plugins that use it.

How can I check which plugins are using the rememberSettings() function?

[taylor.smock]

I'd probably start by doing a grep in a JOSM svn checkout (the one with core plugins).

Beyond that, we have a (currently broken) check-plugins ant target, which someone should probably fix.

I had a script for checking plugins using GitLab CI, but I don't know if it still works. It used ​japi.