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

What things should and shouldn't be actions? #1397

Open
benjamin-kirkbride opened this issue Aug 1, 2023 · 8 comments
Open

What things should and shouldn't be actions? #1397

benjamin-kirkbride opened this issue Aug 1, 2023 · 8 comments
Labels

Comments

@benjamin-kirkbride
Copy link
Contributor

benjamin-kirkbride commented Aug 1, 2023

Related to #1305 and #1342

I think <Tab> should be an Action consumer, so when it is pressed it checks the registered actions (such as an indenting action, based on the availability callbacks, etc)

Other things that I think should be action consumers, with similar logic:

  • saving a file (trigger linting, autoformatting, etc)
  • pressing enter (can make a new line, execute a command, select an autocomplete etc)
  • arrow keys (navigate menus, move cursor, etc)

Thoughts?

@benjamin-kirkbride benjamin-kirkbride changed the title RFC: <Tab> Being a Action Consumer RFC: New and Exciting Action Consumers Aug 1, 2023
@benjamin-kirkbride
Copy link
Contributor Author

After more research, I think that util.bind_tab_key should be moved to it's own module, and it should be converted into an action consumer (or it should be deprecated and a new tab_key module should be created, depending on what is more feasible to do in one shot)

@rdbende
Copy link
Collaborator

rdbende commented Aug 2, 2023

  • pressing enter (can make a new line, execute a command, select an autocomplete etc)
  • arrow keys (navigate menus, move cursor, etc)

I don't think these should be converted to action consumers.

  1. They are context-dependent, so it would require too much logic to handle.
  2. Most widgets that use them, handle the event by themselves, so removing them and then reimplementing their functionality doesn't seem like a good idea.

@Akuli
Copy link
Owner

Akuli commented Aug 2, 2023

  1. Tkinter's .bind() already does everything we want it to do. You can add multiple callbacks to the same thing, you have sufficient control on the order in which they are executed, and a callback can prevent other callbacks from running.

@benjamin-kirkbride
Copy link
Contributor Author

benjamin-kirkbride commented Aug 2, 2023

So does that mean yes to tab and save being an action consumer, but no to enter and arrow keys?

I will have to get more familiar with .bind() to see if I understand. I didn't think it did everything we expect out of an action consumer, but I may be ignorant.

@Akuli
Copy link
Owner

Akuli commented Aug 3, 2023

As I see it, binds are great for handling key presses in various widgets, and actions are for stuff that was previously handled by the menubar.

Some examples:

  • When you press an arrow key, the cursor moves. Tk's default bindings do this. We don't touch it. It just works.
  • When you press the tab key, a bunch of custom logic runs. Maybe autocompletions pop up, maybe you get more indentation. For implementing this, the first instinct of any tkinter programmer would be to use binds. We don't want to reinvent the wheel and make that the "wrong" way to do it.
  • When you save a file, there are multiple ways to do it: Ctrl+S, menu bar, command palette, etc. This is an action.

Maybe you can ask whether it makes sense to do X with the command palette or the menubar. These are action consumers that apply to all or almost all actions. For example:

  • I have never seen an application with a "Move cursor right" item in a menubar. It also doesn't make sense to move the cursor with the command palette (why not just press arrow key?).
  • I think some ancient IDEs let you trigger autocompletions through the menubar, but nobody ever does it. Similarly, autocompleting with the command palette doesn't make sense.
  • Most programs for editing files let you save through a "File" menu. It also somewhat makes sense to save a file with the command palette, though it's not super useful for users who know about Ctrl+S.

I will have to get more familiar with .bind() to see if I understand.

This should really be documented somewhere, but add=True and return "break" are relevant to this discussion. For example, consider:

import tkinter

def foo(event):
    print("foo")

def bar(event):
    print("bar")
    return "break"

def baz(event):
    print("baz")

root = tkinter.Tk()
root.bind("<Button-1>", foo, add=True)
root.bind("<Button-1>", bar, add=True)
root.bind("<Button-1>", baz, add=True)
root.mainloop()

This will print foo and bar when you click the window. The bar function can return "break" in some cases and None in other cases, so you can dynamically decide whether to run other stuff. This is perfect for stuff like pressing enter: it can make a new line or select an autocompletion, depending on whether return "break" is used.

saving a file (trigger linting, autoformatting, etc)

