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

Fix Bug #1074 #1319

Conversation

Wittmaxi
Copy link
Contributor

@Wittmaxi Wittmaxi commented Nov 21, 2023

Fixes Bug #1047: Multiselection: lost after restart
When saving it's state, an AbstractEditor will now consider MultiSelection.

This PR is a proposal for improvement and requires a few more iterations before release.
In particular, I would like to check:

  • Do I break backwards-compatibility with prior Mementos?
  • Are there some special AbstractTextEditor's which break because of my change?
  • Is the format I chose for saving the different regions of a MultiSelection sane?

Copy link
Contributor

github-actions bot commented Nov 21, 2023

Test Results

     900 files  ±0       900 suites  ±0   1h 7m 34s ⏱️ + 3m 17s
  7 468 tests ±0    7 315 ✔️ +19  153 💤 ±0  0  - 15 
23 553 runs  ±0  23 044 ✔️ +19  509 💤 ±0  0  - 15 

Results for commit 3c84b7b. ± Comparison against base commit 6e19205.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

This is definitely going in the right direction. I've submitted some proposals to minimize the amount of extra fields in the memento.

@Wittmaxi
Copy link
Contributor Author

@mickaelistria I have addressed all of your concerns. After a bit of manual testing, I am more confident in this PR.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

Code is good to be merged, thanks.
However, can ayou please reformat the commit message. The usual best practice is to mention what's fixed in the commit header, then an optional details block and then the reference to the fixes issue.

For example here it could be

Restore editor MultiSelection on restart

Stores the multi-selection for the text editor in the memento; and restores it on restart.

Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/1074

@mickaelistria mickaelistria added this to the 4.31 M1 milestone Nov 21, 2023
@Wittmaxi Wittmaxi force-pushed the MW_MultiselectionLostAfterRestart branch from 42939fb to 87c7088 Compare November 21, 2023 16:13
@Wittmaxi
Copy link
Contributor Author

@mickaelistria I changed the message of the commit!

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

Thank you. We'll merge it as soon as the 4.31 stream is open for development (in a couple of weeks IIRC)

@mickaelistria mickaelistria force-pushed the MW_MultiselectionLostAfterRestart branch from 87c7088 to b66c61a Compare December 4, 2023 12:39
Stores the multi-selection for the text editor in the memento; and restores it on restart.

Fixes eclipse-platform#1074
@HeikoKlare HeikoKlare force-pushed the MW_MultiselectionLostAfterRestart branch from b66c61a to 3c84b7b Compare December 5, 2023 14:13
@mickaelistria mickaelistria merged commit 0bf3af9 into eclipse-platform:master Dec 6, 2023
16 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.

2 participants