Skip to content

Improve phrase history mechanism#758

Merged
pokey merged 4 commits intomasterfrom
pokey/issue757-Improve-phrase-history-mechanism
Feb 13, 2022
Merged

Improve phrase history mechanism#758
pokey merged 4 commits intomasterfrom
pokey/issue757-Improve-phrase-history-mechanism

Conversation

@pokey
Copy link
Collaborator

@pokey pokey commented Feb 4, 2022

Fixes #757
Fixes #660

Also adds support for user-defined history phrase transformation by overriding user.history_transform_phrase_text action

This PR starts to move us in a direction where users have more flexibility to influence knausj behaviour without needing to have a fork, by adding an action that the user can override

Here's an example of user-defined history processing

@pokey
Copy link
Collaborator Author

pokey commented Feb 4, 2022

@knausj85 @rntz @splondike @nriley @wolfmanstout @AndreasArvidsson WDYT about this approach to moving away from users needing to fork knausj to alter its behaviour?

@rntz
Copy link
Collaborator

rntz commented Feb 4, 2022

I like the general idea of making knausj more configurable without editing it, but what are the use-cases for this history rewriting hook you've added? Unless we know of something someone would like to do with it, I think it's better to keep things simple.

The one use-case I can think of is letting users decide whether to include or exclude phrases based on whether we're in sleep mode or not: but this PR hardcodes that we don't include them rather than letting the action determine that.

@AndreasArvidsson
Copy link
Collaborator

I definitely agree that configurability is a decide feature of knausj. I'm not a big fan of the command history and talon subtitles diverging though.

@rntz
Copy link
Collaborator

rntz commented Feb 4, 2022

Oh, I just noticed that you have an example linked in the PR itself. Hm.

Well, I'd be happy to merge this, but I think it would be good to put the check for sleep mode into the body of user.history_transform_phrase_text instead of hard-coding it.

@wolfmanstout
Copy link
Contributor

On the specific question of this pattern as a way to support customization, I do think it makes sense. It's aligned with the approach in other systems I'm aware of that allow deep customization like WordPress and Emacs. I'm in favor of only breaking out functions when there's a demonstrated need otherwise it will add overhead everywhere. It'll be a judgment call where to draw the lines, of course, but this is probably a better approach for the long term health and adoption of knausj than having everyone maintain forks.

Reflecting a bit on fork maintenance: for the most part I don't mind handling merge conflicts when there are updates as it educates me about new functionality that touches code I cared about enough to customize. The biggest issue I've bumped into is the work of creating clean pull requests, which requires me to work with a clean branch that's difficult for me to operate hands free. I suppose this pattern would let me avoid that, although there is some risk that if I only test my PR with my overrides in place that I may be accidentally depending on that behavior. I guess we could ask people to do a final "vanilla check". Just food for thought.

@splondike
Copy link
Collaborator

Learnt some new python syntax today: if blah := foo(): print(blah).

I like the idea of making knausj more customisable without forking. As others have mentioned it'd have to be case by case though.

This specific change seems fine to me, though moving the sleep filter into the action as rntz said makes sense to me.

@rntz
Copy link
Collaborator

rntz commented Feb 5, 2022

I guess the disadvantage of putting the sleep filter into the action is that then if the user overrides it they need to either do the sleep check themselves or else call actions.next. I think that's probably ok?

@pokey
Copy link
Collaborator Author

pokey commented Feb 7, 2022

The main reason I didn't add sleep check to user-overridable function was that I wanted it to be a pure function, but maybe that's a bit silly. I'm not opposed to putting it there, but yeah as @rntz mentions then user would need to implement it themselves

We could add a setting for the sleep-mode thing?

@AndreasArvidsson see talonvoice/talon#348 (comment)

I like the idea of making knausj more customisable without forking. As others have mentioned it'd have to be case by case though.

Yeah I think it should be driven by usage. At some point I'm planning to do cursorless-dev/cursorless#547 (comment)

@splondike
Copy link
Collaborator

You could pass in whether speech was enabled as well if you wanted to keep it pure. I think purity is mainly useful for managing complexity (particularly when the type system can enforce it, i.e. Haskell). This case is pretty simple though.

I reckon if people are fancy enough to override the action then they would be fine to put in the sleep mode check as well, particularly if you pass in the state. Not fussed either way though.

@pokey pokey force-pushed the pokey/issue757-Improve-phrase-history-mechanism branch from f9aa032 to 1a1e09b Compare February 8, 2022 18:40
@pokey
Copy link
Collaborator Author

pokey commented Feb 8, 2022

Ok sleep filter moved into user action. Look good to everyone now?

@splondike splondike added the enhancement New feature or request label Feb 11, 2022
@pokey pokey merged commit b1fa725 into master Feb 13, 2022
@pokey pokey deleted the pokey/issue757-Improve-phrase-history-mechanism branch February 13, 2022 14:12
purpleP pushed a commit to purpleP/knausj_talon that referenced this pull request Jun 27, 2022
* Improve phrase history mechanism
Fixes talonhub#757

* Don't add phrases to history if speech is not enabled

* Move check for whether speech is enabled into user action

* PR feedback
pokey added a commit to pokey/talon_hud that referenced this pull request Jan 29, 2024
This PR leverages the `user.history_transform_phrase_text` action to allow users to customize the way that phrase history is processed. For example, a user could remove everything after `drowse` so as not to pollute command history. See eg https://github.com/pokey/pokey_talon/blob/main/custom/code/history.py.

This functionality was introduced to community in talonhub/community#758 two years ago, so should be safe to rely upon
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

Successfully merging this pull request may close these issues.

Improve phrase history mechanism Command history records all speech while talon is asleep

5 participants