Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Correctly update location attributes in Target Editor #1256

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Correctly update location attributes in Target Editor #1256

wants to merge 4 commits into from

Conversation

tivervac
Copy link
Contributor

Fixes #1255

@tivervac
Copy link
Contributor Author

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

@tivervac tivervac force-pushed the 1255_serialize_location_changes branch from 4801ce1 to 3980d24 Compare April 26, 2024 13:25
@tivervac
Copy link
Contributor Author

tivervac commented Apr 26, 2024 •
edited

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>

@tivervac
Correctly update location attributes in Target Editor
650d7ea 
These were completely ignored before

iar

add test
@tivervac
Small cleanup
cb8f641 
Using a local variable and putIfAbsent makes the code
more compact and legible
@tivervac
Pass expected result as expected, not actual
78407cd
@tivervac tivervac force-pushed the 1255_serialize_location_changes branch from 3980d24 to 78407cd Compare April 26, 2024 14:42
@laeubi laeubi requested a review from HannesWell April 29, 2024 07:19
laeubi
laeubi approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 29, 2024 •
edited

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
Copy link
Member

I'll have a look at this tomorrow.

HannesWell
HannesWell requested changes Apr 30, 2024
View reviewed changes
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...ension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java Outdated Show resolved Hide resolved
...ension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java Outdated Show resolved Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java Outdated Show resolved Hide resolved
@tivervac @HannesWell
Apply suggestions from code review
afdb8e5 
Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
@tivervac
Copy link
Contributor Author

tivervac commented May 2, 2024

... 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
Copy link
Member

... 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HannesWell HannesWell HannesWell requested changes

@laeubi laeubi laeubi approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modifying the software site "Included Software" settings in the Target Editor doesn't do anything
3 participants
@tivervac @HannesWell @laeubi