Markdown Open in Preview not working

[isidorn]

Open in preview for markdown in explorer context menu is not working.
@bpasero found out that we try to send over the full editor context to extensions(image).

The issue is that for the context of the commands in the explorer we are seting the resource as the first argument and the IExplorerContext as the second. This contains the EditorInput which can not be serialized to JSON.
We should only send the first argument over to extension the resource. @jrieken Do you think it makes sense to limit sending only the first argument to extensions, and if yes could you give me a pointer?
If that is not possible we should look into replacing the EditorInput with something else in the IExplorerContext since the whole EditorInput is probably not needed.

just fyi @mattbierner in case somebody complains about markdown preview not working

[jrieken]

and if yes could you give me a pointer?

That information doesn't exists and won't be easy to add because most (extension) commands are unknown until their first invocation (because of lazy loading).

Not yet sure that passing multiple arguments is a good idea in general, but most importantly the first argument must be the URI as we made that API'ish. Then bleeding more (explorer internal) information to extensions is questionable. So, why do we need to pass the IExplorerContext around? Why can command not query the explorer for that information (given an URI)?

[isidorn]

Not all editors have an resource URI, and more importantly in the open editors list, a uri can be opened multiple times, so we need to pass a pair of URI and a Group to know the exact Open Editor the command should work on.

[jrieken]

so we need to pass a pair of URI and a Group to know the exact Open Editor the command should work on.

Which in theory you can all do with an URI. But yeah understood. However it seems to be specific to Open Editors...

@isidorn @bpasero what elements can appear in the explorer that don't have a URI? same question about the open editors view. And then does it makes sense to fill the gaps? Inventing some context objects and passing them to extensions is API, so care must be taken. I'd then rather opt to skip such objects...

[isidorn]

@jrieken a Welcome screen can show in the Open Editors which does not have a URI. As for the explorer I believe we could cover everything with a URI.

Let me check if I could refactor the explorer commands to only work with a URI.
As for the Open Editors we can leave them as they are since it is not exposed to extensions.

[isidorn]

I changed the explorer such that we only pass the uri around and everything seems to work fine.
We have only lost the functionality that alt + context menu delete = permant delate.

@bpasero check out my changes. Now for all the explorer commands I always get the focused element from the list via the listService. This works great because these commands are shared via context menu and keybindigs (example: rename via menu and F2). I do not even look at the passed uri.

Open Editors I have left untouched. I can change that if we decide that we want to expose it to extensions.

[bpasero]

@isidorn you will most likely get bugs for the missing "permanent delete". can we try to revive that one via the alt condition of a IMenuItem? Not sure though if that one is supported for context menus... but it would be the correct and clean solution anyways.

[jrieken]

...try to revive that one via the alt condition of a IMenuItem? Not sure though if that one is supported for context menus...

We never had that because of the stop-the-world-approach of the native menus... The challenge is that they should toggle while the menu is open (see preview/preview to side in markdown). We could have a compromise and read the alt-state before building the menu. The value of the alt-key is send as event but could also be stored there

[isidorn]

@bpasero yes, if the alt condition was supported I could easily plug this in. I can push that right away.
@jrieken Reading the alt before the menu is clicked sounds fair imho (even though we are not behaving like that currently I believe that users tend to hold down the alt before even doing the context menu click)

[bpasero]

Yeah, I was not expecting the context menu label to change once Alt is pressed (which is imho what macOS does in that case), but if we can restore the previous behaviour for permanent delete, that would be great (either Alt pressed before the menu opens or while clicked).

[isidorn]

@bpasero yes all is there except the label change which we can aslo do (Move to Trash and Delete), we just need to plug in the alt support in the menu service.
@jrieken do you want to give me some pointers on how to plug the alt key pressed in or did you plan to tackle it?

[jrieken]

@jrieken do you want to give me some pointers on how to plug the alt key pressed in or did you plan to tackle it?

Yeah, depends on how to integrate this... When calling Menu#getActions you already get an array of MenuItemAction which has MenuItemAction#alt being the alternative action. When populating the UX the alt-key-state must be read. The MenuItemActionItem does that for instance.

There is fillInActions but that only exists as a bridge between the commands/menus-world and the old world. Ideally, it will disappear after the adoption because the context menu service can handle grouped menu items itself. So,fillInActions might be a place for this but not for long.

[isidorn]

@jrieken I have modified the fillInActions such that it now respects the altKey. You can see it in the commit attached and let me know what you think.
@bpasero everything works fine now, just the user has to press alt before starting the menu. I have decided to use the labels: Delete and Delete Permanently as I did not want to break muscle memory of users and change Delete to Mode to Trash.

[bpasero]

@isidorn I am seeing a bug though:

• right click with Alt key pressed
• right click somewhere else on another item

=> the "Delete Permanently" sticks and never changes back

[isidorn]

@bpasero I also see this corner case. The issue is that the context menu stops the world, so I do not get any events at all to update my is_alt_key_pressed_state
Is there some electron event fired when a context menu is opened?

[bpasero]

@isidorn hm, we use a timeout of 0 though before opening the next menu so I would assume that gives time enough to detect that the modifier key is no longer pressed?

If we need the event at all cost, you could add it to the showContextMenu method.

[isidorn]

@bpasero I introduced the event in the contextMenuService this unfortunetely led that the contextMenuService spreads everywhere which you san see in the attached commit. But now the issue is fixed.

[bpasero]

@isidorn I hope we can take this out when we adopted async context menu.

Out of curiosity, does it make any difference if we delay the menu for a longer time, e.g. 20ms for you to find out that the key is no longer pressed?

[isidorn]

@bpasero I would say probably not since the user can hold down the key for as long as he wants. And the most natural scenario is that he releases the key once he sees the context menu (and then the world is blocked)

[jrieken]

You can see it in the commit attached and let me know what you think.

I think its good but some doc and validation updates are now required as well. See https://code.visualstudio.com/docs/extensionAPI/extension-points#_contributesmenus and https://github.com/Microsoft/vscode/blob/master/src/vs/platform/actions/electron-browser/menusExtensionPoint.ts#L359

[isidorn]

@jrieken thanks. I have simply removed the check via 627d4fd
And I have updated the docs via microsoft/vscode-docs@5eae1eb

Also @bpasero and me plan to update the docs more thourougly with new group names once we have them settled in endgame week. You might have feedback on group naming, so here they are:

• navigation
• 2_save
• 2_workspace
• 3_compare
• 4_close
• 4_search
• 5_cutcopypaste
• 7_modification

We need the numbers for proper group sorting. Some numbers are duplicated since those groups can not appear together on an entry.

Code pointer