Context menu enhancements

[tortmayr]

What it does

Enhances the memory webview to properly support context menu actions contributed via webview/context contribution point. This includes

• Augmenting the webview components with data-vscode-context custom data properties.
  These properties are used to provide additional context info (as JSON string). The composed context is then available in when conditions from webview/context contributions and as command argument when executing the associated command.

Provides the following context menu enhancements outlined in #51

• Copy selection: Copies the current text selection/datatable selection to the clipboard. Text selection has priority.
  If no text is selected the value of the selected datatable cell will be copied instead.
• Quick access to window configuration by
  • providing show/hide entries for variable & ascii column
  • show/hide for radix prefix
  • Show advanced Options command to show the advanced settings overlay
• Allow contributions from other extensions

How to test

Open a context menu in the memory webview. The display entries (Show/Hide xyz, Show Advanced Options) should always be visible and work as expected.

Open a context menu ontop of a variable entry in the variables column. The Test variable context entry should appear.
On click it will print the following to the memory output channel:

• Current webview selection (retrieved via the newly introduced command memory-inspector.get-webview-selection )
• The complete context that was applied when opening the context menu

(The TestVariableContext command/menu is a temporary menu to test the extension functionality during review and will be removed before merging)

Review checklist

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

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[tortmayr]

context_demo.mp4

Add additional context menu commands (also from external extension)

New context menu commands can be contributed via the webview/context menu extension point in an extension package.json.

