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

configurable number prefix without mouse grid #1342

Conversation

jaresty
Copy link
Contributor

@jaresty jaresty commented Dec 27, 2023

  • Attempt to implement optional numb prefix
  • The tags need to be logically combined
  • Respond to suggestion that the mouse grid should not be considered

@fidgetingbits
Copy link
Collaborator

Thanks for your patience. I've pushed some changes that I think are more in line with what @auscompgeek originally
suggested in #943, and that will be easier to maintain. I'll detail the changes a bit below, and why I did it this way
instead of how you did it, in case you're interested:

  • The name of the 'numb' prefix is no longer in the tag name. It's possible people would change the prefix to something else, so should never include the actual spoken form in the tag name imo. The name "disable_unprefixed_numbers" also fits nicely in that mouse_grid setting the tag becomes more self-explanatory.
  • Set the tag in mouse_grid, so that it still works
  • Move the unprefixed version of the command in it's own file, rather than the prefixed version. This is because many users add other types of numbers (hexadecimal, etc) to numbers.talon already, which also have prefixes of various forms. So just isolating the non-prefixed version seems cleaner.
  • Add documentation to the settings file so that users now how/why to use the tag.

@fidgetingbits
Copy link
Collaborator

I guess one remaining question is if we should set the tag by default or not.

settings.talon Outdated Show resolved Hide resolved
@jaresty
Copy link
Contributor Author

jaresty commented Jan 15, 2024

my preference would be for the recommended behavior (I.e. not requiring the prefix) be the default.

Copy link
Collaborator

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks ok to me. tempted to have it on by default, but then we'd need to do some kind of deprecation process

also, double negatives always confuse me. I wonder if we should have a allow_unprefixed_numbers tag that is turned on by default in settings, and the user can comment it out if they don't want unprefixed numbers

@fidgetingbits
Copy link
Collaborator

also, double negatives always confuse me. I wonder if we should have a allow_unprefixed_numbers tag that is turned on by default in settings, and the user can comment it out if they don't want unprefixed numbers

I considered that but since mouse grid needs to disable non prefixed numbers to use its own nonprefixed number commands, it seemed simpler to have a disable tag.

Maybe there is a good way for mouse grid to remove the tag when it activates? I don't recall off hand if you can just shut off a a tag like that.

@fidgetingbits
Copy link
Collaborator

fwiw I'm happy to have prefixed the default as well.

random thought, but for deprecating stuff I almost wonder if we should introduce a deprecated notification action that pops a notification only the first time you use a deprecated command after starting talon (or every N times maybe), but that also spams the logs on every use. that way it's easier for less technical users to find out (since I suspect many don't read the logs), but still in a way thats not too spammy. it would be useful here and for future depreciation I suspect.

@pokey
Copy link
Collaborator

pokey commented Jan 20, 2024

for deprecating stuff I almost wonder if we should introduce a deprecated notification action that pops a notification only the first time you use a deprecated command

I believe that kind of thing is already covered by our deprecation infrastructure / policy decoumentation

@@ -177,6 +177,7 @@ def split_list(value, l: list) -> Iterator:
number_small_map = {n: i for i, n in enumerate(number_small_list)}

mod.list("number_small", desc="List of small numbers")
mod.tag("disable_unprefixed_numbers", desc="Require prefix when saying a number")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might suggest just calling this "prefixed_numbers"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment about why it's this way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could call it "numbers must be prefixed" if the double negative is too confusing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll update it to be non-double negative tag then. user.prefix_numbers or user.numbers_must_be_prefixed? I think I prefer the latter because it's a bit more self explanatory, but good with whatever others prefer.

@knausj85
Copy link
Member

I agree the prefix should be the default to improve compatibility with cursorless / reduce misrecognitions

@pokey
Copy link
Collaborator

pokey commented Jan 23, 2024

I agree the prefix should be the default to improve compatibility with cursorless / reduce misrecognitions

makes sense, tho in that case we should prob use command deprecation infra so it doesn't just break when user updates?

@fidgetingbits
Copy link
Collaborator

for deprecating stuff I almost wonder if we should introduce a deprecated notification action that pops a notification only the first time you use a deprecated command

I believe that kind of thing is already covered by our deprecation infrastructure / policy decoumentation

Thanks for the reminder. I just added the relevant updates. Of course wording and such will be subject what tag ends up being settled on.

Copy link
Collaborator

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a couple final minor comments

core/numbers/numbers_unprefixed.talon Outdated Show resolved Hide resolved
settings.talon Outdated Show resolved Hide resolved
fidgetingbits and others added 2 commits February 9, 2024 20:05
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
@jaresty
Copy link
Contributor Author

jaresty commented Feb 21, 2024

Anything left to fix on this? Wanted to make sure you weren't waiting on me 😀

@fidgetingbits
Copy link
Collaborator

I was mostly waiting for someone else to actually approve after I addressed the last of the suggested changes, but I suppose since pokey said it was fine with the minor changes I can approve it myself.

@fidgetingbits fidgetingbits merged commit 54ce69d into talonhub:main Feb 22, 2024
2 checks passed
Mark-Phillipson pushed a commit to Mark-Phillipson/community that referenced this pull request Mar 29, 2024
- Attempt to implement optional numb prefix
- The tags need to be logically combined
- Respond to suggestion that the mouse grid should not be considered

---------

Co-authored-by: fidgetingbits <fidgetingbits@memeoid.cx>
Co-authored-by: FidgetingBits <fidgetingbits@users.noreply.github.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
sisi-sh added a commit to sisi-sh/sisi_talon that referenced this pull request Apr 1, 2024
* 'main' of github.com:talonhub/community: (73 commits)
  add lowercased firefox-esr to firefox.py (talonhub#1410)
  configurable number prefix without mouse grid (talonhub#1342)
  Update app.exe matching to work with both public and beta Talon (talonhub#1385)
  Add comment line to javascript (talonhub#1387)
  Add detail to documentation (talonhub#1380)
  Switch to black pre-commit mirror (talonhub#1378)
  Update app_name_overrides.windows.csv to use Windows Terminal instead of iTerm2 (talonhub#1379)
  [pre-commit.ci] pre-commit autoupdate (talonhub#1376)
  coalesce some list updates (talonhub#1348)
  Remove unused setting from c code language (talonhub#1368)
  mouse: adding synonyms for starting left drag. (talonhub#1356)
  Adds commands to focus on the last application (talonhub#1336)
  Stop using “brackets” to mean “braces” (talonhub#1344)
  Update settings descriptions (talonhub#1371)
  Revert changes to `user.mouse_enable_pop_click` (talonhub#1370)
  Fix typo in `mouse.py` variable name (talonhub#1372)
  Convert boolean settings to boolean (talonhub#1360)
  Add ruby op-or-equals command (talonhub#1347)
  added app name Mate-terminal (fork of Gnome-terminal) (talonhub#1367)
  added app name Caja (fork of Nautilus) (talonhub#1366)
  ...
emragins pushed a commit to emragins/talon-community that referenced this pull request May 28, 2024
- Attempt to implement optional numb prefix
- The tags need to be logically combined
- Respond to suggestion that the mouse grid should not be considered

---------

Co-authored-by: fidgetingbits <fidgetingbits@memeoid.cx>
Co-authored-by: FidgetingBits <fidgetingbits@users.noreply.github.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants