gh-89520: IDLE - Make extentions use user's keys, not all defaults #28713
Conversation
I was trying to write an extension for Idle, and was noticing extension keys defined in ~/.idlerc weren't being used, so I looked into it and found `GetExtensionKeys`, `__GetRawExtensionKeys`, and `GetKeyBinding` would never check for user keys, always assuming default keys have everything. This is not always true. In a system with multiple users, you would not want to modify the default config file for a custom extension, as that is used for everyone. This pull request is changing aforementioned functions to also check user config files, and overwrite values acquired from the default config file if the user config redefined one.
Config saving and loading also weren't working quite right for extensions.
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Fixed failing tests as of 016ce89 |
|
As mentioned above, I'm not able to provide a full review on this PR. |
|
@terryjreedy Ping, it's been 3 years and I think my newest approach will work well. I've been testing a version of what this PR does as it's own module that has to do some nasty monkeypatching stuff, but it allows IDLE to successfully load extensions from user configuration files, and I would like to see this become something standard. |
There was a problem hiding this comment.
These comments are very preliminary. And when I wrote the first 2, I had forgotten that this PR intentionally changes a config rule. But its late, so I am submitting them as they are to give you some feedback.
I am a bit more favorable toward this than I was on the issue, but no promises yet.
If I were to approve, new tests would be needed, modeled on existing tests that avoid reading/writing the user files on disk by loading the in-memory config dicts. zzdummy.py, or a copy in idle_test, might be usable.
| for key in self.userCfg: | ||
| self.userCfg[key].Load() |
There was a problem hiding this comment.
This change looks wrong, as it would load any garbage that users add to the user file. Since it affects all confit files, not obvious related to this PR.
There was a problem hiding this comment.
This change allows keys that are not in the default configuration to be loaded from user configuration. This means sections like extensions that might not be in the default config can be loaded.
@terryjreedy Ping, I have reviewed your feedback and have made noted changes, and I've added a few tests that try to capture the fact that and extension that is not enabled will not have it's keybinds bound but that one that is enabled will, as well as making sure extensions that are enabled entirely from user configuration files will have their keybinds found successfully. To do this I made a copy of |
|
All commit authors signed the Contributor License Agreement. |
I was trying to write an extension for Idle, and was noticing extension keys defined in ~/.idlerc weren't being used, so I looked into it and found that in idlelib.config.idleConf,
GetExtensionKeys,__GetRawExtensionKeys, andGetKeyBindingwould never check for user keys, always assuming default keys have everything. This is not always true. In a system with multiple users, you would not want to modify the default config file for a custom extension, as that is used for everyone. This pull request is changing aforementioned functions to also check user config files, and overwrite values acquired from the default config file if the user config redefined one.Edit:
The especially interesting part is that extensions are loaded anyways if they have an entry in the user's IDLE configuration file, but their keybinds are only loaded properly if they are defined in the root configuration file at the moment. This pull request is fixing this difference and allows extension keybinds to be loaded from the user's IDLE configuration file, making user extensions work just as well as root extensions.