"menus": {
      "webview/context": [
        {
          "command": "memory-inspector.show-advanced-display-options",
          "group": "display@4",
          "when": "webviewId === memory-inspector.memory"
        }
      ]

To restrict the context/menu to the memory inspector at the webviewId has to be restricted in the when clause.
Additional context keys are provided by the webview to further control the visibility:

• webviewSection: restrict to a certain subsection of the webview (memoryTable, optionsWidget or advancedOptionsOverlay)
  -columnId: set when the context menu was invoked on top of a table cell. (values: data|adress|ascii|variables)
• showRadixPrefix
• visibleColumns: string representation of the visible columns separeted by |. Alows regex restriction like visibleColumns =~ /\\|ascii\\|/"

Each cell can also apply value specific custom context keys e.g. name, value and reference for variable cells.

In addition, the context is also available as command argument in the handler of the corresponding command.
We also provide the webview message participant here. This enables sending requests/notification back to the webview inside of the command handler:

   vscode.commands.registerCommand(MemoryWebview.ShowAdvancedDisplayConfigurationCommandType, async (ctx: WebviewMenuContext) => {
               this.messenger.sendNotification(showAdvancedOptionsConfigurationType, ctx.messageParticipant, undefined);
           });

In addition,the memory-inspector.get-webview-selection command is available and can be used in external extension to retrieve the current text/dataTableSelection of the webview.

[colin-grant-work]

I'd probably prefer that we make this field nullable, as the default value isn't actually meaningful.

[tortmayr]

I also thought about that. The question is then how do we handle the potential undefined value?
This would mean the resulting global context object would conform to this interface.

export interface WebviewMenuContext {
    messageParticipant?: WebviewIdMessageParticipant,
    visibleColumns: string,
    showRadixPrefix: boolean,
}

So in theory, in a command handler for a context menu command we would always have to check whether ctx.messageParticipant is defined before sending a message to the webview. In practice this check is unnecessary, because the participant is only undefined on initial render and should be correctly set once the initial settings are transferred from the extension.

At some point we have to make the implicit assumption that the messageParticipant is set to the correct/valid value.
If would rather not have this on the user facing side (i.e. command context)

[colin-grant-work]

Fair enough. It's really just an artifact of the interaction between React and the webview lifecycle. I suppose the question is what it looks like to a user if the synchronization goes bad for some reason. Is it a no-op in the VSCode messenger package, or does it throw an error; if error, how transparent is it? Could we provide a better message if something did go wrong?

[tortmayr]

I quickly tested this. Using an invalid webview will be handeled by vscode-messenger silently (or at least not with a direct user-facing error message). A simple error message is logged in the debug console but that's it.

The messageParticipant is part of the inital settings notification that is dispached as soon as the webview is ready.
So in practice we can only end up with an invalid (emtpy id) messageParticipant if

• The webview never sends the ready notification
• or something goes wrong when retrieving the display settings from the vscode configuration.

Both cases are very unlikely, and if they should occur we probably have more pressing concerns than the invalid messageParticipant as the entire webview-extension communication flow is not working as expected.

If we want to consider these corner cases we should probably handle this on a more generic level e.g. freezing the webview and/or display a loading indicator until the initial settings are received. In addition, we could add timeout error handling for cases where the extension never receives the webviewReady notification. But this should probably be tackled in a follow-up PR.

[colin-grant-work]

Failing silently in unlikely circumstances is good enough for me. I was worried that it would throw an error, and if VSCode encounters an unhandled error during command execution, it'll show a very ugly dialog. As long as we avoid that, I'm happy.

[tortmayr]

Thanks for the swift review @colin-grant-work.

I have reworked the entire approach for toggling columns.
Instead of a visibleColumns string we now sent explicit visibility flags for each (configurable) column and construct
the visibileColumns array on the extension side from the given context.
This also makes usage in the when clauses easier. No more regex comparison needed 😉.

In addition, I created common WebviewContext types for the different context levels that can be provided by the webview (app/column/variable).

[colin-grant-work]

I'm satisfied 👍. Only sticking point is the question of separate on/off vs. toggle commands for the columns, if you have an opinion @jreineckearm.

[jreineckearm]

Functionality in this PR looks really good, @tortmayr !

Only sticking point is the question of separate on/off vs. toggle commands for the columns

I agree with @colin-grant-work that a single toggle would be probably better. Just wondering how difficult it would be to have something like this, where the context menu entry indicates the current toggle state via a check mark?

A few comments from testing:

• For me, the default entries Cut, Copy, Paste don't work. Should/can we remove Cut and Paste? Or will this be wired in when we add editing memory contents (@martin-fleck-at ) ?
• Keyboard shortcut Ctrl+C works nicely. Ctrl+X doesn't (in the sense of behaving like CTRL+C), but see above comment.
• If I select a complete row, then I see line breaks inserted when I paste it into a text editor window. It probably should keep it as a single line in the clipboard. Especially, if you select multiple lines where line break should be inserted between rows.
  
  becomes
  

[haydar-metin]

@jreineckearm

For me, the default entries Cut, Copy, Paste don't work. Should/can we remove Cut and Paste? Or will this be wired in when we add editing memory contents (@martin-fleck-at ) ?

For now, it was only planned to allow editing directly by clicking on the groups and not through Cut/Copy and Paste. This would require us to concretize how we want to handle such behavior, e.g., what to do on invalid/less/more data and what the cut operation does?. Consequently, we can remove Cut and Paste without any problems from my side.

[tortmayr]

I agree with @colin-grant-work that a single toggle would be probably better. Just wondering how difficult it would be to have something like this, where the context menu entry indicates the current toggle state via a check mark?

I tried that initially, but unfortunately we are restricted by the limits of the VS Code API here. Theoretically it is possible to define
an icon for a command. However, they are only supported in a very limited subset of contribution points (e.g. the editor/title menu). Unfortunately this is not supported in context menus 😞 .

We could use a workaround using text emojs to visualize the toggle state e.g. ☐ ☑

Note that since conditional labels are not supported this would still require two commands per column:

       {
        "command": "memory-inspector.show-variables-column",
        "title": "☐ Variables Column",
        "category": "Memory",
        "enablement": "webviewId === memory-inspector.memory"
      },
      {
        "command": "memory-inspector.hide-variables-column",
        "title": "☑ Variables Column",
        "category": "Memory",
        "enablement": "webviewId === memory-inspector.memory"
      }
  

For me, the default entries Cut, Copy, Paste don't work.

Looks like the not working context menu entries is a know issue for Windows. There is an open bugfix PR: microsoft/vscode#206529 and looking at the comments its about to get merged.
So this should be fixed with next VS Code release.

Should/can we remove Cut and Paste? Or will this be wired in when we add editing memory contents (@martin-fleck-at ) ?

It's not possible to remove individual entries of the default copy/paste menu. But we could deactivate the default items completely by adding "preventDefaultContextMenuItems": true to the data-vscode-context.
However, this also means we have to manually implement the copy/cut/paste menu items (and the logic for which UI elements they should be active). Not sure if it's worth the effort right now.

Keyboard shortcut Ctrl+C works nicely. Ctrl+X doesn't (in the sense of behaving like CTRL+C), but see above comment.

This could be fixed easily fixed if necessary (i.e. if we don't opt for removing the cut command)

If I select a complete row, then I see line breaks inserted when I paste it into a text editor window. It probably should keep it as a single line in the clipboard. Especially, if you select multiple lines where line break should be inserted between rows.

Looks like this behavior was indirectly introduced with the autofit PR #82.
Before that it used to work as you described i.e. maintaining white spaces. This can also be seen in the attached Video which was taken before #82 got merged.
@haydar-metin Any ideas why the autofit feature changes the copy behavior?

[haydar-metin]

@tortmayr yes, it changes how the groups are rendered to make the resizing (adding and removing groups) more smoothly 30ab368. It seems, flex has an influence on how the copy works. We could undo the changes from there, however, then decreasing the width of the webview by a lot will make a second row visible briefly.

[jreineckearm]

@haydar-metin , @tortmayr

This would require us to concretize how we want to handle such behavior, e.g., what to do on invalid/less/more data and what the cut operation does?. Consequently, we can remove Cut and Paste without any problems from my side.

I'd suggest to remove cut and paste from the context menu. If having them at all, I'd limit that to when editing of a group is active. Would suggest to move that discussion to the according issue.

We could use a workaround using text emojs to visualize the toggle state

Thanks for explaining this in more detail. Sounds like it is not as straight forward. In which case we shouldn't force it now. Let's go with a single toggle command. The columns state is in fact quite obvious without it. Just would have been the sugar on top. :-)

However, this also means we have to manually implement the copy/cut/paste menu items (and the logic for which UI elements they should be active). Not sure if it's worth the effort right now.

Hm.....probably we should just remove all of them then? Do the keyboard shortcuts then still work?

We could undo the changes from there, however, then decreasing the width of the webview by a lot will make a second row visible briefly.

How bad is the flickering in reality? What would I need to change locally to see the behavior you mean?

[haydar-metin]

@jreineckearm

How bad is the flickering in reality? What would I need to change locally to see the behavior you mean?

You can easily test it by disabling the css provided here either in code or in the devtools

30ab368#diff-1bcd15bcaf7de0329d93551add5ee225916db13b63a0f94b3f486f3443512f57R26

Then decrease the width of the webview. If you are fast enough, a second row will be rendered briefly due to the word breaking. That happens because there is a delay between rendering and until our fitting executes.

To be honest, it wasnt that bad. I would say, the copy operation is more of a deal breaker than that. However, we should still investigate the issue.

[jreineckearm]

TBF: I am having a hard time to reproduce the duplicate column with that local change. But I do see an improvement. Now I only have linebreaks after each column. I guess it will be harder to remove those as well so that there is only a linebreak on a new row?

That's what I see when pasting two rows into a new VS Code text file after commenting out the class style that you pointed me to.

0x2004002c	
dfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdf
keyboard, dir	................................
0x20040034	
dfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdf
keyNum, keyMax	................................

Interestingly, it behaves differently when I try to paste directly into this GitHub comment form. Looks like some underlying HTML is included?

<html>
<body>
<!--StartFragment-->
0x2004002c | dfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdf | keyboard, dir | ................................
-- | -- | -- | --
0x20040034 | dfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdfdfdfdfcfcfdfdfdf | keyNum, keyMax | ................................

<!--EndFragment-->
</body>
</html>

[tortmayr]

@jreineckearm The latest commit should address all open issues
The PR has been rebased on the latest master (and squashed to make conflict resolving easier).

With this change we

• Hide the default copy/paste/cut context menu entries
  (Keyboard short cuts still work as expected)
• Also handle cut events on the table so that the ctrl+x behaves like copy
• Fixed the issue regarding copying text selection introduced with Introduce Autofit choice for groups per row #82 by
  • Using a span instead of a div for the root element of the data column. (A div will introduce additional line breaks in the text selection, whereas a span does not)
  • Add special handling to the onCopy listener of the memory table for the autofit case. If autofit is enabled,
    the listener removes the groups-per-row-autofit class from the table to copy the correct selection and then adds it again
  • Use single toggle commands for radix prefix, variables and ascii column
  • Remove the Test Variable Context command

[jreineckearm]

Thanks a lot, @tortmayr , works nicely now!

[tortmayr]

@jreineckearm I have rebased the change to resolve conflicts and also removed the workaround/fix for the copy&paste behavior again. This will be addressed with #99.
Could you pleas have another look and then ping someone with write access to get this merged before more conflicts are introduced?

[jreineckearm]

@tortmayr , sorry, the line breaks between columns are back with the latest push. :-(

[tortmayr]

@colin-grant-work
Yes, that's expected. Proper copy& paste without linebreaks will work again once #99 is merged.

[jreineckearm]

My bad, @tortmayr . Didn't read the previous comment thoroughly enough where you stated this before.
@thegecko , could you please merge this one? This will help us to confirm that #99 fixes the line break issue.