search

python / cpython

The Python programming language
https://www.python.org
Other
63.35k stars 30.32k forks source link

tkinter interface to fontchooser #72880

Open 5b6135d8-ca9d-489e-abea-0c63081b9a95 opened 7 years ago

5b6135d8-ca9d-489e-abea-0c63081b9a95 commented 7 years ago
BPO 28694
Nosy @terryjreedy, @roseman, @zware, @serhiy-storchaka, @E-Paine
Files
  • fontchooser.py
  • fontchooser.py
  • mark.diff
    		•

    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: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'expert-tkinter', '3.10'] title = 'tkinter interface to fontchooser' updated_at = user = 'https://bugs.python.org/LanceWare' ``` bugs.python.org fields: ```python activity = actor = 'epaine' assignee = 'none' closed = False closed_date = None closer = None components = ['Tkinter'] creation = creator = 'Lance Ware' dependencies = [] files = ['45536', '49459', '49472'] hgrepos = [] issue_num = 28694 keywords = ['patch'] message_count = 11.0 messages = ['280820', '280823', '280824', '281133', '281150', '281641', '376953', '376972', '377177', '377197', '377443'] nosy_count = 6.0 nosy_names = ['terry.reedy', 'markroseman', 'zach.ware', 'serhiy.storchaka', 'Lance Ware', 'epaine'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue28694' versions = ['Python 3.10'] ```

    5b6135d8-ca9d-489e-abea-0c63081b9a95 commented 7 years ago

    Tcl/Tk 8.6 now has a fontchooser command. I have developed a tkinter interface to it, similar to colorchooser/askcolor. How does one go about contributing this or proposing for future enhancement to the tkinter module suite?

    zware commented 7 years ago

    You've already done the proposing part by opening this issue ;). To contribute you work, attach it here as a patch to the tkinter sources found in Lib/tkinter/ of a cpython source checkout. You'll need to sign a contributor agreement before we can look at or accept it, the tracker should prompt you to do so when you attach your patch. The devguide is also a great resource for questions you may have while preparing your patch.

    Thanks for your interest in contributing!

    terryjreedy commented 7 years ago

    https://www.tcl.tk/man/tcl/TkCmd/fontchooser.htm I agree that this should be wrapped, as is colorchooser. There are a few details that may need to be thrashed out. It is a nuisance that the native font dialog is modal on Windows and not on Mac (and ??? on *nix?).

    5b6135d8-ca9d-489e-abea-0c63081b9a95 commented 7 years ago

    Not having any luck creating a patch. I have retrieved source, built python, put my fontchooser.py file (attached) in cpython\Lib\tkinter, done hg add and hg commit, but when I try to do hg diff it gives me nothing.

    zware commented 7 years ago

    I would recommend backing out your commit (hg rollback if you haven't pulled or otherwise changed your checkout since you made your commit), and just do 'hg diff' at the point where you would commit. In this particular case, if there are no changes other than the added file it doesn't really matter much that it's not in patch form.

    It would be nice to have some unit tests. It may not be possible to test anything but your _str_to_font and _font_to_str functions, though.

    I notice that your Fontchooser class doesn't inherit from commondialog.Dialog like colorchooser.Chooser does; is commondialog.Dialog usable for this? Or are there improvements that can be made to commondialog.Dialog to make it suitable for Fontchooser and also improve colorchooser.Chooser? Can this reuse anything from Lib/tkinter/font.py or perhaps be merged into that file to keep all the font stuff together?

    After we have some of those details ironed out, this is going to make a nice addition to tkinter!

    5b6135d8-ca9d-489e-abea-0c63081b9a95 commented 7 years ago

    Good points, Zachary.

    Regarding the commondialog.Dialog class, I think it would need to be enhanced to make sense using it with Fontchooser. It really doesn't do a whole lot, and the awful kludge in the show method (creating a Frame widget just so that some low-level operations can be performed) is repugnant to me. I think the whole thing is pretty out-of-date.

    With the Font class, perhaps some of its functionality can be used. I will need to better understand the architecture of Font and see what I can do with it.

    I think another issue that needs to be addressed is the modal/non-modal aspect of Tk fontchooser. I have access only to Windows OS, where it is modal. I have read that it is non-modal on Mac and perhaps other platforms. If that is the case, then the show method may not work properly on those other platforms. On Windows, the Tk command "tk fontchooser show" does not return control to the caller until after the user has clicked the OK or Cancel buttons. If it does return immediately on other platforms, then the show method may need do an update loop (or something better?) to keep the behavior consistent on the Python side.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    +1 I also think that the fontchooser should be wrapped. I briefly tested your wrapper, Lance, and found it worked fine on Windows but did not on my Linux setup (because it is non-modal).

    I went about implementing my own wrapper based loosely around your's (see attached) and the way I overcame this problem was to use vwait (this still works on Windows where the call is modal). vwait is good because it returns once the condition is fulfilled without the need for polling or similar.

    While I use the commondialog module in the wrapper, this is simply for its handling (in __init__) of the master (I don't think it would make sense to enhance the Dialog class for this wrapper).

    the awful kludge in the show method (creating a Frame widget just so that some low-level operations can be performed) is repugnant to me

    You're going to moan at me then for using something similar in the attached! The reason I use it is to avoid the logic found on line 77 of your wrapper and *hopefully* make the code more readable (whether I achieve this is somewhat debatable).

    I have access only to Windows OS

    Is this still the case or can you now test on a different system which is non-modal?

    Do you wish to go ahead with authoring a patch for this or would you prefer me to do it? (I would appreciate you reviewing it either way and you would still get credit in the news as I took sections from your wrapper) If everything goes well, we can hopefully get this into Python 3.10 (as this is a new feature, it won't be back-ported to 3.9 and before).

    f4315b71-14d3-4c16-bb7a-6534e5fa1d04 commented 4 years ago

    Elaine, I was just having a look at this the other day too! I agree, this is definitely worth some effort to get done.

    To be honest, I'm not a fan of the vwait solution to force the dialog to be modal. It doesn't actually make it modal (i.e. you can still do stuff in other windows). Moreover, the Mac version (which is native, so couldn't easily be made modal via Tkinter) has a very different design than on other platforms. It doesn't have "ok", "apply", "cancel" buttons or similar. It's designed to just hang around.

    When the font dialog was originally proposed for Tk (see http://tip.tcl.tk/324) they'd tried a modal API first and found it unworkable.

    In light of that, my preference for the official wrapper would be to mirror the underlying API, as per what Lance's version first tried, using a callback mechanism to return results. Most importantly, it would eliminate the assumption that there's always a way to make it modal. Documentation would be key. Mind you, there's no official docs for the other dialogs. I can guarantee it would be well-documented with an example on TkDocs anyway!

    If there aren't any strong objections, I'm able to have a go at modifying Lance's version over the next week or so. I've got Mac, Windows, and Linux environments to test everything out.

    Question... 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? Would providing an alternative implementation for Tk \< 8.6 be something that would be done in an external module, or might that be something that could/should be included in the official library?

    f4315b71-14d3-4c16-bb7a-6534e5fa1d04 commented 4 years ago

    For future reference, if anyone is wondering why the font chooser is so complicated to use in a way that makes sense across platforms, here is its current behaviour...

    From the manual:

    Windows

    X11

    macOS

    f4315b71-14d3-4c16-bb7a-6534e5fa1d04 commented 4 years ago

    I've put together the first cut of a wrapper that tries to smooth over some of the non-essential differences in implementation details across platforms, while still respecting essential platform conventions. It also works around a few bugs I discovered along the way.

    It includes the wrapper class itself, a minimal demo, a more realistic demo, and the notes about current behaviour of the underlying Tk widget on different platforms.

    Current snapshot is at https://github.com/roseman/tkdocs/blob/fontchooser/fontchooser.py

    Obviously, this borrows hugely from previous snapshots by Lance and Elisha. Thanks!

    If you get a chance to try it out, would appreciate feedback if this is on the right track or not.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    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):

    ** 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).

    chrstphrchvz commented 1 year ago

    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).

    There are no other windowing systems supported by upstream Tcl/Tk beyond those 3: 'win32', 'aqua', and 'x11'. There are no imminent efforts to add support for anything else.