Closed tortmayr closed 4 months ago
Thanks, @tortmayr !
Handling looks good in general (see one comment about the code changes).
Two corner cases which might be worth to address if effort is reasonable:
Address
field => Actual
is rendered underneath the address field
Length
field (renders Actual: 256
)Address
field (spaces or any value) => Offset
and Length
get populated with value 0
.
I don't know what's involved to fix item 3. It probably is the one with the least of the three findings.
Enter only spaces in the Address field => Actual is rendered underneath the address field
It seems like each of the three fields has its own actual
field that is rendered if the value defined in the form field diffs from the actual set value:
We typically only see the actual field of the offset
field because that's the one that naturally divergates when fetching additional rows before or after.
I guess the idea is to give the user feedback about the current value if they input an invalid value on accident.
IMO we have two options here either
1) we remove the actual fields for address
or and length
2) or we adapt the render logic and only render them if we have valid (i.e. non-empty) table data.
Currently the options widget does not have any knowledge about the table contents so we would have to pass this info in as property.
@jreineckearm What do you prefer?
Thanks, for looking into my feedback, @tortmayr !
I guess the idea is to give the user feedback about the current value if they input an invalid value on accident. IMO we have two options here either
we remove the actual fields for address or and length or we adapt the render logic and only render them if we have valid (i.e. non-empty) table data. Currently the options widget does not have any knowledge about the table contents so we would have to pass this info in as property.
Thanks, I see now. Ideally, the options widget shouldn't know about the window status.
Revisiting this part again, I believe the label "Actual" is a bit ambiguous: it could mean both the actual value of the window contents and the last read argument. It is the latter. But let's not alter the label with this PR.
Having had a bit more of a play with it, I think what confused me was the "Actual:" label below "Address" without a value. Especially when just adding spaces to the edit field (the trimming part).
It could be enough to solve that confusion by trimming the input for comparison (and actually before passing to the memory read, I saw unexpected errors with our backend if passing in something untrimmed, though that may only be our debug adapter), and only show "Actual" below "Address" if the memoryReference
in activeReadArguments
is not an empty string.
Except from the initial state, memoryReference
in activeReadArguments
can never be the empty string because the options widget blocks an empty address field.
Does this make sense?
@jreineckearm I have addressed your feedback.
webview-types.ts
What it does
Avoid dispatching of a fetch memory request if there is not enough information available i.e no memory address is set. Add validation for the default values of the memory address widget. => Prompts a validation error if the memory inspector has been openend for an empty/not set address
Fixes #126
How to test
Start a debug session and open the memory inspector via command palette Observe that
Once the user manually changes the address, the error disappears and memory is fetched normally.
https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/assets/2311075/6a630ee6-0ae8-4a34-81f5-ca87fd958a05
Review checklist
Reminder for reviewers