IDLE: configdialog -- factor out VarTrace class

[terryjreedy]

BPO	30853
Nosy	@terryjreedy, @csabella
PRs	bpo-30853: IDLE: TraceVar class and tests #2872 [3.6] bpo-30853: IDLE: Factor a VarTrace class from configdialog.Con… #2903 bpo-30853: IDLE: Convert font and general vars to use VarTrace #2914 [3.6] bpo-30853: IDLE: Convert font and general vars to use VarTrace … #2935 bpo-30853: IDLE - touch-up configdialog.VarTrace and tests. #2936 [3.6] bpo-30853: IDLE - touch-up configdialog.VarTrace and tests. (GH… #2937

^{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:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2017-07-28.22:37:27.464>
created_at = <Date 2017-07-05.06:03:26.402>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'IDLE: configdialog -- factor out VarTrace class'
updated_at = <Date 2017-07-28.22:39:16.355>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2017-07-28.22:39:16.355>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2017-07-28.22:37:27.464>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2017-07-05.06:03:26.402>
creator = 'terry.reedy'
dependencies = []
files = []
hgrepos = []
issue_num = 30853
keywords = []
message_count = 22.0
messages = ['297703', '298886', '298921', '298929', '299012', '299014', '299270', '299271', '299274', '299280', '299283', '299286', '299312', '299313', '299359', '299382', '299388', '299420', '299428', '299430', '299438', '299439']
nosy_count = 2.0
nosy_names = ['terry.reedy', 'cheryl.sabella']
pr_nums = ['2872', '2903', '2914', '2935', '2936', '2937']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30853'
versions = ['Python 3.6', 'Python 3.7']

[terryjreedy]

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:
changes = <instance of changes> # set externally somehow
var = set()

    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)

[terryjreedy]

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 = StringVar(parent)
  

+ self.font_name = tracers.add(StringVar(parent), var_changed_font)

[csabella]

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?

[terryjreedy]

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
2545153771784c
hello ('PY_VAR0', '', 'write')
hello ('PY_VAR0', '', 'write')

The class needs two lists: untraced and traced. Attach should pop from untraced and append to traced. The reverse for detach (remove renamed).
One of the test tracers should increment an int var, so we can test that after calling attach twice, there is one one increment.

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.

[csabella]

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.

[terryjreedy]

I am going to continue with the tests.

[terryjreedy]

New changeset 45bf723 by Terry Jan Reedy (csabella) in branch 'master':
bpo-30853: IDLE: Factor a VarTrace class from configdialog.ConfigDialog. (bpo-2872)
45bf723

[terryjreedy]

I will include new blurb with PR that uses this with font vars. I might do this tonight.

Test coverage of class is 100%.

[csabella]

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.

[terryjreedy]

New changeset 0243bea by Terry Jan Reedy in branch '3.6':
[3.6] bpo-30853: IDLE: Factor a VarTrace class from configdialog.ConfigDialog. (GH-2872) (bpo-2903)
0243bea