-
Notifications
You must be signed in to change notification settings - Fork 193
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
FindReplaceLogic.updateTarget should call endSession on current target #1846
FindReplaceLogic.updateTarget should call endSession on current target #1846
Conversation
@@ -588,12 +588,11 @@ public void updateTarget(IFindReplaceTarget newTarget, boolean canEditTarget) { | |||
this.isTargetEditable = canEditTarget; | |||
|
|||
if (this.target != newTarget) { | |||
if (newTarget != null && newTarget instanceof IFindReplaceTargetExtension) | |||
((IFindReplaceTargetExtension) newTarget).endSession(); | |||
if (this.target instanceof IFindReplaceTargetExtension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should end the session for the current target not for the new target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
this.target = newTarget; | ||
if (newTarget != null) | ||
isTargetSupportingRegEx = newTarget instanceof IFindReplaceTargetExtension3; | ||
isTargetSupportingRegEx = newTarget instanceof IFindReplaceTargetExtension3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null guard seems pointless.
FYI, I noticed that find in Oomph's setup editors was broken, often requiring me to close the editor to get it working again. I tested against EMF's editors (e.g., the Ecore editor) and it had the same problem. I finally tracked it down to this lifecycle problem. EMF add this control: And when the editor no longer has focus, the session should end and the control should be removed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix! It properly corrects a recently introduced regression. Thus, I propose to merge this once CI has finished. Then it should also make its way into the M2 build.
Test Results 920 files 920 suites 38m 39s ⏱️ For more details on these failures, see this check. Results for commit 15169b5. |
Windows/Mac failures unrelated. |
No description provided.