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

Modify the 'Close Editor' handler and enabled when evaluation to supp… #2315

Merged

Conversation

feilimb
Copy link
Contributor

@feilimb feilimb commented Sep 23, 2024

Modify the 'Close Editor' handler and 'enabledWhen' evaluation to add support for both compatibility-layer editor parts, and new Parts contributions which represent an Editor and are contributed via eg. a PartDescriptors in a Model Fragment addition.

Associated with Issue#2176 (#2176).

@feilimb
Copy link
Contributor Author

feilimb commented Oct 1, 2024

Could anyone pick this up? @jukzi ? @iloveeclipse ?

IWorkbenchWindow window = HandlerUtil.getActiveWorkbenchWindowChecked(event);
IEditorPart part = HandlerUtil.getActiveEditorChecked(event);
window.getActivePage().closeEditor(part, true);
public Object execute(ExecutionEvent event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should always try to look in the event to ensure we get the right editor. Other strategies can have flaws and return another editor than the one that triggered the event.
Would it be possible to get the e4 element (MPart) from the event context instead?

If not, I think it would be worth keeping usage of HandlerUtil.getActiveEditorChecked(event) if it's not null, and if null, then failback to an alternative strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback - yes that makes sense, I have updated the PR now such that the event context is used to retrieve the active part. If the active part is an IEditorPart I perform the previous logic, else if it is an E4Wrapper such as when we contribute a PartDescriptor via a model fragment it is dealt with via the EPartService.

Copy link
Contributor

github-actions bot commented Oct 1, 2024

Test Results

 1 815 files  ±0   1 815 suites  ±0   1h 38m 29s ⏱️ + 7m 52s
 7 702 tests ±0   7 474 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 267 runs  ±0  23 520 ✅ +1  747 💤 ±0  0 ❌  - 1 

Results for commit 86bd7be. ± Comparison against base commit 419bb61.

♻️ This comment has been updated with latest results.

…ort for Compatibility parts and new Parts which represent an Editor and are contributed via eg. PartDescriptors in a Model Fragment. Associated with Issue#2176.
@mickaelistria
Copy link
Contributor

Thanks for the change, this looks much better to me.
Let's try to merge it as soon as CI completes (particularly Jenkins build). I have rebased the commit to increase the chances of success.

@feilimb
Copy link
Contributor Author

feilimb commented Oct 1, 2024

Thanks for the change, this looks much better to me. Let's try to merge it as soon as CI completes (particularly Jenkins build). I have rebased the commit to increase the chances of success.

Many thanks Mickael, lets see how the CI jobs go this time.

@feilimb
Copy link
Contributor Author

feilimb commented Oct 1, 2024

Also, just a heads up : I may look into a similar approach in a separate PR for handling the 'Close Active Editors' (plural), if I ever get that far I might tag you in that PR if that's OK.

@mickaelistria mickaelistria merged commit 04dc50b into eclipse-platform:master Oct 1, 2024
17 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