Author epaine
Recipients Lance Ware, epaine, markroseman, serhiy.storchaka, terry.reedy, zach.ware
Date 2020-09-24.10:09:52
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1600942195.9.0.00881290169539.issue28694@roundup.psfhosted.org>
In-reply-to
Content
I have started looking at Mark's wrapper (though I have not been able to
​spend as much time on it as I would like). It's mostly small changes
​that I have made (in no particular order):
- TkFontchooserFontChanged event calls 'command' (to be behave like an
'apply' button - this only occurs on platforms that set font due to user
​actions, otherwise we don't know what font has been selected)
- a cget method (which getitem now forwards to)
- 'show' takes 'configure' kwargs
- allow instantiation without kwargs before root creation*
- 'ismodal' decorated as property
- 'compatible' module attribute set at import (`tkinter.TkVersion >= 8.6`)
- winfo_toplevel used to ensure parent is a window (see comment at bottom)
- TkFontchooserVisibility has a visibilitycommand kwarg**

* In most use cases, the user will only want a single instance of the
Chooser class through the entire application run and then just change
change the parent as required. The idea here is that we can instantiate
the class at import and then lazy-load the parent so the user can call
something like `tkinter.fontchooser.chooser.show()`. This is the reason
​I decided to decorate 'ismodal' as a property, rather than having it as
​a variable set on class load.

** I would prefer to keep the user from binding virtual events to the
parent themselves and instead intend to provide a kwarg for a command to
be called when this event is generated (I think binding to the parent is
needlessly complicated, especially when we call winfo_toplevel before
​passing to Tk anyway, so I thought it was either this or we provide the
​user with a 'bind' method for the class). Between this proposal and
​TkFontchooserFontChanged calling 'command', this would eliminate all
need for the user to bind the events themselves (hence, they should not
​be included in the docs).

I haven't yet properly tested it (just focusing on changing the
​user-facing api), however I think what Mark has got so far is very close
​to what we should submit as a patch (my ideas are attached as a diff of
​Mark's file). The above are just QOL changes to *hopefully* make it
​easier for the user, rather than another rewrite of the wrapper.

> Documentation would be key

Definitely! I think this is quite possibly the hardest part of the patch
(after deciding what is the 'cleanest' api for the user) because we need
​to ensure the docs are detailed and precise while being accessible to
​those who don't know Tk (and hence won't be digging through the Tk man -
​in which case, as helpful as the "Notable differences from underlying Tk
​API" is internally, it should not be part of the docs).

> I assume the official wrapper needs checks to ensure fontchooser is
> available (i.e. 8.6 or greater). If not, should the wrapper just error out?

Personally, I would just leave the wrapper without any checks for
version validity. All the official installers use 8.6 and there are very
​few distros that don't provide 8.6 as their default Tk package (most
​notably CentOS 6 & 7). We should provide a module-level attribute that
​checks the Tk version for the user (this is the 'compatible' attribute
​included in the diff) but I don't think we should do anything beyond
​this (it is hence up to the user to ensure they are on a compatible
​system). Applications like IDLE, which try to be compatible with older
​versions of Tk, should provide their own fallback interface rather than
​building such an interface into tkinter.


I am not the biggest fan of the current implementation of ismodal as
​this is based purely on Mark's testing of just 3 windowing systems
​(Windows, X11 & MacOS - my point is about the limited number not that I
​doubt Mark's testing!). While I think the BSDs also use X11, we are
​effectively declaring that there are no other windowing systems (on an
​OS CPython supports) where fontchooser is modal (which I very reluctant
​to do). At the same time, though, I don't have a clue how else it would
​be implemented in a user-friendly way (I can think of a couple of ways
​but all of them involve showing the fontchooser temporarily).

My suggestion is, therefore, to raise a NotImplementedError if the
​current windowing system is not one of 'win32', 'x11' or 'aqua'
​(because, of these, we know that only win32 is modal - the docs would
​have to note this limited availability).


Addressing a couple of Mark's points in an email he sent me:

> the -parent option doesn’t need to be a toplevel
In my limited testing (on X11), I found that the virtual events were not
​called unless this was true (or at least it appeared so...).

> there’s some argument to be made that because the bind lets you
​> trigger multiple callbacks it may promote looser coupling
While I see the point, I think the same could be said of Tk having
​-command rather than a virtual binding. I also think we should pick one
​of either kwargs or virtual events and then stick to that to avoid an
​inconsistent api (in my diff, I chose the former of the two).
History
Date User Action Args
2020-09-24 10:09:55epainesetrecipients: + epaine, terry.reedy, markroseman, zach.ware, serhiy.storchaka, Lance Ware
2020-09-24 10:09:55epainesetmessageid: <1600942195.9.0.00881290169539.issue28694@roundup.psfhosted.org>
2020-09-24 10:09:55epainelinkissue28694 messages
2020-09-24 10:09:55epainecreate