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

Place Talon and Python files side-by-side #897

Merged
merged 22 commits into from
Oct 1, 2022
Merged

Conversation

wenkokke
Copy link
Collaborator

@wenkokke wenkokke commented Jun 23, 2022

Opening this pull request to have a working example for discussion about #669.

My attempt with this restructuring is to only focus on renaming files, to ease future merges.

I've not updated any of the imports or file paths, as I mostly intend for this to fuel discussion.

My rationale for organizing is:

  • apps: concrete commands for an application
  • core: concrete commands and actions that are always available and any other code can depend on
  • lang: concrete commands for a programming language
  • tags: concrete commands with some abstract actions, activated with a tag
  • plugins: a self-contained set of commands not linked to any application or programming language but whose actions probably should not be depended upon

My aim with grouping each pair of Python and Talon files in their own subdirectory is (1) to make it extremely clear which files belong together, and (2) to encourage some modularisation. As a bonus, these subdirectories can contain a README.md, which can really help someone browsing the repository on GitHub.

@wenkokke wenkokke marked this pull request as draft June 23, 2022 23:13
@wenkokke
Copy link
Collaborator Author

wenkokke commented Jun 24, 2022

The pull request creates a temporary directory, core/misc, which contains code that either should be pried apart...

  • tags.py creates a list of tags which should be inlined into their respective files
  • application_matches.py creates a list of application matches which should be inlined into their respective files

...or contains general actions, which it very least could do with some documentation as to where they are used...

  • delayed_speech_off.py defines delayed_speech_on and delayed_speech_off
  • engine.py defines an alias for speech_system.engine_mimic as well as shortcuts for "go to sleep" and "wake up"
  • system_command.py defines aliases for os.system and subprocess.Popen

@wenkokke
Copy link
Collaborator Author

Further changes, that should perhaps be made in a separate pull request, would be to pry apart the implementations of the built in Talon name spaces windows and tabs, which are deeply intertwined with each other as well as with window snapping.

@wenkokke
Copy link
Collaborator Author

One further change that should be considered for this pull request is to flatten...

apps/platforms/PLATFORM/APP/APP.{py,talon}

...to simply...

apps/APP/APP_PLATFORM.{py,talon}

It makes sense to want to separate out apps that are a core part of an OS, such as Windows Explorer or Nautilus, and these apps should perhaps be tagged with their OS.
However, a lot of the apps in these folders could run on different systems, but simply don't have a Talon implementation for other systems, e.g., tmux or keepassx.
Furthermore, a lot of the apps at the top level are portable but do not implement support for all OSes, e.g., eclipse.

It honestly might make more sense to group some apps by the main tag they implement, e.g., apps/browser, apps/editor, apps/file_manager, apps/terminal, apps/reader, etc.

@rntz
Copy link
Collaborator

rntz commented Jul 11, 2022

After looking at this in more detail, I mostly like it but have quibbles with how nested some things are and the organization of things in core/. Things I'd like changed:

  1. This isn't something you've introduced, but I'd like to reduce nesting in apps/platforms/PLATFORM/APP/ by, as you suggest, flattening to just apps/APP/. We already did this (by accident?) with apps/windbg/. We may need to rename some apps with overly generic names, eg apps/platforms/mac/notes -> apps/apple_notes. I don't think we particularly need to group apps by their main tag or function.

  2. Flatten core/app_switcher/app_names/ into core/app_switcher/.

  3. core/history/ combines the command and phrase histories; do these really belong together? I think the phrase history either belongs on its own or with formatters.

  4. I'd prefer if all subdirectories of core/modes/ were removed and it was entirely flat.

  5. In particular, core/modes/dictation_mode/dictation_mode.py (formerly code/dictation.py) isn't really the implementation of dictation-mode. It implements (a) the various text captures (<user.word>, <user.text>, <user.prose>); (b) prose auto-formatting; (c) context-sensitive dictation. It should be renamed; maybe text_and_dictation.py? Other suggestions welcome.

Arguably vocabulary.py, formatters.{py,talon}, and text_and_dictation.py belong together; they're the "text input module". The phrase history either belongs in this module or it could be its own. vocabulary.talon and the corresponding functions in vocabulary.py could perhaps be split off into their own module.

The most extreme and simple approach to these "where do things in core belong" difficulties is to completely flatten core/**/ into core/, regarding it as just one big pile of stuff. How would you feel about that?

