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

Avoid redundant declarations of bindings #42

Open
stevenguh opened this issue Jun 3, 2021 · 8 comments
Open

Avoid redundant declarations of bindings #42

stevenguh opened this issue Jun 3, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@stevenguh
Copy link
Member

stevenguh commented Jun 3, 2021

Problem

Currently, we don't have a way to reuse bindings across different bindings. Hence, we have the following issues related to not having a way to share declaration across different language id like c and c++ or js/typescript.

Solution

Supply location of the configuration to value of "bindings". For example:

{
    "key": "b",
    "name": "+Buffers/Editors",
    "type": "bindings",
    "bindings": "whichkey.bindingsRef.buffers"
}

This will refer to the bindings defined at whichkey.bindingsRef.buffers in the configuration for example

Alternatives

We can change the conditional key to support logical or to solve issue of sharing bindings between different language mode like c and c++; however, that is limited in what it can solve, and could make the declarations the key for the condition.

Additional context

  • The implementation of layer mode (Implement Layer VSpaceCode#199) might affect the solutions to this issue
  • The purposed solution can share an identical copies of bindings; however, that would not able to support if a user want to mutate some part of the same config. For example: imaging this as an inheritance relationship, where the c++ bindings are inherited from the c bindings with some extra overrides.
@stevenguh
Copy link
Member Author

On the last point of inheritance, we might able to do something like

[
    {
        "key": "languageId:c",
        "name": "+Buffers/Editors",
        "type": "bindings",
        "bindings": "vspacecode.bindingRef.c"
    },
    {
        "key": "languageId:cpp",
        "name": "+Buffers/Editors",
        "type": "bindings",
        "bindings": {
            "ref": "vspacecode.bindingRef.c",
            "overrides": "vspacecode.overridesRef.cpp"
        }
    }
]

@MarcoIeni
Copy link
Member

Instead of inheritance, we could use composition, i.e. we have some group of bindings called "shared c/c++" and both c and c++ include those bindings.
This could be useful because we could share all the common bindings across all major modes

@stevenguh
Copy link
Member Author

I think I understand mostly what you are saying, but to drive it home. Can you provide an example of the "composition"?

@MarcoIeni
Copy link
Member

MarcoIeni commented Jun 17, 2021

Something like this:

{
	"vspacecode.bindings": {
		"type": "array",
		"markdownDescription": "The bindings of the which key menu",
		"lsp": [
			{
				"key": "=",
				"name": "+Format",
				"type": "bindings",
				"bindings": [
					{
						"key": "=",
						"name": "Format region or buffer",
						"type": "command",
						"command": "editor.action.format"
					}
				]
			}
		],
		"default": [
			{
				"key": "languageId:rust",
				"name": "Rust",
				"type": "bindings",
				"common_bindings": ["lsp"],
				"bindings": [
					{
						"key": "T",
						"name": "Toggle inlay hints",
						"type": "command",
						"command": "rust-analyzer.toggleInlayHints"
					}
				]
			},
			{
				"key": "languageId:c",
				"name": "C",
				"type": "bindings",
				"common_bindings": ["lsp"],
				"bindings": [
					{
						"key": "X",
						"name": "Do amazing c Stuff",
						"type": "command",
						"command": "c-analyzer.command"
					}
				]
			}
		]
	}
}

This is just a sketch to give you the idea, do not take into consideration field names and syntax :)
In this way both rust and c have the common "lsp" bindings. And we could stop copy pasting those everywhere.
At the same time you could have "common-js" for js/ts or "common-c" for c/cpp similarly to how I defined "lsp".
So for example c would have "common_bindings": ["lsp", "common-c"]

@stevenguh
Copy link
Member Author

stevenguh commented Jul 18, 2021

Thank you for the example. It still feels a little bit like an inheritance and can have conflicting bindings where two common binding ref might have conflicting bindings.

To keep it simple, we can offer a simple binding reference like

[
    {
        "key": "languageId:c",
        "name": "+Buffers/Editors",
        "type": "bindings",
        "bindings": "vspacecode.bindingRef.c"
    },
]

so we don't have duplicate bindings for c/c++ and js/ts.

In addition, we can just use strict composition, for some case it may still be tedious to declare not exact duplicate bindings but at least it is clear.

[
    {
        "key": "languageId:c",
        "name": "+Buffers/Editors",
        "type": "bindings",
        "bindings": [
            {
                "key": "g",
		        "name": "+Go to",
		        "type": "bindings",
		        "bindings": "vspacecode.lsp.goto"
            }
            // ...
        ]
    },
]

Also, I am thinking the thinking in implementation of layers might change how this should be implemented. See VSpaceCode/VSpaceCode#199

@MarcoIeni
Copy link
Member

sorry but it's not very clear to me. Maybe it would be helpful if you reproduce my example with your syntax, if you don't mind.

@stevenguh
Copy link
Member Author

{
	"vspacecode.bindings": {
		"type": "array",
		"markdownDescription": "The bindings of the which key menu",
		"lsp": [
			{
				"key": "=",
				"name": "+Format",
				"type": "bindings",
				"bindings": [
					{
						"key": "=",
						"name": "Format region or buffer",
						"type": "command",
						"command": "editor.action.format"
					}
				]
			}
		],
		"lsp2": [
			{
				"key": "=",
				"name": "+Format",
				"type": "command",
				"bindings": [
					{
						"key": "=",
						"name": "do something else",
						"type": "command",
						"command": "do.something.else"
					},
					{
						"key": "x",
						"name": "do something else2",
						"type": "command",
						"command": "do.something.else2"
					}
				]
			}
		],
		"default": [
			{
				"key": "languageId:rust",
				"name": "Rust",
				"type": "bindings",
				"common_bindings": ["lsp", "lsp2"],
				"bindings": [
					{
						"key": "T",
						"name": "Toggle inlay hints",
						"type": "command",
						"command": "rust-analyzer.toggleInlayHints"
					}
				]
			}
		]
	}
}

Thank you for bearing with me. For example, default will have two common bindings lsp and lsp2; however, lsp and lsp2 both defined = key. We will have conflict in this case, we will have figure out how to resolve them (e.g. subsequent common_bindings override or merging automatically). It reminds me of multiple inheritance problems, which can introduce a lot of complicity.

I was suggesting to start something simple which allows direct reference only like

{
	"vspacecode.bindings": {
		"type": "array",
		"markdownDescription": "The bindings of the which key menu",
		"lsp2": [
				{
					"key": "=",
					"name": "do something else",
					"type": "command",
					"command": "do.something.else"
				},
				{
					"key": "x",
					"name": "do something else2",
					"type": "command",
					"command": "do.something.else2"
				}
		],
		"default": [
			{
				"key": "languageId:rust",
				"name": "Rust",
				"type": "bindings",
				"bindings": [
					{
						"key": "=",
						"name": "+Format",
						"type": "bindings",
						"bindings": "vspacecode.bindings.lsp2"
					}
					{
						"key": "T",
						"name": "Toggle inlay hints",
						"type": "command",
						"command": "rust-analyzer.toggleInlayHints"
					}
				]
			}
		]
	}
}

However, it is quite inflexible as it requires a set of bindings to defined elsewhere. In addition, the implementation of layer will influence how we should declare bindings greatly.

@MarcoIeni
Copy link
Member

Oh, I see now, thanks a lot for bearing with me too!
Yes it looks good to me :D

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