-
Notifications
You must be signed in to change notification settings - Fork 279
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 set config engine command #989
Conversation
I'm not against adding this to Plover, although I see no reason this can't be implemented in a separate plugin. |
I actually did try implementing this as a plugin first, but there were a couple reasons I thought it would be better as a built-in feature.
|
6c6b904
to
985e63e
Compare
Okay, it's been converted to a built-in plugin in the same style as the built-in macro commands. engine.py is no longer changed. |
Just an aside, a short plugin is an asset in my mind.
…On Fri, Aug 10, 2018, 7:10 PM fourshade ***@***.***> wrote:
I actually did try implementing this as a plugin first, but there were a
couple reasons I thought it would be better as a built-in feature.
1. The command itself is an incredibly simple addition. I ended up
with a plugin that was six lines of code. About 90% of the plugin's total
content was distribution metadata. Felt like kind of a waste.
2. Config values are central to the operation of Plover; they affect
nearly every part of the system. The effects of config values are
incredibly varied. Plugins are best for specific use-case scenarios; this
command is really a much more general addition with all kinds of uses.
3. The current config code isn't very user-friendly. Right now, the
_lookup of available config options by key in *getitem* does not catch
exceptions; without modifying the core program to catch the KeyError as
mentioned above, the program would simply crash if there was a misspelled
option key in the user's dictionary. A plugin couldn't have changed this
behavior.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#989 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFkgSvxVif2mu4qYDgy01yWh3KLSwE5Vks5uPhLdgaJpZM4Vu1GL>
.
|
I agree a short plugin can indeed be useful. I wish more of the simple command/meta plugins were built-in by default, since they take very little space and don't clutter the UI in any way. Anyway, I've moved everything out of the main code into the plugin. I still feel like config.py could use a good cleanup (exception handling, refactoring all those ConfigOption factory functions into classes) but maybe that's best for another PR. |
Those could be made into default plugins which is also very convenient
…On Fri, Aug 10, 2018, 8:52 PM fourshade ***@***.***> wrote:
I agree a short plugin can indeed be useful. I wish more of the simple
command/meta plugins were built-in by default, since they take very little
space and don't clutter the UI in any way.
Anyway, I've moved everything out of the main code into the plugin. I
still feel like config.py could use a good cleanup (exception handling,
refactoring all those ConfigOption factory functions into classes) but
maybe that's best for another PR.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#989 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFkgSohaVbPxWxjQIUYfxskEU69W2vXOks5uPiq5gaJpZM4Vu1GL>
.
|
This really needs some tests. |
Okay, this looks more promising. By parsing the value with the AST module, it should be possible to set any option you can set in a config file. |
Commits are re-ordered and squashed, tests are fixed up to handle expected exceptions better. |
Okay, it's fixed up a bunch now. Split everything into methods and had them raise a custom exception class. Making the syntax the same as Python dicts means there's no need to keep track of special delimiter constants. Passes all PEP 8 checks (except for possibly line length, but 80 characters feels like the dark ages with tons of continuations on long strings and comments. PyCharm defaults to 120.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't think wrapping the exceptions is needed.
There's no point in validating each option manually before trying to set it when the engine will already do it. And by setting each option individually you potentially break the use-case where some are interdependent: like setting the machine type and its options. This can also result in only part of the settings being applied, instead of the all or nothing behavior if you just use engine.config = _cmdline_to_dict(cmdline)
.
This also results in multiple configuration changes instead of just one.
That's a good idea; doing the update in one shot also simplifies the plugin a great deal. All command parsing exceptions are folded into one try-except now and the actual config update is left to propagate exceptions as normal. |
news.d/feature/989.core.rst
Outdated
"O*EP": "{PLOVER:SET_CONFIG:translation_frame_opacity,100}", | ||
"TR*PB": "{PLOVER:SET_CONFIG:translation_frame_opacity,0}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to show the correct format
"O*EP": "{PLOVER:SET_CONFIG:translation_frame_opacity,100}", | |
"TR*PB": "{PLOVER:SET_CONFIG:translation_frame_opacity,0}", | |
"O*EP": "{PLOVER:SET_CONFIG:'translation_frame_opacity':100}", | |
"TR*PB": "{PLOVER:SET_CONFIG:'translation_frame_opacity':0}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, amended.
Fixes #983.
I've added an engine command: SET_CONFIG, which allows you to set a Plover config setting using a stroke. Mirabai asked for a command that would change the add-translation window opacity, and this does that among other things. Adding entries like this into your dictionary will allow you to set values in the global configuration:
"O*EP": "{PLOVER:SET_CONFIG:translation_frame_opacity,100}",
"TR*PB": "{PLOVER:SET_CONFIG:translation_frame_opacity,0}"
Those change the add translation window opacity, but it should work for any config setting with a value that is type-castable from a string. With a little further work I could make it usable for the more complex config settings, possibly allowing you to do things like switch out dictionaries using this command.