cursorless-dev
Error occurs if settings.json does not exist (such as for new install before any settings are changed)
I found an interesting scenario and wanted to check how I should handle it.
I was practicing my demo for my new tool - Talon Automated Installation Platform (TAIP) - https://github.com/CodesAway/TAIPCodesAway
This is able to install Talon command sets and VSCode extensions, such as Cursorless.
I learned that a brand new install of VSCode has no settings.json file, and I kept getting errors from the below code, since the settings.json file didn't exist. It only gets created when you modify a setting (opening VSCode won't create the file)
https://github.com/cursorless-dev/cursorless/blob/83820ffe395a7ee134aec1215e01f7d93901a9a6/cursorless-talon/src/marks/decorated_mark.py#L89
When I created an empty settings.json file with "{}", it worked fine (no error)
The error happens when this method cannot find the settings.json and read it as dict, an error occurs.
https://github.com/cursorless-dev/cursorless/blob/83820ffe395a7ee134aec1215e01f7d93901a9a6/cursorless-talon/src/apps/vscode_settings.py#L32
In the meanwhile, I was thinking since my tool is installing Cursorless anyway, I can create the empty dict settings.json file first, so Cursorless doesn't result in an error, confusing new users who have a fresh install of VSCode.
Thoughts? Concerns? Questions? ⁉️
This was a problem back in #543, but this should since be fixed? We intentionally fall back to enabling everything if the VSCode settings files don't exist.
https://github.com/cursorless-dev/cursorless/blob/83820ffe395a7ee134aec1215e01f7d93901a9a6/cursorless-talon/src/marks/decorated_mark.py#L95-L99
That's correct, the code still works, though puts an "error message" in the Talon log each time (line 99 in your referenced code)
It also notifies the user (via Talon notification) about the "error", asking them to restart Talon.
It will do this EVERY time the user loads Talon. I see this as very confusing. I was confused until I checked the code and realized the "error" didn't impact anything. Sounds like it maybe should be a warning and NOT notify the user each time to restart Talon. Thoughts?
https://github.com/cursorless-dev/cursorless/blob/4e83c80f6b8041ec102ae32a57a79301c70c9358/cursorless-talon/src/marks/decorated_mark.py#L145-L146
It's duck typing...if an action
- Logs like an error
- Notifies like an error
- It's an error
My goal is to reach non-technical people to allow Talon, Cursorless, and voice control to become widespread. Having "errors" that aren't really errors will likely confuse people (as it did me).
How can we improve the user experience?
For example, adding an existence check to see if the file exists. If not, show a warning message and don't notify the user.
Do this before the dict fails to read and shows as an error.
That is basically what the error is indicating. We couldn't find a setting file. Since there is many places the settings file could be located there is no way (today) for cursorless to differentiate between looking in the wrong place and the settings file not existing.
We could fix this two different ways:
- Build a better detection mechanism to see if we have found the vscode folder. We were hoping that we would have tall on rpc at this point so we haven't prioritizing building something ourself.
- Update the error message with a note saying that if it's a fresh vscode installation you need to change a setting to create the file.
I like the idea of checking the folder! That's what I did in my workaround. If the folder is there, but not the settings.json, then TAIP will create it for you when installing Cursorless (so the user doesn't see an error notification).
One question I have specifically is around the notification. The log messages make sense; I'd call them warnings not errors, but that's semantics.
The notification says Error reading vscode settings. Restart talon; see log
When would restarting Talon help? It seems if the setting file can't be found, then restarting Talon wouldn't help...
This would lead to the user restarting and seeing the same error notification...over and over after each restart (happened to me, so confirmed).
Can someone help clarify this notification? How does it improve the user's experience?
Checking the folder makes sense.
No in this case restarting Talon wouldn't do anything.
Any objections with me submitting a PR where it looks for the folder first? If it finds the folder and not the settings file, it's not an error, jist fallback.