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

Allow Passing Window to Snap User Actions and Introduce Configurable Snap Name List #1629

Merged

Conversation

jaresty
Copy link
Contributor

@jaresty jaresty commented Dec 2, 2024

Background:

We want to be able to call these snap positions from another module (#1578), but the existing action takes a spoken form. We'd rather not build a dependency on those, because they can be brittle if the user changes them; it's become a best practice recently (formatters, etc) to use static identifiers instead, and use a Talon list to provide the spoken form.

Change:

  • This change changes the keys of the snap positions dictionary to have static identifiers, instead of the previous spoken forms.
  • A talon list is introduced to map the spoken forms to the new identifiers.
  • To reduce user friction, as people are probably calling the existing action with a spoken form, we'll attempt to convert it to the new identifier if we can, to allow the action to succeed, although we will still show a deprecation warning so they can update their code.

- This extracts the snap positions as a talon list and exposes a public method to use them
- It falls back if users are using the existing names directly
- use programmatic formatting instead of static map
- show deprecation warning
@jaresty jaresty changed the title Extracts snap positions as a talon list Allow Passing Optional Window to Snap User Actions Dec 2, 2024
core/windows_and_tabs/window_snap.py Outdated Show resolved Hide resolved
core/windows_and_tabs/window_snap.py Outdated Show resolved Hide resolved
core/windows_and_tabs/window_snap.py Outdated Show resolved Hide resolved
- Update docstrings.
- Make explicit case for when an invalid position name is passed
jaresty and others added 5 commits December 3, 2024 17:32
- Used deprecate action instead of deprecate command since this is an API change instead of a spoken form change.
- Introduced an optional argument to suggest a replacement action on deprecate action. If the argument is not provided it will not mention the replacement action in the notification.
nriley
nriley previously requested changes Dec 14, 2024
core/windows_and_tabs/window_snap.py Outdated Show resolved Hide resolved
core/windows_and_tabs/window_snap.py Outdated Show resolved Hide resolved
@jaresty jaresty requested a review from nriley December 14, 2024 17:40
@phillco
Copy link
Collaborator

phillco commented Dec 15, 2024

Can you make the title reflect everything the change does?

@jaresty jaresty changed the title Allow Passing Optional Window to Snap User Actions Allow Passing Window to Snap User Actions and Introduce Configurable Snap Name List Dec 15, 2024
@jaresty
Copy link
Contributor Author

jaresty commented Dec 15, 2024

Adjusted the title to mention the new talon list. Let me know if you think I should add more.

@phillco phillco dismissed nriley’s stale review January 11, 2025 15:01

New review needed

@phillco
Copy link
Collaborator

phillco commented Jan 11, 2025

Ah weird @nriley's old requested changes was keeping this one in the "reviewed needed" state even though a new review had been requested.

@AndreasArvidsson AndreasArvidsson merged commit 5c98848 into talonhub:main Jan 11, 2025
2 checks passed
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.

4 participants