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

Synchronize values changed in the Overview with the instructions file #560

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Apr 4, 2023

Currently one can edit the instructions file and after safe the manifest is updated (and thus also the editor), but the other way does not work, if one for example edit the name in the overview page the changes are just overwritten on the next compile.

This installs listeners and implements a very basic model for the instructions so changes are mirrored to the instructions file as it already happens for the MANIFEST.MF source tab.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Test Results

     240 files  ±0       240 suites  ±0   1h 8m 7s ⏱️ + 11m 53s
  3 278 tests ±0    3 252 ✔️ +4  24 💤 ±0  0  - 4  2 🔥 ±0 
10 131 runs  ±0  10 057 ✔️ +4  72 💤 ±0  0  - 4  2 🔥 ±0 

For more details on these errors, see this check.

Results for commit ff03acb. ± Comparison against base commit 71e89bb.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 4, 2023

@vik-chand @HannesWell it seems I missed the freeze yesterday, nerveless I wanted this to be merged as soon as possible, would you as PL approve this?

Even though it might bee seen as a new feature, it also fixes some issues one currently see with using that feature as currently sometimes the manifest editor disappears and the manifest is not regenerated when things run "too fast" after each other and users might be confused when there changed values are not taken into account.

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 only checked the existing code that is not only related to the new BND processing to ensure existing functionality is not broken and that looks good.
Since you just added the BND processing recently there is nothing to break that used to work in a previous release/milestone.
Therefore and since you said it contains fixes I'm fine with merging this today.

Only two remarks below. They are not critical but should be addressed eventually.

Currently one can edit the instructions file and after safe the manifest
is updated (and thus also the editor), but the other way does not work,
if one for example edit the name in the overview page the changes are
just overwritten on the next compile.

This installs listeners and implements a very basic model for the
instructions so changes are mirrored to the instructions file as it
already happens for the MANIFEST.MF source tab.
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.

Looks good. Thank you. 👍🏽

@laeubi laeubi merged commit 9cddd29 into eclipse-pde:master Apr 4, 2023
4 of 7 checks passed
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.

None yet

2 participants