[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
resolution:  -> fixed
stage: needs patch -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset ecc80b3f1b56f1e4df9e592f8527e622a6b45e01 by Terry Jan Reedy in 
branch '3.6':
[3.6] bpo-30853: IDLE - touch-up configdialog.VarTrace and tests. (GH-2936) 
(#2937)
https://github.com/python/cpython/commit/ecc80b3f1b56f1e4df9e592f8527e622a6b45e01


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
pull_requests: +2989

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset 5d0f30aae5fccc99690923fc5c7cb58de8ad7eec by Terry Jan Reedy in 
branch 'master':
bpo-30853: IDLE - touch-up configdialog.VarTrace and tests. (#2936)
https://github.com/python/cpython/commit/5d0f30aae5fccc99690923fc5c7cb58de8ad7eec


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset 02f88d2a411a6a789b33be281adfc3570c49efd5 by Terry Jan Reedy in 
branch '3.6':
[3.6] bpo-30853: IDLE: Convert font and general vars to use VarTrace (GH-2914) 
(#2935)
https://github.com/python/cpython/commit/02f88d2a411a6a789b33be281adfc3570c49efd5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
pull_requests: +2988

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
pull_requests: +2987

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset 5b59154c0d3d91c0766b9177f6b737b1abcbf3f6 by Terry Jan Reedy 
(csabella) in branch 'master':
bpo-30853: IDLE: Convert font and general vars to use VarTrace (#2914)
https://github.com/python/cpython/commit/5b59154c0d3d91c0766b9177f6b737b1abcbf3f6


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

"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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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 `name` and `name2` though (I think that's 
the only one with two add_option calls).  If the callback didn't have a 
changes.add_option, then it can send None for config.

I hadn't thought of separating `parent`, but I like that idea.  It fits in with 
the rest of how the widgets are created. 

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:
changes.add_option(*tracers[var].config, value)

Wouldn't work for var_changed_font because that has the three add_option calls.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Changes by Cheryl Sabella :


--
pull_requests: +2966

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset 0243bea55dc340067247e635442f2a227705315a by Terry Jan Reedy in 
branch '3.6':
[3.6] bpo-30853:  IDLE: Factor a VarTrace class from configdialog.ConfigDialog. 
(GH-2872) (#2903)
https://github.com/python/cpython/commit/0243bea55dc340067247e635442f2a227705315a


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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

Test coverage of class is 100%.

--
stage: test needed -> needs patch
versions: +Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
pull_requests: +2954

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:


New changeset 45bf723c6c591ec56a18dad8150ae89797450d8b by Terry Jan Reedy 
(csabella) in branch 'master':
bpo-30853:  IDLE: Factor a VarTrace class from configdialog.ConfigDialog. 
(#2872)
https://github.com/python/cpython/commit/45bf723c6c591ec56a18dad8150ae89797450d8b


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Changes by Cheryl Sabella :


--
pull_requests: +2923

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

I am going to continue with the tests.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Cheryl Sabella]

Cheryl Sabella added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30853] IDLE: configdialog -- factor out Tracer subclass

[Terry J. Reedy]

Terry J. Reedy added the comment:

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)

--
nosy: +csabella
title: IDLE: configdialog -- factor out Variable subclass -> IDLE: configdialog 
-- factor out Tracer subclass

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com