[patch] Not possible to save a layer after marked as "Discourage upload"

[gaben]

What steps will reproduce the problem?

1. Have a new layer with some data
2. Save it
3. Mark it as "Discourage upload"
4. Try to save it again

What is the expected result?

It's possible to save the layer OR the upload attribute automatically propagated to the file without user intervention.

What happens instead?

The save button greyed out. To save the upload=false attribute to the file, I need to make a change to the dataset, then the save button will be available and the attribute will be saved to the file.

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

It was modified during #23506, #23408 and #22817.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-04-27 10:24:46 +0200 (Sat, 27 Apr 2024)
Revision:19064
Build-Date:2024-04-28 01:31:14
URL:https://josm.openstreetmap.de/svn/trunk

[GerdP]

Hit the same problem in the last days.

OR the upload attribute automatically propagated to the file without user intervention.

I wouldn't want that.

[GerdP]

BTW: the save action is still available in the context menu of the layer side window.
Proposed solution:

src/org/openstreetmap/josm/gui/layer/OsmDataLayer.java

@@ -1195 +1195 @@
         if (data.getUploadPolicy() != UploadPolicy.BLOCKED &&
                 (uploadDiscouraged ^ isUploadDiscouraged())) {
             data.setUploadPolicy(uploadDiscouraged ? UploadPolicy.DISCOURAGED : UploadPolicy.NORMAL);
+            setRequiresSaveToFile(true);
             for (LayerStateChangeListener l : layerStateChangeListeners) {
                 l.uploadDiscouragedChanged(this, uploadDiscouraged);
             }

[gaben]

Didn't test the patch yet, just putting the existence in the title.

[GerdP]

@gaben: Do you think there is a better solution?

[gaben]

I don't know, probably yes, but I have to check and don't have much time for investigation, unfortunately :(

Setting it to true all the time looks suspicious to me. Before the preconditions cleanup, it was working fine, and this part wasn't touched. There is an isSavable() method for layers, which should say yes, and the handling code should respect it. If I have to guess, it goes wrong somewhere here.

[GerdP]

Question is why Save is disabled at all. In my eyes it should always be possible to save the layer to file again, even if that would just overwrite the existing file with the same data.
Example case: I save to work.osm and use a command line or file manager to move that file somewhere else to experiment with the data.
JOSM.
File->Save is disabled, the right click on the layer work.osm shows an enabled Save function.

Seems this behaviour was introduced for #12669

[GerdP]

The patch is still incomplete. I think there's one more issue: The ToggleUploadDiscouragedLayerAction really toggles the uploadDiscouraged flag, but the action text says tr("Discourage upload") and the icon is always the same.
I think the text should be changed to tr("Encourage/Discourage upload") and a notification should be shown when the state is toggled.

[GerdP]

The patch introduces these changes:

• enable Edit->Save if layer is saveable (remove complex logic)
• show notification when uploadDiscouraged is toggled
• set requiresSaveToFile flag when the uploadDiscouraged is toggled
• change action text to tr("Discourage/Encourage upload")

One small problem: If you toggle the uploadDiscouraged flag twice there is not really a need to save to file, but we have the same issue when you modify data and then undo all changes.

[gaben]

One small problem: If you toggle the uploadDiscouraged flag twice there is not really a need to save to file, but we have the same issue when you modify data and then undo all changes.

I think the requiresSaveToFile property is there for this reason. So there should be a better way, which I don't know (yet).

Now, if the layer is savable (yes?) and requires saving to the file (yes?) why cannot it save?

I just noticed a weird behaviour: after I save the file in step 2, the save button is still active and I can save it again.

[GerdP]

Did you try with 23648.2.patch?

I think the requiresSaveToFile property is there for this reason. So there should be a better way, which I don't know (yet).

The property is set when either the data or the meta data like the uploadDiscouraged flag is changed. I see no need to really check if the content of an existing file would be changed or not.

I just noticed a weird behaviour: after I save the file in step 2, the save button is still active and I can save it again.

That's exactly what I want. See comment:8. The layer action Save is also always enabled.

[gaben]

Did you try with 23648.2.patch?

Yes.

The property is set when either the data or the meta data like the uploadDiscouraged flag is changed. I see no need to really check if the content of an existing file would be changed or not.

There is a * mark in the window title if there are any unsaved changes, like * Java OpenStreetMap Editor. With the patch it is becomes confusing.

I just noticed a weird behaviour: after I save the file in step 2, the save button is still active and I can save it again.

That's exactly what I want. See comment:8. The layer action Save is also always enabled.

This behaviour is without the patch, please try it out. I meant to say, it is possible to save exactly twice in this case, but only after the save. There is something wrong deeper.

[GerdP]

I fear I don't understand what problem(s) you want to solve.

[gaben]

:(

When I mark a layer as discourage upload, I'd like to save it exactly once as it was working previous releases.

(Disabling the save adds extra information that anything is open is actually saved.)

[GerdP]

So you don't agree with comment:8 ?

[GerdP]

Replying to gaben:

This behaviour is without the patch, please try it out. I meant to say, it is possible to save exactly twice in this case, but only after the save. There is something wrong deeper.

I was not able to reproduce this.

[GerdP]

With the patch it is becomes confusing.

It took a while but now I understand why you think the enabled Save could be confusing. You probably use the grayed icon in the main menu as an indicator that the file was not changed? That's in fact a good reason to keep the old (in my eyes wrong) behaviour.
I'll have a closer look at the code in SaveAction.updateEnabledState ()

[GerdP]

Replying to GerdP:

Replying to gaben:

This behaviour is without the patch, please try it out. I meant to say, it is possible to save exactly twice in this case, but only after the save. There is something wrong deeper.

I was not able to reproduce this.

Now I found out how to reproduce:
1) Have a new layer with some data
2) Save it with File->Save As

[GerdP]

23648-3.patch fixes the problem with "Save As". I've moved the complex code to SaveActionBase.updateEnabledState () as this is also called by SaveAsAction. With the unpatched code SaveAction.updateEnabledState () was not called when "Save As" was used.
I'm not sure if the test should use this instanceof SaveAction or simply check the quiet flag.
Maybe I should also add the final modifier to SaveActionBase.updateEnabledState ()?

[gaben]

Thank you for working on it, and I'm sorry if I wasn't clear.

This v3 patch is better, but I still see an issue. The asterisk remains there after the file is saved. To reproduce:

1. Have a new layer with some data
2. Save it with File → Save As
3. Check the window title and layer name.

Edit: I just checked without the patch. It's there, so I think it's a new bug, yay!

[GerdP]

I think the asterisk tells you that data can be uploaded and that the upload is not discouraged?

[gaben]

Ah, it's defined by isDirty():

    /**
     * Determines if this layer is "dirty", i.e. requires save or upload
     * @return if this layer is "dirty"
     * @since 18287
     */
    public boolean isDirty() {
        // Override if needed
        return requiresSaveToFile() || (requiresUploadToServer() && !isUploadDiscouraged());
    }

and my picture shows an upload encouraged state. Sigh. Everything is alright. Sorry, I'm tired.

The last remark: I don't like the "Discourage/Encourage upload" string change, as usually there is no sentence like

• "(x) Enable/disable feature y", just
• "(x) Enable feature y"

[GerdP]

The action toggles the state. I found no similar case in JOSM as an example. Do you have one?

[GerdP]

In 19116/josm:

fix #23648: Not possible to save a layer after marked as "Discourage upload"

• mark layer as changed and show notificaton when the toggle was triggered
• remove overwrite of SaveActionBase.updateEnabledState() for "Save"

[gaben]

The action toggles the state. I found no similar case in JOSM as an example. Do you have one?

I don't, it's unique stuff. Thanks btw!

[GerdP]

You're welcome! The toggle action confused me as well in the past, I just needed a push to look at the details ;)

[gaben]

Let me know if you have something else in your mind, maybe we can cooperate ;)