I'm pretty happy with <<BeforeSave>> and <<AfterSave>>. If I understand correctly, it seems like you want to run callbacks after an action (Save) ran? If saving is not the only example where this would be useful, we can consider it.

@benjamin-kirkbride
Copy link
Contributor Author

benjamin-kirkbride commented Aug 3, 2023

I think I see the disconnect in how we are thinking about this.

To me, "actions" do two things:

  1. enable the sharing of a common functionality between multiple interfaces
    • example: black auto-format action available from a keyboard shortcut, the menu bar, a toolbar, on save, etc
    • example that motivated this issue: markdown list indentation manipulation action is available from the toolbar, command palette, and also occurs when pressing <tab>
  2. providing a common pattern/api for determining when certain functionality is available
    • example: black auto-format action is only available when:
      • python file
      • black is installed
      • black is the selected autoformatter in the config (vs YAPF or something)
    • example that motivated this issue: markdown list indentation manipulation action is available when:
      • markdown file
      • cursor is on a line that is a valid markdown list

Basically, I want to convert the markdown stuff I've been working on into actions, but to do that tab needs to be an action consumer. It doesn't only have to be an action consumer; I certainly agree we shouldn't go and re-work the way that all the bindings work for say, the text widget.

Does this make sense at all? Am I missing something?

@Akuli
Copy link
Owner

Akuli commented Aug 5, 2023

I like your markdown list indentation example. It feels like an action, since you might want to trigger it from a toolbar. But there should also be a <Tab> key binding on each text widget.

It is possible to specify a global key binding for an action (currently goes through menubar and keybindings.tcl but rewriting is planned). This won't work well for <Tab> because you don't want to mess with bind_tab_key()s of other plugins. You really only want to make indenting work in a more natural way in markdown files, you don't want to associate all <Tab> presses with your action.

I think you should simply add an action and a bind.

This is basically what the comment_selected_lines plugin does now. It adds a thing to menubar (ideally this would be an action) and makes the text widget do the right thing when a special key is pressed (currently that key is the filetype's comment character):

def comment_or_uncomment(tab: tabs.FileTab, pressed_key: str | None = None) -> str | None:
    if <some condition>:
        return None
    <it does stuff here>
    return "break"

def on_new_filetab(tab: tabs.FileTab) -> None:
    tab.textwidget.bind("<Key>", (lambda event: comment_or_uncomment(tab, event.char)), add=True)

def setup():
    menubar.add_filetab_command("Edit/Comment//uncomment selected lines", comment_or_uncomment)
    get_tab_manager().add_filetab_callback(on_new_filetab)
    ...

I'd imagine you typically want the bind to check that the action is currently available and then run it (though there's no particular reason why it would need to do exactly that). To make it easier, we could add a run() or invoke() method to actions that you could bind to. Basically:

class FileTabAction:
    ...
    def invoke(self, tab):
        if self.availability_callback(tab):
            self.callback(tab)

@benjamin-kirkbride
Copy link
Contributor Author

I think we are mostly on the same page now, thanks for working through this with me :)

It feels like an action, since you might want to trigger it from a toolbar.

IMO not just because you "might want to trigger it from a toolbar", but also because it is conditionally available (with the same sorts of conditions that other actions use). Even if we weren't wanting to use it in a toolbar I would want it to be an action to reduce the number of places the same sort of conditional availability logic needs to be managed.

It is possible to specify a global key binding for an action (currently goes through menubar and keybindings.tcl but rewriting is planned).

I assume you are talking about #1351 / #1346 ?

you don't want to associate all presses with your action.

I'm not 100% sure what you mean by this. In my mind, every time <Tab> gets pressed, the availability of any actions that are assigned (bound) to it will get checked.

Thinking about it though, you would need a way to return "break" from the action (not currently done). More thought is needed on that.

I think you should simply add an action and a bind.

This makes sense to me. A better way to phrase this whole proposal would have been "I want to have an established pattern for making Actions work with binds".

This is basically what the comment_selected_lines plugin does now

Yeah that is very similar to how an action looks/works

To make it easier, we could add a run() or invoke() method to actions that you could bind to

Or perhaps __call__?


I'm going to do some playing around with this, and maybe start with the "comment_or_uncomment" plugin instead of Markdown as <tab> has other complications that maybe should be left to be worked out once we have a general pattern for this.

@Akuli Akuli added the docs label Aug 8, 2023
@Akuli Akuli changed the title RFC: New and Exciting Action Consumers What things should and shouldn't be actions? Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants