-
Notifications
You must be signed in to change notification settings - Fork 166
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
Find/Replace overlay: handle the case when moving editor between windows #1946
Find/Replace overlay: handle the case when moving editor between windows #1946
Conversation
d5d0b0a
to
ffd1d61
Compare
ffd1d61
to
e543eee
Compare
With this PR overlay never appears for me on Linux, so Ctrl+F is broken completely. |
...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java
Outdated
Show resolved
Hide resolved
...es/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceOverlay.java
Outdated
Show resolved
Hide resolved
e543eee
to
57d9c32
Compare
@iloveeclipse can you please confirm that this PR now opens on Linux? |
You mean not that PR opens but that Ctrl+F shows an overlay now? Yes, it does. |
@iloveeclipse very good! |
8fc0654
to
7ddf9d7
Compare
7ddf9d7
to
41e4727
Compare
@@ -353,6 +353,10 @@ public boolean close() { | |||
|
|||
@Override | |||
public int open() { | |||
super.open(); |
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 results in multiple calls to super.open()
, as it is already called in a subsequent if block. Why is an additional call necessary? At least, I would only expect one of the calls to be necessary.
@@ -412,7 +412,7 @@ private void showDialog() { | |||
} | |||
|
|||
private void showOverlayInEditor() { | |||
if (overlay == null) { | |||
if (overlay == null || !overlay.isOverlayOpen()) { |
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 will result in always creating a new overlay, so all the logic in the overlay used for re-opening it will be unused, won't it? Why is this necessary?
41e4727
to
2048963
Compare
@HeikoKlare so the logic here is: if the Overlay senses that it's viewPart is not the same anymore, it closes itself and disposes itself. The action doesn't know about this and should absolutely not open the overlay and instead create a new one. I am not sure what I thought when I implemented this a few weeks ago, but it seems to be far more complex than It should be. I have made the implementation more "naive" and in my tests, the implementation still works perfectly! |
2048963
to
88357bc
Compare
Sorry, I still don't get it. With the current implementation, the My expectation of flows/responsibilities is as follows:
The latter two actions are currently the responsibility of the consumer. So the consumer must be aware that there will be events that close the overlay and require a new overlay to be created (instead of reopening the existing one). |
I completely agree that we should
A few technical limitations made the solution not very straightforward
I have thus come up with this solution which clearly states as API contract that the overlay closes itself when it detects that the parent shell is changed (i.e. the targeted viewpart was moved to another window) I have not dropped the commit for the old change and I will fix the merge errors once I have received confirmation that this is the direction we should move to @HeikoKlare |
d394096
to
9117357
Compare
@@ -840,9 +855,13 @@ private void repositionTextSelection() { | |||
} | |||
|
|||
private void updatePlacementAndVisibility() { | |||
if (isInvalidTargetShell()) { | |||
getShell().getDisplay().asyncExec(onParentShellChange); |
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.
Instead of using the callback provided by the consumer, just internally updating the target seems to work for me:
getShell().getDisplay().asyncExec(() -> {
close();
setParentShell(targetPart.getSite().getShell());
open();
targetPart.setFocus();
});
Can you try this and check whether it also works?
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.
9117357
to
1f1092b
Compare
When moving the targeted editor between different windows, the overlay is closed to allow a retargetting. fixes eclipse-platform#1945
1f1092b
to
d121c1e
Compare
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.
I've tested this on all three OS, in particular with the problematic scenarios to be addressed by this change. I did not find any issues so far, so let's have this merged.
When moving the targeted editor between different windows, the overlay is closed to allow a retargetting.
fixes #1945