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

Add a text field for entering multiple remote repositories at once #1308

Merged
merged 48 commits into from
Apr 16, 2024

Conversation

dominiccooney
Copy link
Contributor

@dominiccooney dominiccooney commented Apr 11, 2024

This facilitates bulk editing of multiple remote repositories, with an autocompleter and fuzzy search to make it easy to find repositories.

This needs to land after sourcegraph/cody#3775 and roll past that Agent version.

Fixes #982

Test plan

Manually tested with sourcegraph/cody#3775:

  • Switch from an Enterprise to dotcom account and back. The enhanced context selector in the chat panel should change presentation.
  • Click the pencil and enter repository names. Duplicates should have yellow underlines. More than 10 repositories should have yellow underlines. Unknown repositories should have red underlines. Note, in JetBrains 2022.x, Annotators are not DumbAware: The underlines will only appear after the project finishes indexing.
  • Ctrl-Space should open an autocomplete with fuzzy search of remote repositories.
  • Command/Ctrl-Enter should update the selected repositories in the enhanced context selector. The entered text, including errors, should persist for the life of the chat panel to facilitate incremental editing; new chat panels should pick up a list of repositories separated with newlines.
  • Opening and closing the enhanced context selector tree view should resize the bottom of the panel. Checking and unchecking the tree nodes should influence whether enhanced context is used for chats (root node) or for specific repositories (leaf nodes.) Note, the repository associated with the open workspace is included implicitly when enhanced context is enabled.

@dominiccooney
Copy link
Contributor Author

dominiccooney commented Apr 11, 2024

@pkukielka if you could PTAL, I would love your guidance here.

I'm unsure about Kotlin coroutines here, maybe I should revert the coWithAgent and go back to threads. Feels wasteful to burn a thread if we are already on a background thread for completion candidates. And coroutines are easy to use. On the other hand, some of these are marked as experimental (in 2022.x?) and the integration with the cancellation exceptions is awkward.

What's the rules for tagging methods with needs EDT/background/etc.? I am sure I have this wrong but couldn't find something succinct describing the right thing to do. Was also unsure about read and write transaction scopes, for example, when the popup creates a PsiFile and PsiDocument, should it initiate a write?

  • Worked out how to test this with 2024.1, etc. and updated CONTRIBUTING.md
  • I have dug through APIs and best I can tell the read/write are fine, but doc comments about does/doesn't require read are scant.
  • Is there a way to run the plugin in a validating mode for read/write? Found the "Thread Access Info" plugin but state assessment (read allowed, write allowed) is useless without understanding what requires read and write.

Pointer to releasing and rolling agent would be appreciated although I'm happy to dig for it.

Is there a formatter? IntelliJ seemed to be all over the place.

  • spotlessApply

