Improving the expression evaluator #798
Conversation
| @@ -58,14 +59,15 @@ public ErlangStackFrame(@NotNull ErlangXDebugProcess debugProcess, | |||
| @Nullable | |||
| @Override | |||
| public XDebuggerEvaluator getEvaluator() { | |||
| return new XDebuggerEvaluator() { | |||
There was a problem hiding this comment.
Could you please comment this change?
| myDebuggerNode.evaluate(expression, traceElement); | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized void handleEvaluationResponse(OtpErlangObject response) { | ||
| if (myEvalCallback != null) { | ||
| myEvalCallback.evaluated(ErlangXValueFactory.create(response)); | ||
| mySession.resume(); | ||
| String error = null; |
There was a problem hiding this comment.
As for me, it's better to extract a separate function and assign it to the error variable.
| if (response instanceof OtpErlangAtom && ((OtpErlangAtom) response).atomValue().equals("Parse error")) { | ||
| // it is a parsing error | ||
| error = "Parse error"; | ||
| } else if (response instanceof OtpErlangTuple) { |
There was a problem hiding this comment.
Please use
if () {
}
else {
}
style.
|
@wk8 Any updates? |
|
@ignatov : sorry, I actually don't like this change, there must be a better way of doing this. I need to get back to the drawing board, but haven't had time yet. Soon! :) |
wk8
changed the title
|
@ignatov : made a little better, plus addressed your comments, could you please have another look? :) Thanks! |
wk8
force-pushed
the
wk8/eval_fixes
branch
3 times, most recently
from
b823e9f to
e5c4f49
Compare
* right after evaluating an expression, the erlang debugger will trigger a new "breakpoint reached" event; unfortunately, that means this plugin will try to concurrently render the new state of the bindings as well as the result from the evaluation, which Intellij doesn't seem to handle too well, resulting in the debugging session losing track of what the current frame is... so to fix that we just ignore the next "breakpoint reached" event occuring in an Erlang process right after evaluating an expression if it's the same breakpoint, in the same process. Better ideas welcome! * checking whether the evaluation resulted in an error, and nicely formatting it in if that's the case
|
@ignatov Any updates? :) thanks! |
|
Thanks for the ping. |
|
@ignatov any chance you'd have some time to review this in the coming days? :) |
| ErlangProcessSnapshot processInBreakpoint = ContainerUtil.find(snapshots, erlangProcessSnapshot -> erlangProcessSnapshot.getPid().equals(pid)); | ||
| assert processInBreakpoint != null; | ||
|
|
||
| boolean ignoreIfSame = myIgnoreNextBreakpointIfSame; | ||
| myIgnoreNextBreakpointIfSame = false; |
There was a problem hiding this comment.
A bit tricky assignments, do you need that because of the syncronization?
| // unfortunately, that means this plugin will try to concurrently render the new state of the bindings as | ||
| // well as the result from the evaluation, which Intellij doesn't seem to handle too well, resulting in | ||
| // the debugging session losing track of what the current frame is... so we just ignore the next "breakpoint | ||
| // reached" event after evaluating an expression if it's the same breakpoint - better ideas welcome! |
There was a problem hiding this comment.
Well that's the thing, I wasn't able to understand exactly why Intellij doesn't handle it well when the "breakpoint reached" event is fired right after an "eval result" event comes in, but it really doesn't; hence the need to ignore that "breakpoint reached" event. But at the same time, we don't want to ignore just any such event: it's very possible that, while evaluating an expression, you hit another breakpoint - and then we do want it to trigger.
Which results in this condition.
But I said, better ideas welcome ;)
|
@ignatov : any chance you'd have a few moments to spare to review this? :) thanks!! |
|
@ignatov sorry for pinging you again, it's that time of the month ;) (happy new year btw!) |
|
Could you please provide a small sample for reproducing, ideally a single file and describe steps to reproduce, I'd like to try your patch before the merge. Right now it looks a bit hacky, and it will be great to dive into it. |
|
@wk8 I'd like to merge the PR, could you please provide a small sample for reproducing. |
"breakpoint reached" event; unfortunately, that means this plugin will try to
concurrently render the new state of the bindings as well as the result from
the evaluation, which Intellij doesn't seem to handle too well, resulting in
the debugging session losing track of what the current frame is... so to fix
that we just ignore the next "breakpoint reached" event occuring in an Erlang
process right after evaluating an expression in that process. Better ideas
welcome!
it in if that's the case