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.

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

Support names for Updates repositories in product definition #824

Merged
merged 1 commit into from
Oct 28, 2023

Conversation

jcompagner
Copy link
Contributor

@jcompagner jcompagner commented Oct 26, 2023

2 editors are changed: product file editor and then the repository section but also the category editor which uses the same stuff (code is very simular)

UI part for eclipse-equinox/p2#345

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.

Thanks @jcompagner for this PR.
I had only time for a quick look yet, but it looks good so far.

But please revert all the changes in the launch config. Those are not related.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Test Results

     270 files  +       6       270 suites  +6   1h 13m 56s ⏱️ + 23m 17s
  3 327 tests  -        1    3 297 ✔️ ±       0  30 💤 ±  0  0  - 1 
10 278 runs  +2 721  10 188 ✔️ +2 698  90 💤 +24  0  - 1 

Results for commit 373e5aa. ± Comparison against base commit b3d1df7.

This pull request removes 1 test.
AllPDETests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

♻️ This comment has been updated with latest results.

@jcompagner jcompagner force-pushed the repo_names_in_editors branch 2 times, most recently from 1abed0e to 629cbdc Compare October 27, 2023 09:35
@HannesWell HannesWell force-pushed the repo_names_in_editors branch 3 times, most recently from 3d6dc69 to 6af0930 Compare October 27, 2023 21:26
@HannesWell HannesWell changed the title Support names for Updates repositories in product definition #345 Support names for Updates repositories in product definition Oct 27, 2023
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.

Thanks @jcompagner for this PR.

I did a few cosmetic changes, unified the order of some elements, moved the URL and name trimming to the canonical constructor of the RepositoryResult record and adjusted the reference to the P2-issue (since it is in another repo, the full URL is required).

Furthermore I improved the handling of empty names so that in case one leaves the name blank the name element is not written. This is especially important since no existing product has the name element and will therefore first have a null name read when being loaded the next time. Without consideration the name null would be set and we would suddenly have many repos named null. Long story short, if no name element exists none will be added automatically but one actively has to add a name. In order to remove an existing name, one can just clear the name field.

With that this is ready.

product file editor and then the repository section but also the category editor which uses the same stuff (code is very simular)

Indeed I'm tempted to unify this code as far as possible but this is something for follow up PRs.
In general I would also like to make the dialog to enter the name and the URL obsolete and let users just enter the name and URL in the table directly.

@HannesWell HannesWell merged commit 86e7922 into eclipse-pde:master Oct 28, 2023
16 checks passed
@HannesWell
Copy link
Member

I also checked the processing of category.xml files in p2 and it looks like that is already handled, so nothing needs to be done for that part:

https://github.com/eclipse-equinox/p2/blob/e55ba98eadc7e7d9d1e8630ad48187a62546b230/bundles/org.eclipse.equinox.p2.updatesite/src/org/eclipse/equinox/internal/p2/updatesite/CategoryParser.java#L695-L703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants