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(amazonq): open diff in a new tab when another modal is open #5289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xiaox-chen
Copy link

@xiaox-chen xiaox-chen commented Jan 22, 2025

fix(amazonq): /doc agent - fix open diff in a new tab when another modal is open
When another modal is open: Help -> Diagnostic Tools -> Activity Monitor. Keep focus on this modal. Diff was opened in a new modal
image
After this change, the diff always opens in a new tab.
generate Readme:
image

generate Readme with chat:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xiaox-chen xiaox-chen requested review from a team as code owners January 22, 2025 18:54
@@ -804,6 +804,7 @@ class DocController(
followUp = getFollowUpOptions(session.sessionState.phase)
)

this.toolWindow?.activate(null, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: second parameter is redundant, you can use activate(null) method instead
question: processOpenDiff is invoked from 2 places in the DocController, was this issue reproduced for both of them ?

Copy link
Author

Choose a reason for hiding this comment

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

Removed the second parameter

Good catch.
One place is after use click Generate readme.
The second one is generate readme using chat. Update this PR to fix both.

@@ -720,6 +720,7 @@ class DocController(
}

if (filePaths.isNotEmpty()) {
this.toolWindow?.activate(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

the intent of this is not clear. how does a reader know that this is a magic workaround to make the popup occur non-modal?

Copy link
Contributor

Choose a reason for hiding this comment

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

please see if com.intellij.diff.DiffDialogHints works

@@ -804,6 +805,7 @@ class DocController(
followUp = getFollowUpOptions(session.sessionState.phase)
)

this.toolWindow?.activate(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

leaky abstraction. shouldn't this be handled in the logic that makes the diff dialog appear? instead of the caller needing to know?

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the tool window does not exist?

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.

3 participants