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

add user.open_file action and list/commands for editing settings csvs #847

Merged
merged 10 commits into from
Jun 18, 2022

Conversation

rntz
Copy link
Collaborator

@rntz rntz commented May 9, 2022

Changes:

  1. Adds a new user.open_file action which opens a file in whatever the appropriate application is.

  2. Adds a list user.talon_settings_csv for paths to talon user settings csv files, e.g. additional_words.csv, search_engines.csv, etc.

  3. Adds a command customize {user.talon_settings_csv) that opens the corresponding csv and jumps to end of file.

This is heavily based on #810.

Feedback requested:

  1. Is customize the command prefix we want here? At present this will bind the ccommands:

    customize search engines
    customize websites
    customize file extensions
    customize system paths
    customize words to replace
    customize additional words
    

    Alternatives might be edit or open (a bit short), add new, edit talon, edit {talon_settings_csv} csv.

  2. Is there a better place to put this command than in a new file? Maybe standard.talon?

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 is really nice! I like how you've factored the original PR into one clean concern. I actually think you could factor it even more, though. I don't feel super strongly, but see comments

code/open_file.py Outdated Show resolved Hide resolved
Comment on lines 12 to 20
mod.list('talon_settings_csv', desc="Absolute paths to talon user settings csv files.")
_csvs = {
x: os.path.join(SETTINGS_DIR, '_'.join(x.split()) + ".csv")
for x in ["file extensions", "search engines", "system paths",
"websites", "words to replace", "additional words"]
}
_csvs.update({
"homophones": os.path.join(REPO_DIR, 'code', 'homophones.csv'),
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think maybe this stuff would go into a separate file, along with the proposed edit_user_file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it might belong in user_settings.py

code/open_file.py Outdated Show resolved Hide resolved
@codecat555
Copy link
Contributor

This looks good, I agree it's an improvement on what I submitted in #810.

code/open_file.py Outdated Show resolved Hide resolved
code/open_file.py Outdated Show resolved Hide resolved
@rntz
Copy link
Collaborator Author

rntz commented Jun 4, 2022

Ok, we rewrote this during the maintenance session to make the following changes:

  1. We now use contexts instead of examining app.platform.
  2. The action is named edit_text_file, since that's our current use-case for it. We can add an open_file command later if we need it.
  3. The action doesn't do any path handling and doesn't wait for 500ms. (Instead the "customize" voice commmand calls sleep.) This should make it easier for users to override the action if they want it to do something else.
  4. The action does error handling on each platform.
  5. The action tries to open the file in a text editor if there's a natural way to do that (eg. invoking the 'edit' action on Windows, passing the -t flag on Mac).

Would appreciate another review.

@rntz rntz requested a review from pokey June 4, 2022 15:02
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.

Beautiful 👏

Lmk if you want me to test it out locally; otherwise feel free to merge it in

@nriley
Copy link
Collaborator

nriley commented Jun 4, 2022

WFM on macOS and Windows. Perhaps consider renaming open_file.py to edit_[text_]file.py?

code/open_file.py Outdated Show resolved Hide resolved
code/open_file.py Outdated Show resolved Hide resolved
@codecat555
Copy link
Contributor

codecat555 commented Jun 4, 2022 via email

@rntz rntz merged commit e314f89 into main Jun 18, 2022
purpleP pushed a commit to purpleP/knausj_talon that referenced this pull request Aug 3, 2022
@pokey pokey deleted the file-open branch August 19, 2022 14:22
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.

5 participants