Block further inline edits until previous one completes #1481
Conversation
Agree. And from the technical POV having the option allows for better flexibility in the future regarding having more complex multi tasking scenarios |
There was a problem hiding this comment.
Everything except the blocking to start new sessions looks good to me.
We should not stray from what the TypeScript does in VSCode without a clear justification, otherwise we end up with two overlapping implementations which start to interfere. Where we do want to stray from VSCode, we should go clean up the TypeScript side so there's a hook which will invoke a VSCode path, or a JetBrains path.
@steveyegge mentioned being able to relate edits to the FixupTask that caused them would make it possible to reach back to the related FixupSession. Do we still need that if streaming is off? And if it is what we need so we can get to a simpler, concurrent edits all the time model, then let's do it...
The VSCode API editing event is synchronous. We can maintain a shadow stack of an acting fixup and when we're publishing the edit event, consult the shadow stack to see whether a fixup is the cause.
If there's concurrent edits in JetBrains and Agent, the Agent edit may need to fail and be retried. Let's make an abstraction for applying retryable edits in TypeScript and have fixups (and eventually everything touching buffers) use it.
| Thread.sleep(100) | ||
| fun startNewSession(newSession: FixupSession, code: () -> Unit) { | ||
| activeSession?.let { currentSession -> | ||
| if (currentSession.isShowingAcceptLens()) currentSession.accept() else currentSession.cancel() |
There was a problem hiding this comment.
My $0.02, let's avoid the situation we had with chat repo picking where Kotlin and TypeScript have separate, uncoordinated representations of the same thing. TypeScript has some facility for multiple simultaneous edits, scheduling idle diff computation work to apply edits, autoaccept, etc. Anything it lacks we should add for the benefit of VSCode and JetBrains. If we want to do bespoke stuff for JetBrains (why?) then we should go do the work in TypeScript to refactor policy so it is out of the way first, like we did for the lens displays.
There was a problem hiding this comment.
@dominiccooney that change is needed specifically because we decided to temporarily diverge from VSCode implementation.
VSCode supports parallel edits, but Jetbrains currently does not. @steveyegge recently cleaned up FixupService and removed edits queue to reflect that fact. That is supposed to be temporary situation, and we wan to make it parallel after GA. But for now, if we only support one edit at a time, we need to deal with UI/UX consequences of that.
If we agree on partially blocking user actions based on the active task state (per my question and @danielmarquespt response) I can (and will :)) simplify this code because situation of two simultaneous edits won't be possible.
There was a problem hiding this comment.
if we only support one edit at a time
That's the issue, basically... the TypeScript extension supports multiple simultaneous edits, so we're accruing some tech debt here: the TypeScript side can't add anything that would cause simultaneous edits for JetBrains. The rest of the team is about to embark on an effort to increase edits/user-day, and we are surreptitiously adding this subtle constraint on them.
The other way to approach this would be to make a one-edit-at-a-time-mode in the TypeScript extension so when they run into the constraint the abstractions push back on them instead of JetBrains breaking.
I defer to your judgement on this one. Getting edits out from behind the flag to get some real world use and feedback is certainly valuable.
There was a problem hiding this comment.
The other way to approach this would be to make a one-edit-at-a-time-mode in the TypeScript extension so when they run into the constraint the abstractions push back on them instead of JetBrains breaking.
I'm worrying that it could turn out to be much more complicated and have much more edge cases than we think.
I do not feel it's worth spending that time a week+ before passing GA release to QA.
I will be happy to revisit it afterwards.
pkukielka
changed the title
pkukielka
force-pushed
the
pkukielka/deny-overlapping-edits
branch
2 times, most recently
from
2dce6e3
to
d006d52
Compare
| } | ||
| } | ||
| } | ||
| IgnoreOracle.getInstance(project).addListener(this) |
There was a problem hiding this comment.
@dominiccooney I simplified this, I do not see any reason to keep adding and removing this listener.
If there is any, please let me know.
There was a problem hiding this comment.
Should be fine... I was just keen to avoid references from something held on to by agent callbacks and the opaque JetBrains machinery. Seems like a recipe for leaks.
| Explain("Explain Code", KeyEvent.VK_E), | ||
| Smell("Smell Code", KeyEvent.VK_S), | ||
| Test("Generate Test", KeyEvent.VK_T) | ||
| enum class CommandId(val id: String, val displayName: String, val mnemonic: Int) { |
There was a problem hiding this comment.
The class is CommandId and it has id field now. That leads to commandId.id which looks weird. Can we remove Id from the class name?
There was a problem hiding this comment.
Let me do this either on a separate PR or at the end of the reviews... it is used in a quite a few places, I do not want to introduced such amount of noise on this PR.
There was a problem hiding this comment.
I think this is acceptable. In fact, such naming seems to be rather common in Kotlin and Scala code.
|
|
||
| ApplicationManager.getApplication().executeOnPooledThread { | ||
| FixupService.getInstance(project).waitUntilActiveSessionIsFinished() | ||
| cancellationToken.onFinished { |
There was a problem hiding this comment.
Does this call to onFinished override the call from the init block?
There was a problem hiding this comment.
Nope, they are stacked:
public void onFinished(Consumer<Boolean> callback) {
this.cancelled.thenAccept(
(isCancelled) -> {
try {
callback.accept(isCancelled);
} catch (Exception ignored) {
// Do nothing about exceptions in cancellation callbacks
}
});
}
pkukielka
force-pushed
the
pkukielka/deny-overlapping-edits
branch
from
d006d52
to
f6f229e
Compare
Fixes #1415
Changes
waitUntilActiveSessionIsFinished)cancellationTokenwhich allows us to signal (and wait for) session ending"In-progress" means until Accept lens appears.
If new edit is started before accepting previous on, previous one automatically gets accepted.
Test plan
Manual testing with Jetbrains plugin.