-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
IDLE: configdialog -- factor out VarTrace class #75036
Comments
configdialog.ConfigDialog creates about 20 tk Variables corresponding to configuration options. It defines a var_changed_xyz callback for each xyz variable. 6 have a common template. ConfigDialog turns tracing on in attach_var_callbacks and off in remove_var_callbacks. Each of these two functions has the callbacks set hard-coded. Proposal: Factor out what can. Register callbacks in set when var created. Iterate register to attach and remove callbacks. Something like def IVariable: def register(self, callback)
if isinstance(callback, tuple):
self.callback = default_callback
self.args = callback
else:
self.callback = callback
self.vars.add()
def default_callback(self): # Used for 6 vars.
changes.add_item(*self.args, self.get())
@classmethod
def attach(cls):
for var in cls.vars:
var.trace_add('write', self.callback)
@classmethod
def remove(cls):
for var in cls.vars:
var.trace_remove('write', var.trace_info()[0][1])
cls.vars = set() To get String/Int/BooleanVars, maybe this will work. (I have only toyed with multiple inheritance.) class IString(tk.StringVar, IVariable):
def __init__(self, callback).
StringVar.__init() # Possibly not needed with no value to pass.
self.register(callback) |
This issue is really about managing the set of var,callback pairs. The idea of wrapping vars just clouded the issue. class VarTrace:
def __init__(self):
self.tracers = [] # or set if need to delete
def add(self, var, callback):
if isinstance(callback, tuple):
callback = self.make_callback(var, callback)
self.vartrace.add((var, callback))
return var
def make_callback(self, var, args): # Used for 6 vars.
def var_callback(*params):
changes.add_item(*args, var.get())
return var_callback
def attach(self):
for var, callback in self.tracers:
var.trace_add('write', callback)
def remove(cls): # detach?
for var, _ in self.tracers:
var.trace_remove('write', var.trace_info()[0][1]) # doc this # A functionalist would define a function that returns a sequence of closures. tracers = VarTrace() # write tests, then patch creation of tested vars, retest, patch rest.
+ self.font_name = tracers.add(StringVar(parent), var_changed_font) |
Would you like this change for fonts and indent before the tests for the other tabs are ready or would you like to have tests for all the tabs first? |
Step 1 is to add the class and then add tests. The tests can use artificial examples. Say 3 vars, 2 callbacks, with the 3rd done as default. I want to start this now -- see below. Let me know if you start working on this so we don't duplicate work. I just determined that calling trace_add with the same var,c pair is a bad idea. import tkinter as tk
r = tk.Tk()
v = tk.StringVar(r)
def c(*args): print("hello", args)
print(v.trace_add('write', c))
print(v.trace_add('write', c))
v.set('a') 2545142942664c The class needs two lists: untraced and traced. Attach should pop from untraced and append to traced. The reverse for detach (remove renamed). I will take care of looking at the doc or code to refresh my memory of why the detach is written the way it is. Step 2, putting this in use, is a prerequisite for writing proper tests for extracted tab page classes, which are a prerequisite for putting the class in use. So I am inclined to convert font vars when a tested class is ready and temporarily have old and new attach functions called. We may discover a need or at least a use for other VarTrace methods when writing tests. |
I would like to work on this, but I don't think I'd be able to have something ready for a few days, so I don't want to hold you up if you would like it sooner. |
I am going to continue with the tests. |
I will include new blurb with PR that uses this with font vars. I might do this tonight. Test coverage of class is 100%. |
Thanks! I can try the font vars if you like. One question I keep forgetting to ask -- I can't figure out how remove_var_callbacks gets called. I've grepped for the name and didn't find it anywhere. I don't know what I'm missing. |
Go ahead. If it works with font, add general tab. CD.remove_var_callbacks is currently only called in test_configdialog.tearDownModule. I added the call to prevent getting a TclError for each callback after the test finished. .destroy does not destroy callbacks. I presume remove_var_callbacks was written to be called in self.cancel. Then perhaps someone discovered that there was no visible effect of deleting it, perhaps because TclErrors are caught and ignored. I want to put it, or the new equivalent, into cancel, before destroy. And make sure to add to teardownmodule. |
See in you can install blurb into 3.6 or even 3.5, (pip should work) and run it to add the blurb file. I will fill in the body. |
The font vars went well, so I also added general vars. I created the blurb. I didn't know if it would work with no body, so I just put a placeholder. I did have another question. Instead of just one tracers.add() method, I was wondering if there's an advantage to having one called add_callback(var, callback) and a separate one called add_default(var, config)? Design-wise, I don't know which is the preferred way, but changing the existing code seemed a little hackish. |
Just as an FYI with blurb, I installed it in the same venv as coverage, so I was able to run it with 3.7. It's really cool. |
A minor change. Complete the path by adding the other var-trace pairs and after checking carefully, I will be ready to push. I prefer one easy to remember short-name function to two longer-named functions. However, the docstring should specify the tuple as idleConf config-type, section, and option names. Either *add* or *make_callback* could check len = 3 and callback[0] in idleConf.config_types, with a test added. "changing the existing code seemed a little hackish": I don't understand exactly what you think is hackish. There might be people who would prefer (possibly as less hakish) def add(tkvar, parent, cb_info):
var = tkvar(parent)
if isinstance(callback, tuple):
callback = self.make_callback(var, cb_info)
else:
callback = cb_info
self.untraced.append((var, callback))
return var I don't know if this would make the add calls readable, but is does factor the var calls into one place, replaces () with ',' (at the cost of passing one more reference), and avoids the hackish return of an input. It also simplifies the code in a way by making it 'flatter' instead of nested. Did I miss something? What do you think? The parameter name change is an independent idea that should satisfy someone who objects to calling something an x that is not an x. |
Instead 'hackish', maybe I should have used 'magic'. The overloading just wasn't obvious to me, meaning I have: self.font_bold = tracers.add(BooleanVar(parent), self.var_changed_font)
self.space_num = tracers.add(IntVar(parent), ('main', 'Indent', 'num-spaces')) We defined VarTrace as being (var, callback) pairs and the second example isn't sending a function. So, even though I understand what we're doing, I wanted to ask about using different names for my own education. I was even thinking of a different interface -- add(var, callback=default, config=None) If config was specified even for the non-default callbacks, then each var could have its config defined at create time instead of in the var_changed* function. This wouldn't work for theme/keys I hadn't thought of separating So, if both changes were incorporated: self.font_bold = tracers.add(parent, BooleanVar,
callback=self.var_changed_font,
config=('main', 'EditorWindow', 'font-bold'))
self.space_num = tracers.add(parent, IntVar,
callback=default,
config=('main', 'Indent', 'num-spaces')) Maybe that expands VarTrace too much? Or maybe instead of (var, callback) pairs, it's a dictionary? tracers = {var: (callback, config)} And then the non-default var_changed methods could use: Wouldn't work for var_changed_font because that has the three add_option calls. |
"Either *add* or *make_callback* could check len = 3 and callback[0] in idleConf.config_types, with a test added." I didn't add this because I wasn't sure what you wanted to happen if it wasn't right. I suspect it should fail gracefully (not break IDLE), but in what way? |
IDLE *will* fail if idleConf.SetOption is called with a wrong config-type (KeyError) or the wrong number of other arguments (TypeError). I did not add the check because it does not cover the non-default callbacks (the majority), let alone all the other idleConf calls. The important thing is to make sure that every callback gets called in a test (and indeed, eventually, that every line is executed). Enhancing the doc string should be enough. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: