Ensure addActivityItemStream()
doesn't lose Output
vs Error
stream specialization
#4848
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #4519
In
addActivityItemStream()
, either aActivityItemOutputStream
orActivityItemErrorStream
always goes in, but it was possible for a "generic"ActivityItemStream
to come out here when we make one from theremainderText
after the last newline:positron/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts
Lines 118 to 136 in e51332d
That generic
ActivityItemStream
would get set here:positron/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts
Lines 110 to 116 in c9504d3
And then pushed onto
this._activityItems
positron/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts
Lines 138 to 139 in c9504d3
The problem is that our Console code doesn't know how to deal with a generic
ActivityItemStream
. It must be a specializedActivityItemOutputStream
orActivityItemErrorStream
!positron/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx
Lines 46 to 49 in c9504d3
If a generic
ActivityItemStream
slipped through, the output would never appear in the Console, making it look "dropped", as reported in #4519.I like how we have a generic
ActivityItemStream
class that we specialize for the output/error types, so I've fixed this bug by using polymorphicthis
to allowaddActivityItemStream()
to take and return objects of the correct specialized type.Ideally we'd simply be able to use
new this.constructor()
in place ofnew ActivityItemStream()
to use the polymorphic constructor when we need to create new objects, but due to some well known weirdness in typescript, you have to manually cast it to the constructor type, sonewActivityItemStream()
wraps that up.I've also made the
ActivityItemStream
constructorprotected
to avoid us using it by accident in the codebase. We should always be creatingActivityItemOutputStream
orActivityItemErrorStream
instead.Before 😢
Screen.Recording.2024-09-30.at.2.18.03.PM.mov
After 🥳
Screen.Recording.2024-09-30.at.2.16.33.PM.mov