[Patch] Improve marker handling in sessions

[Bjoeni]

Followup of #19219 and #20913

When opening a *.gpx file containing both GPX tracks and markers two layers are opened. When saving that to a session the markers will essentially be saved twice: Once in the original GPX file (either included in the *.joz or seperately) and once in the marker file included in the *.joz.

That is redundant and can lead to complications since the two layers are no longer associated to each other after being loaded from a session file.

(e.g. when changing the settings of the markers, that would not be reflected when exporting the original GPX file again since it's no longer associated)

I'm planning on not exporting a marker layer to a session file at all when it's already included the GPX file. Markers should only be included when the corresponding GPX track does not exist anymore (i.e. is not loaded as a layer). That also allows saving as *.jos in these cases (see #20913).

This also requires some changes when loading the session since currently all waypoints in GPX files from sessions are simply ignored.

[Bjoeni]

• markers that are linked to a GPX layer will be saved as a "sub layer" of the GPX layer (and only saved once)

          <layer index="1" name="GPX layer name" type="tracks" version="0.1" visible="true">
              <file>layers/01/data.gpx</file>
              <markerLayer index="2" name="Marker layer name" opacity="0.5" visible="true"/>
          </layer>
  

[Bjoeni]

Note: The patches #19219, #21813, #21922, #21923 all affect session handling. Whilst I don't think that they conflict, I only tested them all together.

It might make sense to only apply the all.patch that I attached on #21923

[stoecker]

Milestone renamed

[taylor.smock]

I got a crash while testing this patch:

2022-05-18 13:03:40.963 java[12872:11492749] 2022-05-18 13:03:40.960 SEVERE: Handled by bug report queue: java.lang.NullPointerException
java.lang.NullPointerException
	at java.desktop/java.awt.Container.addImpl(Container.java:1117)
	at java.desktop/java.awt.Container.add(Container.java:997)
	at org.openstreetmap.josm.actions.SessionSaveAsAction$SessionSaveAsDialog.build(SessionSaveAsAction.java:299)
	at org.openstreetmap.josm.actions.SessionSaveAsAction$SessionSaveAsDialog.<init>(SessionSaveAsAction.java:235)
	at org.openstreetmap.josm.actions.SessionSaveAsAction.saveSession(SessionSaveAsAction.java:113)
	at org.openstreetmap.josm.actions.SessionSaveAsAction.actionPerformed(SessionSaveAsAction.java:91)
	at java.desktop/javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1967)
	at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2308)
	at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:405)
	at java.desktop/javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:262)
	at java.desktop/javax.swing.AbstractButton.doClick(AbstractButton.java:369)
	at java.desktop/com.apple.laf.ScreenMenuItem.actionPerformed(ScreenMenuItem.java:129)
	at java.desktop/java.awt.MenuItem.processActionEvent(MenuItem.java:690)
	at java.desktop/java.awt.MenuItem.processEvent(MenuItem.java:649)
	at java.desktop/java.awt.MenuComponent.dispatchEventImpl(MenuComponent.java:375)
	at java.desktop/java.awt.MenuComponent.dispatchEvent(MenuComponent.java:363)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:775)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

Steps to reproduce:

1. Load the test/data/sessions/markers.gpx file into JOSM (easy way: java -jar dist/josm-custom.jar test/data/sessions/markers.gpx)
2. Attempt to save the session

[Bjoeni]

Didn't happen with r18392 with all the above patches applied, so something conflicts once again.

I'll provide a new (combined) patch for this one and #21923. Probably would've been easier to apply all patches at once though...

[taylor.smock]

The reason why I merged #19219 and #21922 separately was because they were small and fairly easy for me to test. Larger patches typically require more testing, more analysis, etc. And I'm also less likely to merge them near a potential release.

[Bjoeni]

Replying to taylor.smock:

The reason why I merged #19219 and #21922 separately was because they were small and fairly easy for me to test. Larger patches typically require more testing, more analysis, etc. And I'm also less likely to merge them near a potential release.

Sure, I didn't mean to criticize that. That was merely a note to myself, I'll probably just not provide potentially conflicting patches at the same time in the future. Either an all in one patch or I'll just wait until the previous one has been committed.

Either way, here's a combined and updated patch for this ticket and #21923, now it should work:

[taylor.smock]

Thank you for the updated patch. I'll take a look at it next week. If I haven't gotten back to you by next Tuesday, ping me.

A few quick comments (from a read through):

• In SessionSaveAction#getTooltip, why didn't you use getInstance().getToolTipText()?
• The LayerManager diff seems a bit large. This is probably due to line endings. It is an unrelated change, but something that is overall "good".
• SessionWriterTest.java L167 (/**R -> /**\n * R)
• I'd like to have @since xxx for the newly public methods in the test classes, but we don't version the josm-unittest.jar file, so not currently useful (this can make tests for plugins fail when JOSM changes its signing key).

[Bjoeni]

Replying to taylor.smock:

Thank you for the updated patch. I'll take a look at it next week. If I haven't gotten back to you by next Tuesday, ping me.

A few quick comments (from a read through):

• In SessionSaveAction#getTooltip, why didn't you use getInstance().getToolTipText()?

Because the class inherits JosmAction which doesn't have that method and doesn't keep the tooltip that was set. It only keeps the dynamically generated tooltip with the shortcut (including HTML formatting).

• The LayerManager diff seems a bit large. This is probably due to line endings.

Yeah don't know what happened there. Changed.

It is an unrelated change, but something that is overall "good".

Do you mean the line endings or the change to isEmpty()? If the latter then no, it's not unrelated because I rely on that event in SessionSaveAction#layerRemoving(). And it simply didn't work before because it checked if there was one layer left after removing the layer. It just wasn't used anywhere in the project and therefore probably never noticed.

• SessionWriterTest.java L167 (/**R -> /**\n * R)

changed.

• I'd like to have @since xxx for the newly public methods in the test classes, but we don't version the josm-unittest.jar file, so not currently useful (this can make tests for plugins fail when JOSM changes its signing key).

Added. Thanks for the feedback.

I updated the patch. Also the LayerChangeListener will now be properly removed.

[taylor.smock]

Replying to Bjoeni:

Replying to taylor.smock:

It is an unrelated change, but something that is overall "good".

Do you mean the line endings or the change to isEmpty()? If the latter then no, it's not unrelated because I rely on that event in SessionSaveAction#layerRemoving(). And it simply didn't work before because it checked if there was one layer left after removing the layer. It just wasn't used anywhere in the project and therefore probably never noticed.

That makes me wonder why it was size() == 1. (When I was skimming the patch, I read it as size() == 0).
So I looked at history: size() == 1 was added in r10432. It looks like r10273 had the removal occur after the event fire, and r10998 changed that (removal occurred before event fire), and due to a lack of tests, it wasn't noticed that the behavior was different for lastLayer.

[...] Thanks for the feedback.

You are welcome.

[taylor.smock]

In 18466/josm:

Fix #21813: Improve marker handling in sessions and #21923: Improve session workflow/Add "save session" (patch by Bjoeni)

• Allow saving a previously saved session
• Add "File" -> "Save Session"
• Add shortcuts for saving sessions
• Add warning if a layer in a session is being removed when saving over the session
• Improve GPX marker handling