Add validation for invalid memory references

[tortmayr]

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

• no memory fetch request is sent to the debug adapter (can be verified via Memory Inspector Output)
• A validation error is displayed for the memory address field because the memory is undefined

Once the user manually changes the address, the error disappears and memory is fetched normally.

demo_val.mp4

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the contribution guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the code of conduct

[jreineckearm]

Would it make sense to move this into a more common place? Like ../common/messaging or ../common/memory? Looks like ReadMemoryArguments is defined in ../common/messaging.

[tortmayr]

IMO I don't see any benefit in doing that.
The default constants defined in the memory-webview-view (MEMORY_DISPLAY_CONFIGURATION_DEFAULTS,DEFAULT_SESSION_CONTEXT etc.) only serve the purpose of constructing the initial state for the App component. They are an implementation detail of ( and scoped to) the webview/react app and should not leak into other components.

[jreineckearm]

OK, not a too strong opinion about this. The main reason for raising this was to avoid a direct dependency between the memory-webview-view and the options-widget.

[colin-grant-work]

I agree with @jreineckearm that it would be better to move this to a constants file (e.g. view-types.ts) to avoid circular dependencies.

[jreineckearm]

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:

1. Enter only spaces in the Address field => Actual is rendered underneath the address field
   
2. Similar for the Length field (renders Actual: 256)
3. If you clear all fields from left to right, then change the content of the 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.

[tortmayr]

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?

[jreineckearm]

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?

[tortmayr]

@jreineckearm
I have addressed your feedback.

• DEFAULT_READ_ARGUMENTS have been moved to common webview-types.ts
• Trim address value before passing to memory read. (and before validation/comparison)
• Don't show option field hints ("Actual: ...) if the webview is still in initial state

[jreineckearm]

Thanks, @tortmayr ! The behavior looks great now! :-)
Also thank you for moving the default arguments to a common file.

Good to go with this change