@wenkokke
Copy link
Collaborator Author

I think that I agree with all of the changes that you've suggested, though for this pull request I believe we should stick as much as possible to renaming files, and not editing their content, for ease of future merges.

Would you be happy to make the changes that you have suggested and push them to this branch?

@rntz
Copy link
Collaborator

rntz commented Jul 20, 2022

Okay, I've updated the branch with my suggested changes. feel free to suggest further renames/reorgs, not confident every choice is the best it could be.

Along the way, I noticed the core/plugin distinction is a bit unclear. Supposedly plugins aren't to be depended on, but mouse_grid defines a tag used in core/numbers/numbers.talon. (And in future we might want talon_draft_window to depend on draft_editor, I think?) And lots of stuff in core/ is self-contained and not depended on by anything else: talon_helpers, text_navigation, macro, the command history, repeater.talon...

We could either:

  1. Keep as-is, maybe there's some reason I'm not seeing.

  2. Collapse the distinction. What should we call the remaining directory? core? misc? modules? code?

  3. Treat lots more stuff as plugins -- anything not depended on by something else, and that doesn't define actions, captures, tags, etc intended for use outside of itself. I've made a branch with this additional change applied: https://github.com/knausj85/knausj_talon/tree/more-plugins (comparison with this PR: https://github.com/wenkokke/knausj_talon/compare/issue669...knausj85:knausj_talon:more-plugins?expand=1)

@wenkokke
Copy link
Collaborator Author

Treat lots more stuff as plugins -- anything not depended on by something else, and that doesn't define actions, captures, tags, etc intended for use outside of itself. I've made a branch with this additional change applied: https://github.com/knausj85/knausj_talon/tree/more-plugins (comparison with this PR: https://github.com/wenkokke/knausj_talon/compare/issue669...knausj85:knausj_talon:more-plugins?expand=1)

That seems like the way to go! I didn't notice these :)

@rntz
Copy link
Collaborator

rntz commented Jul 21, 2022

ok, I've made more things plugins (and also made mouse_grid not a plugin since numbers depends on it)

@rntz
Copy link
Collaborator

rntz commented Jul 21, 2022

Checklist:

  • Merge/rebase on main to fix merge conflict.
  • Get the test suite working.
  • Run talon with this config and test it.
  • Get other maintainers to sign off on this major reorg.
  • Figure out what's up with the deleted files.

@rntz
Copy link
Collaborator

rntz commented Jul 27, 2022

This PR appears to just plain delete some files:

  • eye_tracking_settings.py
  • settings.talon
  • shared_settings_module.py

What's up with that? as I see it:

  • settings.talon we definitely want to keep.
  • shared_settings_module.py defines a setting that is used in the mouse_grid, so maybe it belongs there rather than deleted.
  • eye_tracking_settings.py was entirely commented out. I'm not sure what its purpose is, but it might be important for folks with eye trackers?

@rntz
Copy link
Collaborator

rntz commented Jul 30, 2022

