Message377443
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). |
|
Date |
User |
Action |
Args |
2020-09-24 10:09:55 | epaine | set | recipients:
+ epaine, terry.reedy, markroseman, zach.ware, serhiy.storchaka, Lance Ware |
2020-09-24 10:09:55 | epaine | set | messageid: <1600942195.9.0.00881290169539.issue28694@roundup.psfhosted.org> |
2020-09-24 10:09:55 | epaine | link | issue28694 messages |
2020-09-24 10:09:55 | epaine | create | |
|