Actions accessible from multiple places #1335
Comments
|
Right off the bat, I would like to propose that the nomenclature for this is "action" (or something else) and not "command". The reason for this is because "command" will refer to specifically text that is used to initiate #1319.
Should we consider keybindings, or perhaps virtual events (or both?) as the 6th? Better way to put it, should the same system handle "registering" keybindings as well?
A problem I potentially see (but maybe for some reason this is wrong?) is that we will end up in a situation where a ton of plugins have Could it make sense to have them be part of the core, but still able to be disabled? Or is this not a problem in the way I'm thinking ? To be clear I'm not saying "They shouldn't be able to be disabled.", I am saying "Are there other costs/tradeoffs associated with them being plugins that we could avoid, while still meeting the goal of having them be optional?"
Completely agree, as I said in #1334 I was hoping to use the toolbar demo to spur the conversation about how to handle this holistically. As far as the Lets use
Option 1
It also has ways to gracefully handle those plugins being disabled, but it does not handle those plugins being missing, and it also doesn't have a way for a new plugin to be added, say if someone wanted to make a "cli argument" plugin to trigger actions, or an "http rpc api" plugin for registering actions..... Option 2A more dynamic system that I haven't had the time to flesh out mentally that allows plugins to register themselves as "action consumers" in a plugable way. Maybe using virtual events? Don't really know the system well enough to say Okay I ran out of steam on thinking this through. Curious to hear others thoughts at this point. |
Let's go with action :)
Yes. Currently key bindings trigger menu items through virtual events named
This won't be necessary, because we can use the With that, I don't see why specific ways to trigger actions shouldn't be plugins.
Yes, so that part of my plan is already half-done :)
This is an important question. I think we can break this question down to two subquestions:
For 1, I would say we make it the responsibility of the consumer. For example, a consumer can:
Menubar can maybe use a YAML file that lists all actions shown in it and the submenu structure, but then third-party plugins can't add stuff to the menubar which seems off. Or For 2, I think |
To be clear, what you are saying is for instance is that the plugin that implements I actually don't love this, for two reasons. Reason one is it makes adding new plugins that register new actions only half the story, because now you also have to potentially modify the consumers. I like the way that adding things to the menu works now as far as each plugin explicitly setting it up, though I do think the implementation could be more general (AKA what we are trying to work out). What if each plugin also had a
I think this makes the most sense
Here are the inputs I can think of for actions that we might consider supporting. Many of these might seem "redundant" because you technically can get them via the
Maybe the last two are unnecessary though. Something I don't see addressed here is conditional availability of actions. If I'm working on a Markdown file, I don't think the Command Palette should show the "Black - Format Entire File" command. There will be many many examples of this, and I don't think that After writing the above, I'm honestly wondering other than some utility functions what does Lets get a specific example. A module called class BlackFile(Action):
tooltip = "Format entire file with Black"
active_if_filetype = ["Python"]
def __call__(self, file: Path) -> None:
...
class BlackSelection(Action):
tooltip = "Format selection with Black"
active_if_filetype = ["Python"]
active_if_selection = True
def __call__(tab: FileTab) -> None:
...
def setup() -> None:
menubar.add_filetab_command("Tools/Python/Black", black_file)
buttons = []
buttons.append(
toolbar.Button(text="Black" command=black_file)
)
# toolbar automatically adds `FileTab` to callables
toolbar.add_button_group(filetype_name="Python", name="Python Tools", buttons=buttons)
# automatically adds `FileTab`
rightclick_menu.add("Format file with Black", BlackFile)
rightclick_menu.add("Format selection with Black", BlackSelection)
# automatically adds `FileTab`
command_pallete.add("Format file with Black", BlackFile)
command_pallete.add("Format selection with Black", BlackSelection)Now that I write all that out, I'm actually thinking it may make more sense to instead register to each consumer within the theoretical class Action:
# short name for the action
name: str
# Tooltip text for the action
tooltip: str | None = None
# List of filetypes for which the action is active, if None then active for all
active_filetypes: list[str] | None = None
# Determines if the action is active when there is a selection
active_if_selection: bool = False
actions: dict[str, Type[Action]] = {}
def __call__(self, *args, **kwargs) -> None:
raise NotImplementedError
def __init_subclass__(cls, **kwargs):
super().__init_subclass__(**kwargs)
cls.actions[cls.name] = clsand then the consumer plugins add methods to Then, if someone wants to make a new consumer, they just need to add some more methods to Am I crazy? |
Okay so this part doesn't make sense after more thought, but I think the rest tracks. I think |
|
To summarize/finalize what I think:
I'm happy to take a stab at a first pass of this, if you think my design is reasonable |
I think the "typically" is important. Your preferred way is sometimes good, and sometimes it isn't. For example, it works very well for the menubar: as you said, you don't want to modify the consumers, i.e. add something to a global "which actions should the menubar show" thing. But sometimes this isn't what we want. Consider key bindings. There are only so many keys on the keyboard, so registering a keybinding with a third-party plugin doesn't make much sense: there's no good way to avoid conflicts. It is much better to have a global place that configures all keybindings (
I suggest:
This is the design that menubar uses now. It works.
I want it to be simpler and have less boilerplate. Something like this maybe? from porcupine.actions import add_action
def black_files_on_disk(path: Path) -> None:
...
def black_opened_file(filetab: FileTab) -> None:
...
def setup() -> None:
add_action(
name="Black",
description="Format Python code with black", # for tooltips
menu="Tools/Python", # for the menubar
filetype_names=["Python", "Python stub file"],
path_callback=black_files_on_disk,
path_callback_takes_folders=True,
filetab_callback=black_opened_file,
)This action would be available:
The action registry could be something like
Something like |
Akuli
changed the title
I think it could make sense to register a default, but have a system to override any keybinding, basically replace This perhaps should be it's own issue - "How to Handle Keybindings in the Action Economy"
I agree with this, more thought/discussion about how to implement the API is needed, but broadly "have some helpers for the common cases, and allow a callback for complicated ones" should cover everything :)
A few thoughts: First of all, I think that you need to define actions individually for the actions that require a Second, I think we want the ability to use a class to define an action, because I think we will want the ability for the actions to have state. Take for instance if we had a This obviously doesn't mean someone would be required to use classes to define actions, I think having helper functions for the majority of actions makes sense, but I think we would want them to be classes under the hood, not just a dict or something.
I think we should avoid this, and instead register things to the menubar separately. That said, I think some categorization/tag metadata being added to the actions could make sense, and it could look exactly like the way the menubar is formatted now. black = create_action(
name="Black",
description="Format Python code with black", # for tooltips
category="Tools/Python", # action consumer agnostic
callback=black_files_on_disk
)
def setup() -> None:
menubar.add(black, path=None) # if path == None: use action.category as path
If it's a list, how do you query it? A reason I think that defining actions as top level, importable objects in the plugins where they are implemented is it lets you import actions from other plugins directly, rather than from the central Bringing it all together: filetype_names=["Python", "Python stub file"]
# creates an object with a type that tells action consumers to inject a `Path` as the first arg to the callback
black_format_path = create_path_action(
name="Black Format Path",
description="Format File or Directory with black", # for tooltips
category="Tools/Python", # action consumer agnostic
supports_directories = True,
filetype_names = filetype_names,
)
# creates an object with a type that tells action consumers to inject a `FileTab` as the first arg to the callback
black_format_tab = create_filetab_action(
name="Black Format Tab",
description="Format current tab with black", # for tooltips
category="Tools/Python", # action consumer agnostic
filetype_names = filetype_names,
)
def setup() -> None:
menubar.add(command=black_format_path, menu_path=None) # if menu_path == None: use action.category as path
menubar.add(command=black_format_tab, menu_path=None) # if menu_path == None: use action.category as path
toolbar_buttons = []
# uses `name` for button text and `description` for tooltip by default if nothing else provided
toolbar_buttons.append(toolbar.Button(command=black_format_path, icon=None)
toolbar_buttons.append(toolbar.Button(command=black_format_tab, icon=None)
toolbar.add_button_group(button_group = toolbar_buttons, priority = 1)
command_palette.add(command=black_format_tab)
# no `black_format_path` because it wouldn't make sense
keybindings.add_default(command=black_format_tab, keybinding="<<CTRL-SHIFT-S>>")# sorry I don't actually know how to do this but you get the point
# no `black_format_path` because it wouldn't make sense
tab_right_click_menu.add ...
directory_tree.right_click_menu.add ...Thoughts? |
This is not a problem we currently have, so we'll solve it later :)
I initially wanted to do it with separate actions, then combine them together. Now I'm not sure. Let's try it first with separate actions since you prefer that :)
Code complexity is not just about LoC. To be honest I like to avoid OOP trickery whenever it isn't helpful.
Please no. There's already many ways to have state, and I don't want more. For example, a
Existing plugins already handle this in various ways. For example, the run plugin has a JSON history file, and the find plugin creates a stateful finder widget for each tab when invoking for the first time for that tab.
I think this is a detail that is easy to change later without redesigning everything. Maybe for now we could name the thing
Why would one plugin want to import the action of another plugin? It makes sense to import some stuff though, e.g. the directory tree plugin exposes a
IMO the black plugin shouldn't know about the command palette. The command palette should be able to invoke anything, so adding everything to the command palette explicitly would be just unnecessary copy/paste. |
|
After thinking about the menubar a bit more, I think your way is good:
Some details that I don't really care about at this point, easy to decide/change later:
To make action objects accessible for the menubar, and for stuff in general, the action-adding functions in |
Sooner than later I think ;)
Fair enough, I may be ignorant of the options available here so I'll defer to you on this.
I'm not exactly sure what you mean by this. You are saying still have a
Creating new a new, custom "action consumer" plugin would be the thing I conceive of.
Are you saying specifically the command palette shouldn't have to explicitly have commands registered, or are you saying that all "action consumers" should be able to automatically infer what actions are appropriate to include? I think we are close to having a design we are both happy with :) |
|
I agree that we're close. I like the discussion :)
I added a new comment that is all about the menubar.
This should depend on the consumer. You add explicitly to the menubar, but get everything implicitly into the command palette. It will be hard to add a consumer with explicit adding, since you need to update plugins to tell about their actions to that consumer. I think this is a good tradeoff for making the menubar work without adding all actions there. |
|
To clarify that last point, I think a consumer shouldn't hard-code anything about specific actions. A toolbar isn't very useful if you don't let the user add a custom "Run" or "Paste" button to it, or any other action for that matter. So it doesn't make sense to import a specific action in a consumer, because that's not configurable. There are better options:
|
I agree in general, but I want it to be possible to do this specifically so that 3rd party plugins are capable of implementing consumers.
In my mind, this is the perfect example of when someone should be directed to make a custom plugin for themselves. In the vein of #1325, IMO (feel free to disagree! it's not my project ofc) the audience of Porcupine is the kind of person who would be happy to make a 10 line plugin to add an "undo" or a "run" or a "paste remove indentation" button to their toolbar if that's something they wanted, as opposed to us having to work out a config file format to cover every conceivable way someone might want to customize their toolbar (same reason we don't want a config for the menubar IMO). Using a config file for "defaults" IMO is not a great design especially in the context of supporting 3rd party plugins.
I think it should be like this:
But I'm open to debate :) |
To be clear, having a registry of actions satisfies this need, to my understanding, so I think we are covered here. |
|
Indeed, you can search the registry for whatever you want. This would also make it easier to cover the
There are two big reasons why Porcupine uses defaults config files:
And yes, the downside is that the user has to add third-party things to their config file explicitly. That's why the menubar doesn't use a config file. I agree that if it is more difficult to learn how a config file works than how to make a custom plugin, then the config file is dumb. But it doesn't have to be that way. Default config files help as examples to copy/paste from, but also, I make sure to choose the best possible file formats. That's why Porcupine uses so many different formats:
So if I understand this correctly, the user would be encouraged to make a I'm not sure whether the toolbar should use a config file. The disadvantage is that the syntax can become too complicated if you want to e.g. add all actions from a specific plugin or hide them when the filetype doesn't match. I think we should decide toolbar details later, in a separate issue. |
I was actually hoping the right-click menus would become implicit. They currently use explicit adding, and they are quite empty. I think it would be nice to automagically fill them with useful actions. But again, we can work on this later. The |
You can achieve this with a example user config with commented out examples, which is a much more common pattern.
Not sure I understand this point. If we only had one config file, for overrides, we would just write some tests for it and wouldn't need custom logic for this?
Sure. I think the remaining points of contention perhaps should all be relegated to other issues; to my understanding we seem to have agreed on how to implement actions, and the remaining contention is around how the consumers should be integrated into that system. Perhaps it's time for a prototype of |
|
Let's go for it :) IMO the initial PR should add:
Ideas for what the first consumer could be:
Probably not toolbar, because we have many undecided toolbar details. |
This is a common pattern, but causes problems when upgrading. If you add a new config option, your existing users likely won't use it, because their file full of comments does not have a comment for the new option. I have had this problem with ssh configs on debian-based systems that I maintain remotely, for example. But it would be much more painful with Porcupine, because I tend to change the configs quite aggressively when I change them: lots of added and deleted and renamed options. That's why Porcupine's user config files only contain a comment that links to the default config on the This is the main reason. The other reason is not really as important, but I'll try to explain it too.
The tl;dr is that "custom logic" is usually super simple whereas "just write some tests" is hard/complicated. Let's take def setup() -> None:
default_path = Path(__file__).absolute().parent.parent / "default_keybindings.tcl"
user_path = dirs.user_config_path / "keybindings.tcl"
menubar.add_config_file_button(user_path)
try:
with user_path.open("x") as file:
file.write(
"""\
# This Tcl file is executed when Porcupine starts. It's meant to be used for
# custom key bindings. See Porcupine's default key binding file for examples:
#
# https://github.com/Akuli/porcupine/blob/main/porcupine/default_keybindings.tcl
"""
)
except FileExistsError:
pass
try:
get_main_window().tk.call("source", default_path)
get_main_window().tk.call("source", user_path)
except tkinter.TclError:
# more verbose error message than default, including file names and line numbers
raise tkinter.TclError(get_main_window().getvar("errorInfo")) from NoneThings to note:
So, there's no need to unit-test I'm not saying that unit tests are useless or fully unnecessary though. There are tests for some tricky cases that I don't run into often, at least |
I think menubar, because it 1) exists today and 2) we are in complete alignment (I believe) about how it should be implemented
I'm going to create a high level issue to track the progress of this whole "epic", and sub-issues to it for the specific tasks. I think we should close this issue as it has served it's purpose to kick this off IMO |
From #1334:
This actually existed before, but I got rid of it because the commands ended up only being added into the menubar (it was a plugin), and so the registering thingy only acted as an unnecessary layer that didn't provide anything useful. But back then there was only one place to put commands into, the menubar. Now we have 4, soon 5 or 6:
This IMO should be part of the core Porcupine, not a plugin, since it doesn't really make sense to run Porcupine without being able to at least register some commands, but I would still like to keep the right-click menus and toolbar as plugins so that they can be disabled individually. For example, maybe you don't use the toolbar and you want to disable it, or maybe you keep right-clicking something accidentally and you want to disable the right-click menus.
With that, I suggest:
porcupine/commands.py, where you can register commands and it stores them globally. This module wouldn't actually show the commands anywhere, but it should have some way to query the existing commands for right-click menus and toolbars/menubars. (As I said, this used to exist before, it was calledporcupine/actions.py.)right_click_menus.py, that uses the new command system.I think it is really important to do step 1 first to avoid rewriting code that was just written, but the order of other steps doesn't really matter.
Thoughts?
The text was updated successfully, but these errors were encountered: