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

coalesce some list updates #1348

Merged
merged 6 commits into from
Jan 30, 2024
Merged

coalesce some list updates #1348

merged 6 commits into from
Jan 30, 2024

Conversation

lunixbochs
Copy link
Collaborator

@lunixbochs lunixbochs commented Jan 3, 2024

Snippets and file manager list updates trigger more context refreshes than necessary. Due to an indentation issue, snippets are currently triggering 101 full context re-evaluations every time snippets load/reload (linear to number of snippets).

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.

I don't think this is right. ctx is local to the outer for loop; defined in its guard

@lunixbochs
Copy link
Collaborator Author

lunixbochs commented Jan 3, 2024

If that's the case, I'm going to strongly recommend you remove the dir setting and use a directory relative to the code, so the contexts can be populated before Talon is ready. I'll also recommend in the fs.watch only updating the file that changed (if you're not doing that). This code path is currently triggering a lot of work inside Talon.

@AndreasArvidsson AndreasArvidsson dismissed their stale review January 3, 2024 16:41

Missed the change in indentation

@knausj85
Copy link
Member

knausj85 commented Jan 21, 2024

I'm inclined to merge this after fixing the indentation and open an issue until someone has the bandwidth to address lunixboch's broader suggestion re: removing snippets setting.

This is at least an improvement in an interim.

@pokey
Copy link
Collaborator

pokey commented Jan 23, 2024

If that's the case, I'm going to strongly recommend you remove the dir setting and use a directory relative to the code, so the contexts can be populated before Talon is ready.

We implemented this feature in response to a user request (#1306), but if untenable, I guess we could roll it back?

I'll also recommend in the fs.watch only updating the file that changed (if you're not doing that). This code path is currently triggering a lot of work inside Talon.

Makes sense; prob for a follow-up PR though. Filed #1373 to track

@knausj85 knausj85 requested a review from pokey January 28, 2024 00:43
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 to me! I have not tested locally, though

@knausj85 knausj85 merged commit 47df47e into talonhub:main Jan 30, 2024
2 checks passed
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
Snippets and file manager list updates trigger more context refreshes
than necessary. Due to an indentation issue, snippets are currently
triggering 101 full context re-evaluations every time snippets
load/reload (linear to number of snippets).

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jeff Knaus <knaus.jeff@gmail.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

4 participants