Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support when-like condition #11

Closed
stevenguh opened this issue Aug 3, 2020 · 14 comments
Closed

Support when-like condition #11

stevenguh opened this issue Aug 3, 2020 · 14 comments
Assignees
Labels
enhancement New feature or request

Comments

@stevenguh
Copy link
Member

Split from #5.

My current thinking on this is at #5 (comment)

@stevenguh
Copy link
Member Author

Just jotting here. We will likely need another context variable like whichKeyVisible so we the the shortcut hack won't be executing when if the command is long running like renaming a reference/variable.

@marcoieni
Copy link
Member

could this enable major mode?

@stevenguh
Copy link
Member Author

Can you elaborate a little bit more? I am not familiar with major mode.

@marcoieni
Copy link
Member

In spacemacs you have major-modes, like "python", "markdown", and so on. So for example if you open a .md file spacemacs will use Markdown mode in that buffer.

When you are using a mode, key bindings under <spc> m or under , will be unique to that mode.

This means that when you are editing a markdown, <spc> m b could be assigned to "make bold", while in python <spc> m b could be assigned to "toggle breakpoint" (I am just inventing).

@stevenguh
Copy link
Member Author

Thank you for the clear explanation. I think this work should take this into account and facilitate major mode as well if we are implementing a type called "conditional".

@stevenguh
Copy link
Member Author

Just jotting here again. We probably can use window.activeTextEditor?.document.languageId to detect what language is the active document for the major mode. For the config, I am thinking,

For the conditional command:

{
    "key": "t",
    "name": "Show tree/explorer view",
    "type": "conditional",
    "commands": ["workbench.view.explorer", "workbench.action.toggleSidebarVisibility"],
    "conditional": {"when": [null, "sideBarVisible && explorerViewletVisible"]}
}

For conditional binding like major mode:

{
    "key": "m",
    "name": "Major mode",
    "type": "conditional",
    "bindings": [[...], [...]],
    "conditional": {"langId": [null, "javascript"]}
}

These are not finalized yet, just a thought

@stevenguh
Copy link
Member Author

On second thought that would make the json schema wayyyyy too complicated. The following example could be better in terms of flexibility and consistent style format with the current config. The format of the conditions here changed from specifying one type of conditions to a simple array that do simple waterfall style conditions of any types of conditions (either when or langId) to provide flexibility and consistent format style

For the conditional command:

{
    "key": "t",
    "name": "Show tree/explorer view",
    "type": "conditional",
    "conditional": [
        {
            "type": "command",
            "command": "workbench.view.explorer"
        },
        {
            "type": "command",
            "command": "workbench.action.toggleSidebarVisibility"
        }
    ],
    "conditions": [null, {"when": "sideBarVisible && explorerViewletVisible"}]
}

For conditional binding like major mode:

{
    "key": "m",
    "name": "Major mode",
    "type": "conditional",
    "conditional": [
        {
            "type": "bindings",
            "bindings": [...],
        },
        {
            "type": "bindings",
            "bindings": [...],
        }
    ],
    "conditions": [null, {"langId": "javascript"}]
}

@marcoieni
Copy link
Member

Why the null in conditions?

@stevenguh
Copy link
Member Author

stevenguh commented Aug 22, 2020

The null is used like the else in a if statement. Not sure if there's a better way to handle that.

@marcoieni
Copy link
Member

Oh, ok, so the first condition is referred to the first conditional and so on. It is a little bit confusing. Imaging when you have a lot of conditions.. Keeping track of positions might be difficult.

What if you insert the condition inside each conditional? For example

"conditional": [
        {
            "type": "bindings",
            "bindings": [...],
        },
        {
            "type": "bindings",
            "bindings": [...],
            "condition" : {"langId": "javascript"} 
        }
    ]

If no condition is present it is considered default (null)

@stevenguh
Copy link
Member Author

Oh I like this where all the information is in the object itself instead of two places

@stevenguh stevenguh self-assigned this Aug 24, 2020
@stevenguh
Copy link
Member Author

stevenguh commented Aug 26, 2020

I have a working prototype on conditionals branch; however, as I am trying implement overrides (add/delete/modify) for conditional bindings. I realized that the condition in a conditional binding is basically the key in regular binding but instead of string, we defined it as object.

If a user wants to redefined the condition, the user can use the following example to override the whole conditional binding.

{
    "bindingOverrides": [
        {
            "keys": "m",
            "name": "Major",
            "conditional": [
                {
                    "type": "bindings",
                    "bindings": []
                },
                {
                    "type": "bindings",
                    "name": "JavaScript",
                    "bindings": [],
                    "condition": {
                        "langId": "javascript"
                    }
                }
            ]
        }
    ]
}

In order to implement the traversal of the conditional bindings, I have to re-use the keys in the current overrides format in the following example.
The example is an override for conditional binding with a key of m. In this case the last key will be read as condition for the command with the format of a query string. This will add/modify the conditionals depending if "languageId=typescript" exists. This is also powerful that it can traverse through the conditional bindings (examples keys: ["m", "languageId=typescript", "i"]). Note, this is possible because we do not implicitly any bindings if the part of the key sequence doesn't exist in the overrides.

"bindingOverrides": [
    {
        "keys": ["m", "languageId=typescript"],
        "name": "123",
        "type": "command",
        "command": "workbench.action.editor.changeLanguageMode"
    }
]

Although, this works for the most part, There are two things that bug me with the current implementation.

  1. The asymmetry of bindings and overrides (one using object to present condition and other use a query string)
    Working the issue backward, in order to unify them I think we can use query string to represent the condition as key, and then we can reuse the "bindings" key without introducing a new key. An example binding is as follow:
{
    "key": "m",
    "name": "Major mode",
    "type": "conditional",
    "bindings": [
        {
            "key": "",
            "type": "bindings",
            "bindings": [...],
        },
        {
            "key": "langId=javascript"
            "type": "bindings",
            "bindings": [...],
        }
    ],
}
  1. Needs to escape query string for some character
    This is especially prominent if we have a "when" condition that uses & (e.g. sideBarVisible && explorerViewletVisible) and it makes the config not humanly readable. The example override with escape characters.
"bindingOverrides": [
    {
        "keys": [
            "m",
            "when=sideBarVisible%20%26%26%20explorerViewletVisible"
        ],
        "name": "123",
        "type": "command",
        "command": "workbench.action.editor.changeLanguageMode"
    }
]

I am thinking

  • Use a custom query string that uses ; as separator instead of & (example of this is SQL database connection string)
  • Use a custom query string as the key for conditional binding instead of condition like I mentioned above.
  • Instead of adding a new key conditionals, we can then reuse the key bindings
  • This has also the added benefit of reuse the same schema validation

EDIT:
The customer query using ; as separator is probably not enough, and we will also need use : instead = for the association, like when:sideBarVisible && explorerViewletVisible && focusedView == '' && !whichkeyActive;langId:javascript. We can also use , instead of ; to be more json like.

@stevenguh
Copy link
Member Author

Just implemented a custom query string, that cleaned up both the code and the config a bit :)

@stevenguh
Copy link
Member Author

Released v0.8.0 with this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants