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

Turn hats off by default; add 'play' hat #80

Merged
merged 21 commits into from
Oct 5, 2021
Merged

Turn hats off by default; add 'play' hat #80

merged 21 commits into from
Oct 5, 2021

Conversation

pokey
Copy link
Member

@pokey pokey commented Sep 17, 2021

Closes #79

  • Fix issue with exposing enablement
  • Rename "eye" to improve recognition ("stare"? "eyeball"?)
  • Rename "star" to "cross" to avoid conflict with built-in star
  • Wait for Add new hats and preprocessing script cursorless#277 to be ready
  • Link svgs from cursorless vscode main into docs for shapes

@pokey pokey mentioned this pull request Sep 20, 2021
1 task
@pokey pokey marked this pull request as ready for review September 23, 2021 19:14
def init_csv_and_watch_changes(
filename: str,
default_values: dict[str, dict],
extra_acceptable_values: list[str] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

the extra_acceptable_values makes it so we don't complain if they have customized the name of a style that they're not currently using

"-frame": "frame",
"-curve": "curve",
"-stare": "eye",
"ex": "ex",
Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, we leave them all on by default, but they get filtered if they're not active

def on_ready():
init_csv_and_watch_changes(
"special_marks",
DEFAULT_COLOR_ENABLEMENT = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I know not great to hardcode this stuff. But we won't need to once we do it using the new state sharing stuff so I think this is ok for now

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndreasArvidsson Ok so one problem with this hard coding is that if we turn some shapes on by default extension side, we would have to update this code and then there would be mismatch until they update talon side

We could maybe have cursorless extension write default enablement to json in post-install script?


def on_watch(path, flags):
if vscode_settings_path.match(path):
cron.after("500ms", setup_hat_styles_csv)
Copy link
Member Author

Choose a reason for hiding this comment

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

Was getting some weird race conditions with fs.watch where I'd see the old value sometimes

@@ -0,0 +1,58 @@
import json
Copy link
Member Author

Choose a reason for hiding this comment

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

This file should prob make its way into knausj at some point, but for now leaving it here

@@ -0,0 +1,137 @@
# From https://github.com/linjackson78/jstyleson/blob/8c47cc9e665b3b1744cccfaa7a650de5f3c575dd/jstyleson.py
Copy link
Member Author

Choose a reason for hiding this comment

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

Vendoring this one because it's painful to pip install stuff. Small file so I don't think it's a big deal. We use this for parsing jsonc

@pokey
Copy link
Member Author

pokey commented Sep 30, 2021

btw @AndreasArvidsson one thing I think is nice about this approach is that if they never enable a shape, it never appears in their overrides csv, so if we change the default name later they'll get the latest one when they enable the shape

@@ -16,7 +16,7 @@
"<user.cursorless_target> [{user.cursorless_source_destination_connective} <user.cursorless_target>]"
)
)
def cursorless_move_bring_targets(m) -> str:
def cursorless_move_bring_targets(m) -> list[dict]:
Copy link
Member Author

Choose a reason for hiding this comment

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

talon seems to have gotten stricter about type checking so this one was causing errors; seemed small enough to tack onto this PR

Comment on lines +75 to +78
targets: list[dict],
arg1: any = NotSet,
arg2: any = NotSet,
arg3: any = NotSet,
Copy link
Member Author

Choose a reason for hiding this comment

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

moving to the new generics


def on_watch(path, flags):
if vscode_settings_path.match(path):
cron.after("1s", setup_hat_styles_csv)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not in love with this sleep but seems to make things pretty robust on my computer and I don't think the delay is a big deal

Comment on lines +40 to +42
def pick_path(paths: list[Path]):
existing_paths = [path for path in paths if path.exists()]
return max(existing_paths, key=lambda path: path.stat().st_mtime)
Copy link
Member Author

Choose a reason for hiding this comment

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

here's the recency logic we were talking about

@@ -0,0 +1,50 @@
import json
Copy link
Member Author

Choose a reason for hiding this comment

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

just copied from command server

Copy link
Member

@AndreasArvidsson AndreasArvidsson 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 and is working fine for me now. The whole issue that the settings file could be in a different location is still a problem of course.

@pokey pokey merged commit b6556a1 into develop Oct 5, 2021
@pokey pokey deleted the pokey-hats-off branch October 5, 2021 17:39
@pokey pokey mentioned this pull request Oct 6, 2021
1 task
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.

2 participants