Separate REPL evaluation from it's result; fixes #79196 #80422
Conversation
|
@isidorn Please take a look. I went with two separate objects strategy, as you were suggesting. Thank you! |
|
@dgozman nice work, I really like it. Due to that I suggest the following:
This new class and the
Let me know what you think. Thanks a lot! |
|
@isidorn Thank you for detailed feedback! I followed your suggestions, please take another look.
|
| } | ||
| } | ||
|
|
||
| export class ReplEvaluationResult extends ExpressionContainer implements IReplElement, IExpression { |
There was a problem hiding this comment.
I do not think ReplEvaluationResult should implement IExpression and it should not have a public name which we do not use anywhere.
I see that you are passing ReplEvaluationResult to renderExpressionValue, but can we pass just it's value instead.
I think we get rid of the name it will get clearer that the ReplEvaluationResult is just the result
I might be missing something here, but idealy we can simplify this.
There was a problem hiding this comment.
Unfortunately, just passing value to renderExpressionValue does not work, since it expects some specific classes and properties.
I lifted value, type and valueChanged from IExpression to IExpressionContainer, since these are actually implemented by ExpressionContainer. This way only IExpression has a name, but we reuse value handling and make renderExpressionValue take IExpressionContainer. What do you think?
|
This looks great, thanks a lot!
Thanks again! |
REPL now has two elements: one for the evaluation text added immediately (ReplEvaluationInput), and another for the evaluation result once available (ReplEvaluationResult).
There was a problem hiding this comment.
Thank you for thorough review, @isidorn. I made the changes, please take another look.
- You did changes in the
getHeightmethod and they make sense, however can you please test that the REPL scroll position behaves as expected when new output comes. Here's an example of a bug #71737 which we can potentially break by changing thegetHeight
I played with it, and I cannot reproduce #71737 with this PR. That said, when repl scroll is not at the very bottom, it does sometimes jump by 1-2 pixels when new output arrives. I can repro the same without this PR though.
- Did you run the smoke test so we verify we did not break anything with the css names? Here's how to run it https://github.com/Microsoft/vscode/blob/master/test/smoke/README.md
Yep, see the changes in debugSmoke.ts.
- Is it possible to write a unit test that captures these new behavior so we are sure we do not break it in the future? Adding an element immediatly adds it to the repl, and once it is evaluated we check there is an additional element?
Perhaps I am missing something, but I didn't find a unit test which does have an actual DebugSession to test the whole pipeline. The ReplEvaluationInput being added immediately is already covered in debugModel.test.ts. Any pointers are welcome.
| } | ||
| } | ||
|
|
||
| export class ReplEvaluationResult extends ExpressionContainer implements IReplElement, IExpression { |
There was a problem hiding this comment.
Unfortunately, just passing value to renderExpressionValue does not work, since it expects some specific classes and properties.
I lifted value, type and valueChanged from IExpression to IExpressionContainer, since these are actually implemented by ExpressionContainer. This way only IExpression has a name, but we reuse value handling and make renderExpressionValue take IExpressionContainer. What do you think?
|
Great work, I like that we moved those attributes up to IExpressionContainer. I also tried out your changes and tehy look awesome to me. Merging in. Thanks! |
REPL now has two elements: one for the evaluation text added immediately (EvaluationReplElement), and another for the evaluation result once available (Expression).