@dominiccooney dominiccooney force-pushed the dpc/repo-picker branch 2 times, most recently from 3ef81d9 to ee6cbb7 Compare April 15, 2024 04:50
restartIfNeeded: Boolean,
callback: suspend (CodyAgent) -> T
): T {
if (!CodyApplicationSettings.instance.isCodyEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That change will require testing all the features with disabled cody:

  • chat, @-files
  • commands
  • autocompletion
  • enhanced context
  • edit tasks
  • account management panel

I do not trust we properly handle exeptions everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't because the withAgent wrappers check this boolean. We might get one exception if you disable agent between the time of that check and the time this coroutine runs.

@pkukielka
Copy link
Contributor

pkukielka commented Apr 15, 2024

@pkukielka if you could PTAL, I would love your guidance here.

I'm unsure about Kotlin coroutines here, maybe I should revert the coWithAgent and go back to threads. Feels wasteful to burn a thread if we are already on a background thread for completion candidates. And coroutines are easy to use. On the other hand, some of these are marked as experimental (in 2022.x?) and the integration with the cancellation exceptions is awkward.

I think at the very least we need to test it on the lowest version which we officially support which is:
<idea-version since-build="221.5080.210"/>

What's the rules for tagging methods with needs EDT/background/etc.? I am sure I have this wrong but couldn't find something succinct describing the right thing to do. Was also unsure about read and write transaction scopes, for example, when the popup creates a PsiFile and PsiDocument, should it initiate a write?

For the general guidance please look at this:
https://plugins.jetbrains.com/docs/intellij/general-threading-rules.html

As for the annotations, they are not fully working yet and the documentation is sparse, but we are using them for documentation purposes and with the hope of getting some benefits in future:

From ReqiresEdt.java javadoc:

Methods and constructors annotated with RequiresEdt must be called from the Event Dispatch Thread only. Parameters annotated with RequiresEdt must be callables and are guaranteed to be called from the Event Dispatch Thread.

Aside from a documentation purpose, the annotation is processed by the org.jetbrains.jps.devkit.threadingModelHelper. The plugin instruments annotated elements with Application.assertIsDispatchThread() calls to ensure annotation's contract is not violated at runtime. The instrumentation can be disabled by setting generateAssertion() to false.

Important: the instrumentation has limitations. Please read the docs of the org.jetbrains.jps.devkit.threadingModelHelper to learn about them.
See Also:
General Threading Rules , Application.assertIsDispatchThread(), Application.invokeLater(Runnable, ModalityState), Application.invokeAndWait(Runnable, ModalityState)

And then from plugins/devkit/jps-plugin/src/org/jetbrains/jps/devkit/threadingModelHelper/package-info.java:

/**
 * <h2>Threading Model Helper</h2>
 * <p>
 * Instruments methods annotated with
 * <ul>
 * <li>{@code @RequiresEdt}
 * <li>{@code @RequiresBackgroundThread}
 * <li>{@code @RequiresReadLock}
 * <li>{@code @RequiresWriteLock}
 * <li>{@code @RequiresReadLockAbsence}
 * </ul>
 * by inserting
 * <ul>
 * <li>{@code ThreadingAssertions.assertEventDispatchThread()}
 * <li>{@code Application.assertIsNonDispatchThread()}
 * <li>{@code Application.assertReadAccessAllowed()}
 * <li>{@code Application.assertWriteAccessAllowed()}
 * <li>{@code Application.assertReadAccessNotAllowed()}
 * </ul>
 * calls accordingly.
 * <p>
 * To disable the instrumentation, use the {@code tmh.generate.assertions.for.annotations} key in the Registry.
 *
 * <h3>Limitations</h3>
 *
 * <ul>
 * <li>Only Java code is instrumented,
 * <a href="https://youtrack.jetbrains.com/issue/IDEA-263465">Kotlin instrumentation is planned</a>.
 * </ul>
 */

Is there a way to run the plugin in a validating mode for read/write? Found the "Thread Access Info" plugin but state assessment (read allowed, write allowed) is useless without understanding what requires read and write.

Unfortunately I'm not aware of any static analysis tool which would help us ensure correctness there,

Pointer to releasing and rolling agent would be appreciated although I'm happy to dig for it.

If you want to upgrade agent version in jetbrains all you need is to bump commit there:
https://github.com/sourcegraph/jetbrains/blob/main/gradle.properties#L27
No release or any special procedure on cody side is needed.
But I just bumped version on my other PR so you should be good after the rebase.

@pkukielka
Copy link
Contributor

@dominiccooney Search works very quickly and is really snappy!

But I run into couple of other issues. For the reference I tested it on IJ 2022.1:

  • Edit button is clickable only if I click on its right side. On the Left side it does not react at all with proper on hover behaviour.
  • If I add another repo it does not save it. Do I need to somehow save manually or should it happen automatically? Anyway, I'm too dumb to figure it out and some users might not be smarter then me :)
  • When I click edit cursor is always at the beginning of the edit box. Would be nice if it would be at the end or in the new line, so I can start typing immediately.

@dominiccooney
Copy link
Contributor Author

I think at the very least we need to test it on the lowest version which we officially support which is:

👍 I've been primarily testing on that.

Edit button is clickable only if I click on its right side. On the Left side it does not react at all with proper on hover behaviour.

I have seen exactly this behavior but I can't reproduce it consistently. @danielmarquespt , can we keep an eye on this in user testing? If it happens a lot we should consider going back to the horizontal toolbar.

If I add another repo it does not save it. Do I need to somehow save manually or should it happen automatically? Anyway, I'm too dumb to figure it out and some users might not be smarter then me :)

@danielmarquespt this is good product feedback. FWIW @abeatrix over here did not grok to hit the pencil to edit (I've now changed the minimum sizes so the pencil is, at least, always visible.) Here @pkukielka isn't used to command-enter to commit.

FWIW I confirmed that the git log search widget behaves as you have spec'ed... you need to cmd-enter to commit.

Let's consider not sticking to JetBrains idioms here. The idioms don't seem that strong.

When I click edit cursor is always at the beginning of the edit box. Would be nice if it would be at the end or in the new line, so I can start typing immediately.

100% felt this myself, good call. I have put the cursor at the end of the first line. Feels good because you can see the top of the list, and it is a convenient place to start editing if you want to add.

@pkukielka
Copy link
Contributor

pkukielka commented Apr 16, 2024

@danielmarquespt this is good product feedback. FWIW @abeatrix over sourcegraph/cody#3775 (review) did not grok to hit the pencil to edit (I've now changed the minimum sizes so the pencil is, at least, always visible.) Here @pkukielka isn't used to command-enter to commit.
FWIW I confirmed that the git log search widget behaves as you have spec'ed... you need to cmd-enter to commit.

Let's consider not sticking to JetBrains idioms here. The idioms don't seem that strong.

Oh, cmd+enter. That is totally non-idiomatic for IntelliJ. Is the any reason we would like to do it that way?
I'm kind of against transplanting VSC UX patterns to IJ, it's like trying to force Android users to use iPhone UX, or vice versa - it's deemed to fail. At very least I would change it to autosave on panel close, or something like that.

I think good example of similar native component you can see in Run/Debug configurations:

image

I see there is a tooltip (I apparently need to learn to read) but TBH it's not very visible because of low contrast so it is still possible to miss it especially on smaller screens:

image

Actually @danielmarquespt I think maybe we should work a bit on our colour palette.
I do not wear glasses, but if I squeeze my eyes ever so slightly I have trouble to read any of that grey text.
I think anyone with vision impairment would have serious trouble.

I have seen exactly this behavior but I can't reproduce it consistently. @danielmarquespt , can we keep an eye on this in user testing? If it happens a lot we should consider going back to the horizontal toolbar.

I can reproduce it 100% of times. I will try to check if I can track what is causing it.

@pkukielka
Copy link
Contributor

Also minor thing and definitely not a blocker:
If I have few repos added, and I go back to editing one of them, deleting some letters from the end of the url does not cause any suggestions to show. But if I start adding any letters it appears immediately.

@dominiccooney
Copy link
Contributor Author

dominiccooney commented Apr 16, 2024

@pkukielka

Oh, cmd+enter. That is totally non-idiomatic for IntelliJ. Is the any reason we would like to do it that way?

We picked this up from the same widget in the git log viewer, author filter.

I see there is a tooltip (I apparently need to learn to read) but TBH it's not very visible because of low contrast so it is still possible to miss it especially on smaller screens ... I think maybe we should work a bit on our colour palette.

100% agree the contrast is way too low. On the other hand, this is the theme! We're not picking these colors.

I can reproduce it 100% of times. I will try to check if I can track what is causing it.

🙏 I just saw it again for a while, and then it disappeared again. @danielmarquespt noticed we pack the component tighter than the git log viewer does. Confirmed that they are both ActionViewToolbarImpls.

@danielmarquespt wants to add the action to the global actions panel, so when we do that we can switch to using ActionManager.createActionToolbar the same as the git log viewer does.

@pkukielka is this good to land? It would be great to get it committed and iterate on the details in follow ups.

@pkukielka
Copy link
Contributor

@pkukielka is this good to land? It would be great to get it committed and iterate on the details in follow ups.
Yea, I think nothing from this list is blocking merge.

BTW now edit button started to work for me as well and I cannot force it to be problematic agen 🙃

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@dominiccooney dominiccooney merged commit 2579d69 into main Apr 16, 2024
5 checks passed
@dominiccooney dominiccooney deleted the dpc/repo-picker branch April 16, 2024 09:36
@dominiccooney
Copy link
Contributor Author

Thank you for the review 🙇 I have filed #1322 for the follow-up feedback.

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.

Enhanced Context UX improvements
2 participants