8992-enabling-view-when-clause

[danarad05]

Enabling setting view when clauses with these values: undefined, true, false or expressions.

Signed-off-by: Dan Arad [email protected]

What it does

Fixes: #8992

How to test

follow #8992 explanation - set different test views

• without when clause
• with when = "true"
• with when = "false"
• with when = "{expression}"

i.e.:

"views": {
            "explorer": [
                {
                    "id": "testView",
                    "name": "Test View true",
                    "when": "true"
                },
                {
                    "id": "testView2",
                    "name": "Test View2 false",
                    "when": "false"
                },
                {
                    "id": "testView3",
                    "name": "Test View3 undefined"
                },
                {
                    "id": "testView4",
                    "name": "Test View4 expression",
                    "when": "debugConfigurationType == 'pwa-extensionHost'"
                }
            ]
        },

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

[danarad05]

@colin-grant-work Please review latest changes. Thanks

[colin-grant-work]

I believe the current code is working as expected 🎉. There are some hiccoughs, though. For example, it looks like the data coming in for the .when clause isn't being sanitized, so e.g. " true " is not treated as "true". It may be advisable either to sanitize the when when we first see the view in the registerView method, or to add .trim() anywhere we're checking for strict identity.

[alvsan09]

I tried the fix and it works fine,
I have a question though, it seems like changing the views via the package.json don't take effect unless I used a different view id,
I know the question is unrelated to the change, but I am just wondering if you know about the issue or a workaround for it.

[danarad05]

I believe the current code is working as expected 🎉. There are some hiccoughs, though. For example, it looks like the data coming in for the .when clause isn't being sanitized, so e.g. " true " is not treated as "true". It may be advisable either to sanitize the when when we first see the view in the registerView method, or to add .trim() anywhere we're checking for strict identity.

Thanks for catching this. Fixed.

[danarad05]

I tried the fix and it works fine,
I have a question though, it seems like changing the views via the package.json don't take effect unless I used a different view id,
I know the question is unrelated to the change, but I am just wondering if you know about the issue or a workaround for it.

I also had this issue. The cause of it is that layout is stored in localStorage and retrieved onload. So, to workaround it - you need to clear localStorage.

[colin-grant-work]

I tried the fix and it works fine,
I have a question though, it seems like changing the views via the package.json don't take effect unless I used a different view id,
I know the question is unrelated to the change, but I am just wondering if you know about the issue or a workaround for it.

@alvsan09, the easiest way to get around this is to run the Reset Workbench Layout command from the command palette. Then you'll see what would have happened if you hadn't had a layout stored.

[alvsan09]

the easiest way to get around this is to run the Reset Workbench Layout command from the command palette.

Thanks @colin-grant-work, the reset works great !

[danarad05]

@vince-fugnitto who can also review this ?

[vince-fugnitto]

Ideally the context-key-service should be responsible for properly handling all when clauses including determining enablement through true and false strings.

We should definitely think of improving the service (by re-using implementation from vscode) rather than adding such workarounds for all clients (commands, keybindings, menus, views).

[danarad05]

We should definitely think of improving the service (by re-using implementation f

As I mentioned here #9156 (comment), in monaco 0.23.x there will be additional contextkey types that would streamline the solution but till then - why not accept this change?

[vince-fugnitto]

We should definitely think of improving the service (by re-using implementation f

As I mentioned here #9156 (comment), in monaco 0.23.x there will be additional contextkey types that would streamline the solution but till then - why not accept this change?

@danarad05 if we do merge for now, please open follow-up issues to clean up the technical debt.

[colin-grant-work]

Sorry, I thought I'd already approved this! (But I guess the approval got stale?)

[danarad05]

Sorry, I thought I'd already approved this! (But I guess the approval got stale?)

Thank you @colin-grant-work