-
Notifications
You must be signed in to change notification settings - Fork 70
[WIP] Implement Custom Command #79
base: master
Are you sure you want to change the base?
Conversation
It's mostly UI and some wiring which is left.
This prevents the user from seeing our internals, allowing us to provide a more stable API.
This is definitely a great start for evaluating custom code. Sorry for the delayed response. Creating a GUI for this and deciding how commands should be stored will be challenging. My current idea is to add a new Options Category to the Options Page labeled "Custom Commands." This section would contain a single CustomCommand widget. The CustomCommand Widget will provide the GUI that lets users add, edit, and remove custom commands. For each command, users will have to provide:
The CustomCommand Widget will be similar to the Keybindings widget in that the user can add or delete entries by pressing a single button. Error checking will be done on the key field to ensure user-defined command keys don't conflict. The value of the CustomCommand Widget will be stored just like any other widget type. Look at this example to see how I imagine custom commands will be stored. When the user modifies a custom command entry, the underlying options in storage will change. This will trigger a rerender with a new set of options (e.g. new keybinding widgets representing new commands). Currently, pages are messages their options on load as well as when the user changes an option. See https://github.com/lusakasa/saka-key/blob/master/src/client/index.js#L44, https://github.com/lusakasa/saka-key/blob/master/src/client/modes.js#L6,7 and https://github.com/lusakasa/saka-key/blob/master/src/background_page/modes.js#L87. I'll just add a new field "customCommands" that will contain the function text to the options object sent to every client. It will be important to adjust the extension update logic so that custom commands don't break Saka Key. |
Thanks for the pointers, they sped things up a lot. Right now I am making progress on the options but it seems to assume that the number of options are a compile-time construct. I am working around this by making the option type an array of custom commands but this feels like going against the grain. Would you recomend a different approach? |
It's not wired up at all but it's a start. It is missing keybindings which look like they will take some work.
return ( | ||
<li> | ||
<p>This is a command</p> | ||
<TextArea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, Widgets are meant to be standalone containers representing a single option. Although code reuse is nice, I think it's important to be able to change the TextArea Widget independently of the CustomCommand Widget.
Good start. These are the exact steps I would take. Please refer to the storage example for my vision on the underlying storage.
[
{
"key": "switchToHTTPS",
"label": "Switch To HTTPS",
"function": "()=>{document.location=document.location.href.replace(/^http:/,'https:')}"
},
{
"key": "toggleYoutubePlay",
"label": "Pause/Play Youtube",
"function": "()=>{document.querySelector('.ytp-play-button').click()}"
}
]
"customOptions_General": ["minFontSize"],
"customOptions_Keybindings": ["switchToHTTPS", "toggleYoutubePlay"],
"customOptions_Hints": [],
"customOptions_Custom Commands": [],
"customOptions": {
"minFontSize": {
"type": "number",
"label": "Minimum Font Size",
"key": "minFontSize",
"default": 12
}
"switchToHTTPS": {
"type": "keybinding",
"label": "Switch To HTTPS",
"key": "switchToHTTPS",
"default": []
},
"toggleYoutubePlay": {
"type": "keybinding",
"label": "Pause/Play Youtube",
"key": "toggleYoutubePlay",
"default": []
}
}
In summary, it's a lot of work. You can probably come up with something simpler, but it probably won't be as user-friendly or configurable. And this infrastructure is necessary for the plugin system I've been designing. |
This is now feature complete, however there is a lot of cleanup to be done.
Ok, I now have a feature-complete implementation. There is clearly more work to be done though. I'l going to update the first comment with a checklist of work to be done. However I will likely want feedback on a lot of the points. 1-4 had already been done. I have updated the trie structure to include the custom commands as well as pre-defined commands.
How do I tell if a client isn't privledged? I figured that all clients could use eval since they don't have a CSP applied? If not I suppose the most reliable way to do this is catch an exception from the user code, or eval call and report this nicely. |
Also I took out the |
I gave your branch a shot and it looks promising. Here's some quick feedback. I'll replay again later with something more detailed.
|
Hi, thanks for all the feedback. But it is unlikely that I will continue work on this. Anyone can feel free to take over or you can close this PR and forget about it. |
Thanks for the effort @kevincox. I'll keep this open since I (or someone else) might use it as a base for an implementation in the future. |
This is a very basic implementation but it is actually pretty close. (It turns out calling eval on a string isn't too difficult).
What rocks:
What sucks:
Command has access to a lot more then indended. Right now I want to expose "event" and the standard web globals to the code but as it stands I can't think of a way to whitelist things it has access to. I could try putting it in it's own module but IIUC the bundler basically prevents that from acutally restricting access at all. I might be able to use "let global_eval = eval; global_eval(code)" but I will have to take another look at the bundler to see what globals it leaks.What needs to be done:
Fixes #45