-
Notifications
You must be signed in to change notification settings - Fork 2
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 multi-window chat bug #301
Conversation
25ce53e
to
77b3212
Compare
This seems better than having another window with a broken experience. But do we have an idea of what it would take to actually have functionality across multiple windows? Doesn't seem like a super common use case but it would be interesting to see how involved it would be. |
Oh makes sense. I'll take a look. |
@Override | ||
public final Object execute(final ExecutionEvent event) { | ||
closeAnyOpenQViews(); |
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 doesn't seem necessary. The showChatView call already does the job hiding any open views that are not the view being asked to be shown. See
amazon-q-eclipse/plugin/src/software/aws/toolkits/eclipse/amazonq/views/ViewVisibilityManager.java
Line 67 in 55bba2a
private static void showMutuallyExclusiveView(final String viewId, final String source) { |
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 won't include the AmazonQView though, which is the base view. It just handles the views inside of the base view. The base view itself remains 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.
Did not get it. Each of the two classes than implement the baseview are part of the mutually exclusive view.
Why would this need to handled separately from the mutuallyexclusive call instead?
BaseView cannot exist on its own as an abstract class
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.
Sorry, I should've picked a better name. But basically, the AmazonQView is the skeleton, and all the other views: ChatWebview, DependencyMissingView, and LoginView are all rendered inside it. These views are mutually exclusive, but the skeleton itself is not part of that process. It can remain open without any contents inside it, which is what happens when the window is switched. This logic finds all instances of the skeleton view in any of the open windows and closes them before opening a new skeleton view in some active shell.
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 logic if missing should be moved to within the showMutuallyExclusive call instead of adding it to a third separate location to manage and maintain long term
Also to clarify the skeleton only exists for the chat and login views but not for the other mentioned above
Issue #300
Description of changes:
This PR implements a fix to improve the behavior of Amazon Q views across multiple windows in the IDE.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.