Correctly update location attributes in Target Editor

[tivervac]

Fixes #1255

[tivervac]

@HannesWell Might interest you as it is the indirect result of one of your comments

[tivervac]

Sadly, this patch internally uses parentElement.replaceChild() in TargetDefinitionDocumentTools.updateElements. This function simply inserts the new element in front of the old one and removes the old one, not taking into account the indentation that was present. I had a not-so-brief look at what was going on here, but the serialization code is very manual. Since the utility function replaceChild was already present, I categorize fixing it as is out of scope for this ticket

As an example, disabling includeSource through the UI for the example below

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<?pde version="3.8"?>
<target name="Test">
	<locations>
		<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="true" type="InstallableUnit">
			<repository location="https://download.eclipse.org/justj/jres/17/updates/release/"/>
			<unit id="org.eclipse.justj.openjdk.hotspot.jre.base.feature.group" version="17.0.10.v20240120-1143"/>
		</location>
	</locations>
</target>

Ends up as

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<?pde version="3.8"?>
<target name="Test">
	<locations>
		<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="false" type="InstallableUnit"><repository location="https://download.eclipse.org/justj/jres/17/updates/release/"/><unit id="org.eclipse.justj.openjdk.hotspot.jre.base.feature.group" version="17.0.10.v20240120-1143"/></location>
	</locations>
</target>

[laeubi]

It looks fine for me and a changed URL should of course be respected.

[github-actions]

Test Results

   291 files  +    6     291 suites  +6   1h 0m 14s ⏱️ + 10m 26s
 3 527 tests ±    0   3 467 ✅  -     1   58 💤 ± 0  0 ❌  - 1  2 🔥 +2 
10 878 runs  +2 726  10 699 ✅ +2 701  177 💤 +24  0 ❌  - 1  2 🔥 +2 

For more details on these errors, see this check.

Results for commit afdb8e5. ± Comparison against base commit 00c9caf.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.

AllPDETests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

AllPDETests org.eclipse.pde.ui.tests.target.AllTargetTests org.eclipse.pde.ui.tests.target.IUBundleContainerTests ‑ testSerializationOnlyLocationAttributeChanged

♻️ This comment has been updated with latest results.

[HannesWell]

I'll have a look at this tomorrow.

[HannesWell]

I have tried out your change and it updates the location's attribute. But at the same time it write the entire location element and all its nested children in one line.
I have not found out why this happens, I guess that modifying the XML element does not preserve existing white-space, but that has to be fixed before this can be submitted.

[tivervac]

... it write the entire location element and all its nested children in one line. I have not found out why this happens, I guess that modifying the XML element does not preserve existing white-space, but that has to be fixed before this can be submitted.

@HannesWell I did notice this when creating this patch, see my comment above for more details. As mentioned, if fixing this is required then sadly this ticket will not land at all as I do not plan to fix the existing bug.

[HannesWell]

... it write the entire location element and all its nested children in one line. I have not found out why this happens, I guess that modifying the XML element does not preserve existing white-space, but that has to be fixed before this can be submitted.

@HannesWell I did notice this when creating this patch, see my comment above for more details. As mentioned, if fixing this is required then sadly this ticket will not land at all as I do not plan to fix the existing bug.

Sorry, it looks like it was already a bit late yesterday.
But this is an unfortunate situation. Trading one flaw for the other is not really a satisfying solution.
Some would probably be happy that the UI now works and don't care about the formatting, but at least for me with my usage schema (having a target-file in a SCM, the new situation would be a bit worse: Now I would have to restore the old formatting after I have changed the location through the UI, which would probably take me much time than just editing the attributes in the Source-tab (both would have to be done manually). And I guess there are others that would be even less happy with the new situation.
Therefore I'm not inclined towards the new state. Sorry for that.

Ideally both would be fixed and I would be fine to keep this open until you or somebody else finds the time/motivation to fix the other issues.