We had a big discussion about this on the knausj maintenance meeting today. Everyone is on board with the general direction, we did some bikeshedding about how things should be organized. Changes:

  1. Apps which are platform-specific (eg Finder, Apple Notes) don't get a platform suffix on their .talon or .py file names.
  2. We flattened apps/web/* into apps/. Some apps (eg outlook, in principle github) have both desktop and web versions, we're going to group those together as one directory.
  3. We flattened core/misc/ into core/, as well as core/app_running/app_running.py because people felt it didn't need a directory all to itself.

@rntz
Copy link
Collaborator

rntz commented Jul 30, 2022

Some changes we'll want to investigate post-merge (or pre-merge on main): a bunch of apps are probably overly platform-specific:

apps/signal/signal_linux: probably works on both linux and windows
apps/taskwarrior/taskwarrior_linux: should work anywhere taskwarrior tag is used
apps/keepassx/keepassx_linux.talon: probably linux+windows?
apps/termite/termite_linux.talon: probably linux-specific but has no 'os: linux' header
apps/tmux/tmux.talon: lose the 'os: linux'

@wenkokke
Copy link
Collaborator Author

  1. Apps which are platform-specific (eg Finder, Apple Notes) don't get a platform suffix on their .talon or .py file names.

One thing to discuss might be how to deal with platform specific apps with very generic names, e.g., Apple vs Gnome vs Windows Terminal, or more generally apps which have non-unique names.

@pokey
Copy link
Collaborator

pokey commented Sep 14, 2022

Ok I made an attempt to restore the deleted files in 6e5e705

I tried running this PR locally, and got the following errors:

2022-09-14 13:03:35 ERROR user.knausj_talon.core.create_spoken_forms (/Users/pokey/.talon/user/knausj_talon/core/create_spoken_forms.py) import failed
   13:                    lib/python3.9/threading.py:937* # loader thread
   12:                    lib/python3.9/threading.py:980* 
   11:                    lib/python3.9/threading.py:917* 
   10:                       app/resources/loader.py:894| 
    9:                       app/resources/loader.py:830| 
    8:                       app/resources/loader.py:648| 
    7:                       app/resources/loader.py:684| 
    6:                       app/resources/loader.py:738| 
    5:                       app/resources/loader.py:432| # [stack splice]
    4:           lib/python3.9/importlib/__init__.py:127| 
    3:                       app/resources/loader.py:253| 
    2:                       app/resources/loader.py:248| 
    1: user/knausj_talon/core/create_spoken_forms.py:9  | from .abbreviate import abbreviations
ImportError: cannot import name 'abbreviations' from 'user.knausj_talon.core.abbreviate' (unknown location)
2022-09-14 13:03:35 ERROR user.knausj_talon.core.file_extension.file_extension (/Users/pokey/.talon/user/knausj_talon/core/file_extension/file_extension.py) import failed
   14:                              lib/python3.9/threading.py:937* # loader thread
   13:                              lib/python3.9/threading.py:980* 
   12:                              lib/python3.9/threading.py:917* 
   11:                                 app/resources/loader.py:894| 
   10:                                 app/resources/loader.py:830| 
    9:                                 app/resources/loader.py:648| 
    8:                                 app/resources/loader.py:684| 
    7:                                 app/resources/loader.py:738| 
    6:                                 app/resources/loader.py:432| # [stack splice]
    5:                     lib/python3.9/importlib/__init__.py:127| 
    4:                                 app/resources/loader.py:253| 
    3:                                 app/resources/loader.py:248| 
    2: user/knausj_talon/core/file_extension/file_extension.py:3  | from .user_settings import get_list_fr..
    1:                                 app/resources/loader.py:366| 
ModuleNotFoundError: No module named 'user.knausj_talon.core.file_extension.user_settings'
2022-09-14 13:03:35 ERROR user.knausj_talon.core.vocabulary.vocabulary (/Users/pokey/.talon/user/knausj_talon/core/vocabulary/vocabulary.py) import failed
   14:                      lib/python3.9/threading.py:937* # loader thread
   13:                      lib/python3.9/threading.py:980* 
   12:                      lib/python3.9/threading.py:917* 
   11:                         app/resources/loader.py:894| 
   10:                         app/resources/loader.py:830| 
    9:                         app/resources/loader.py:648| 
    8:                         app/resources/loader.py:684| 
    7:                         app/resources/loader.py:738| 
    6:                         app/resources/loader.py:432| # [stack splice]
    5:             lib/python3.9/importlib/__init__.py:127| 
    4:                         app/resources/loader.py:253| 
    3:                         app/resources/loader.py:248| 
    2: user/knausj_talon/core/vocabulary/vocabulary.py:8  | from .user_settings import append_to_c..
    1:                         app/resources/loader.py:366| 
ModuleNotFoundError: No module named 'user.knausj_talon.core.vocabulary.user_settings'
2022-09-14 13:03:35 ERROR user.knausj_talon.core.websites_and_search_engines.websites_and_search_engines (/Users/pokey/.talon/user/knausj_talon/core/websites_and_search_engines/websites_and_search_engines.py) import failed
   14:                                                        lib/python3.9/threading.py:937* # loader thread
   13:                                                        lib/python3.9/threading.py:980* 
   12:                                                        lib/python3.9/threading.py:917* 
   11:                                                           app/resources/loader.py:894| 
   10:                                                           app/resources/loader.py:830| 
    9:                                                           app/resources/loader.py:648| 
    8:                                                           app/resources/loader.py:684| 
    7:                                                           app/resources/loader.py:738| 
    6:                                                           app/resources/loader.py:432| # [stack splice]
    5:                                               lib/python3.9/importlib/__init__.py:127| 
    4:                                                           app/resources/loader.py:253| 
    3:                                                           app/resources/loader.py:248| 
    2: user/knausj_talon/core/websites_and_search_engines/websites_and_search_engines.py:6  | from .user_settings import get_list_fr..
    1:                                                           app/resources/loader.py:366| 
ModuleNotFoundError: No module named 'user.knausj_talon.core.websites_and_search_engines.user_settings'
2022-09-14 13:03:39 ERROR cb error topic="ready" cb=on_ready
   13:                          lib/python3.9/threading.py:937* # loader thread
   12:                          lib/python3.9/threading.py:980* 
   11:                          lib/python3.9/threading.py:917* 
   10:                             app/resources/loader.py:894| 
    9:                             app/resources/loader.py:844| 
    8: -------------------------------------------------------# [stack splice]
    7:                         talon/scripting/dispatch.py:134| # 'ready' user.knausj_talon.core.app_switcher.app_switcher:on_ready()
    6: user/knausj_talon/core/app_switcher/app_switcher.py:356| update_overrides(None, None)
    5: user/knausj_talon/core/app_switcher/app_switcher.py:226| update_running_list()
    4: user/knausj_talon/core/app_switcher/app_switcher.py:192| running = actions.user.create_spoken_f..
    3:                          talon/scripting/actions.py:62 | 
    2:                          talon/scripting/actions.py:156| 
    1:                          talon/scripting/actions.py:152| 
KeyError: "Action 'user.create_spoken_forms_from_list' is not declared."
2022-09-14 13:03:39 ERROR cb error topic="ready" cb=on_ready
   12:            lib/python3.9/threading.py:937* # loader thread
   11:            lib/python3.9/threading.py:980* 
   10:            lib/python3.9/threading.py:917* 
    9:               app/resources/loader.py:894| 
    8:               app/resources/loader.py:844| 
    7: -----------------------------------------# [stack splice]
    6:           talon/scripting/dispatch.py:134| # 'ready' user.knausj_talon.lang.talon.talon:on_ready()
    5: user/knausj_talon/lang/talon/talon.py:69 | on_update_decls(registry.decls)
    4: user/knausj_talon/lang/talon/talon.py:59 | ] = actions.user.create_spoken_forms_f..
    3:            talon/scripting/actions.py:62 | 
    2:            talon/scripting/actions.py:156| 
    1:            talon/scripting/actions.py:152| 
KeyError: "Action 'user.create_spoken_forms_from_list' is not declared."

@wenkokke
Copy link
Collaborator Author

I never updated any imports, as the PR was originally only intended to get the idea across. I’ll have a look at that now.

core/settings.talon Outdated Show resolved Hide resolved
@rntz
Copy link
Collaborator

rntz commented Sep 18, 2022 via email

@wenkokke wenkokke marked this pull request as ready for review September 27, 2022 13:29
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.

Ok I think this PR is good to go. I did some basic testing, but it's prob worth a couple people taking it for a spin before we merge, given the surface area that it touches (ie every file 😅)

@rntz
Copy link
Collaborator

rntz commented Sep 27, 2022

I think it's probably worth us maintainers dogfooding this for a week before merging it into main, just to see if any unexpected issues crop up. I'll start this weekend or sooner.

@pokey
Copy link
Collaborator

pokey commented Sep 28, 2022

I think it's probably worth us maintainers dogfooding this for a week before merging it into main, just to see if any unexpected issues crop up. I'll start this weekend or sooner.

Are you comfortable enough with stock knausj to use as a daily driver? Or were you planning to merge into your branch and dog food it that way? Tbh I don't know stock knausj well enough to comfortably use it for more than quick tests

@rntz
Copy link
Collaborator

rntz commented Sep 28, 2022 via email

@pokey
Copy link
Collaborator

pokey commented Sep 28, 2022

Fair enough. I don't think I can force-push to this branch, so I made a new branch with everything squashed for ease of merge: place-talon-and-python-files-side-by-side-squashed

@pokey
Copy link
Collaborator

pokey commented Sep 28, 2022

Ok just merged my personal branch with the squashed commit for this PR in place-talon-and-python-files-side-by-side-squashed; was a little more painful than I was hoping, because I guess I've added / removed a couple files, but not tooo terrible. So far so good in terms of my Talon not breaking; will report back if anything seems odd over the next few days. Also confirmed that on my branch nothing unexpected was deleted; all just renames

Fwiw my personal merge is in https://github.com/pokey/pokey_talon/tree/place-talon-and-python-files-side-by-side. Waiting for this PR to merge before I put a merge commit on my main

@pokey pokey merged commit b25bac4 into talonhub:main Oct 1, 2022
rntz added a commit that referenced this pull request Oct 9, 2022
rntz added a commit that referenced this pull request Oct 11, 2022
Addresses #994 and
spruces up a few places where the readme has gotten out of date.
nriley pushed a commit to nriley/talon_community that referenced this pull request Oct 15, 2022
Addresses talonhub#994 and
spruces up a few places where the readme has gotten out of date.
nickvisut pushed a commit to nickvisut/knausj_talon that referenced this pull request Nov 1, 2022
Opening this pull request to have a working example for discussion about
talonhub#669.

My attempt with this restructuring is to *only* focus on renaming files,
to ease future merges.

I've not updated any of the imports or file paths, as I mostly intend
for this to fuel discussion.

My rationale for organizing is:
- `apps`: concrete commands for an application
- `core`: concrete commands and actions that are always available and
any other code can depend on
- `lang`: concrete commands for a programming language
- `tags`: concrete commands with some abstract actions, activated with a
tag
- `plugins`: a self-contained set of commands not linked to any
application or programming language but whose actions probably should
not be depended upon

My aim with grouping each pair of Python and Talon files in their own
subdirectory is (1) to make it extremely clear which files belong
together, and (2) to encourage some modularisation. As a bonus, these
subdirectories can contain a `README.md`, which can really help someone
browsing the repository on GitHub.

Co-authored-by: Michael Arntzenius <daekharel@gmail.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
nickvisut pushed a commit to nickvisut/knausj_talon that referenced this pull request Nov 1, 2022
Addresses talonhub#994 and
spruces up a few places where the readme has gotten out of date.
trillium pushed a commit to trillium/talonhub_community that referenced this pull request Nov 9, 2022
Opening this pull request to have a working example for discussion about
talonhub#669.

My attempt with this restructuring is to *only* focus on renaming files,
to ease future merges.

I've not updated any of the imports or file paths, as I mostly intend
for this to fuel discussion.

My rationale for organizing is:
- `apps`: concrete commands for an application
- `core`: concrete commands and actions that are always available and
any other code can depend on
- `lang`: concrete commands for a programming language
- `tags`: concrete commands with some abstract actions, activated with a
tag
- `plugins`: a self-contained set of commands not linked to any
application or programming language but whose actions probably should
not be depended upon

My aim with grouping each pair of Python and Talon files in their own
subdirectory is (1) to make it extremely clear which files belong
together, and (2) to encourage some modularisation. As a bonus, these
subdirectories can contain a `README.md`, which can really help someone
browsing the repository on GitHub.

Co-authored-by: Michael Arntzenius <daekharel@gmail.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
trillium pushed a commit to trillium/talonhub_community that referenced this pull request Nov 9, 2022
Addresses talonhub#994 and
spruces up a few places where the readme has gotten out of date.
wenkokke added a commit to wenkokke/talon-community that referenced this pull request Dec 28, 2022
Opening this pull request to have a working example for discussion about
talonhub#669.

My attempt with this restructuring is to *only* focus on renaming files,
to ease future merges.

I've not updated any of the imports or file paths, as I mostly intend
for this to fuel discussion.

My rationale for organizing is:
- `apps`: concrete commands for an application
- `core`: concrete commands and actions that are always available and
any other code can depend on
- `lang`: concrete commands for a programming language
- `tags`: concrete commands with some abstract actions, activated with a
tag
- `plugins`: a self-contained set of commands not linked to any
application or programming language but whose actions probably should
not be depended upon

My aim with grouping each pair of Python and Talon files in their own
subdirectory is (1) to make it extremely clear which files belong
together, and (2) to encourage some modularisation. As a bonus, these
subdirectories can contain a `README.md`, which can really help someone
browsing the repository on GitHub.

Co-authored-by: Michael Arntzenius <daekharel@gmail.com>
Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
wenkokke pushed a commit to wenkokke/talon-community that referenced this pull request Dec 28, 2022
Addresses talonhub#994 and
spruces up a few places where the readme has gotten out of date.
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