bpo-25237: Documentation for tkinter modules #1870
Conversation
|
@NP123, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @ezio-melotti and @berkerpeksag to be potential reviewers. |
|
Thanks for the critique; my apologies for the spelling and grammatical errors. I am working towards revising and improving it. |
|
|
||
| The *askcolor* method is a factory method that creates a color choosing | ||
| dialog. | ||
|
|
There was a problem hiding this comment.
Should add that this call will wait for the user to make a selection, and then will return the color selected (or None) to the caller
Doc/library/tkinter.filedialog.rst
Outdated
|
|
||
| .. seealso:: | ||
|
|
||
| :mod:`tkinter.commondialog`, :ref:`tut-files` |
There was a problem hiding this comment.
I would suggest putting the ask* functions first, since those are the preferred interface. For each, mention that these are modal dialogs, also that return value may be None.
As for Directory()/FileDialog() and friends, I'd rather see these after the interfaces that people use, and should explicitly say that they create these dialogs from scratch and they don't resemble the platform ones. (They should also probably be deprecated, but that's a separate issue)
|
|
||
| The :mod:`tkinter.font` module provides the :class:`Font` class for creating | ||
| and using named fonts. | ||
|
|
There was a problem hiding this comment.
The whole idea of "named fonts" is a bit of a cognitive mismatch with font objects, i.e. from the Python point of view you just want a font object, and it being a "named" font doesn't make a lot of sense. (In Tk, the string name of the font plays the same role as an object reference). Could you add something like "Named fonts are Tk's way of defining a font as a single object, rather than specifying the font each time as a list of font attributes."?
There was a problem hiding this comment.
I agree with this addition.
| askokcancel(title=None, message=None, **options) | ||
| askretrycancel(title=None, message=None, **options) | ||
| askyesno(title=None, message=None, **options) | ||
| askyesnocancel(title=None, message=None, **options) |
There was a problem hiding this comment.
Again, just to mention that these are all modal so the calls complete after the user dismisses the dialogs. Return values vary with the type of message box created, which will be some subset of ok, cancel, yes, no, retry, abort, ignore.
In the synopsis I would probably refer to these as "alert" dialogs, which is the more familiar name.
There was a problem hiding this comment.
I suggested a synopsis change. The returns for each functions should be specified. The last four can be covered with
Return one of the alternatives in the function name after display a box.
What does askquestion return? An answer the user enters? If so, the synopsis needs a further change.
|
@roseman thanks for all the feedback! |
|
Thanks for the review @roseman and for the persistence on the PR @NP123. When you are both happy with the PR, please ping me for core review. Thanks. cc/ @serhiy-storchaka FYI as tkinter expert |
|
Looks good to me! @willingc |
|
|
||
| The :mod:`tkinter.font` module provides the :class:`Font` class for creating | ||
| and using named fonts. | ||
|
|
There was a problem hiding this comment.
I agree with this addition.
| askokcancel(title=None, message=None, **options) | ||
| askretrycancel(title=None, message=None, **options) | ||
| askyesno(title=None, message=None, **options) | ||
| askyesnocancel(title=None, message=None, **options) |
There was a problem hiding this comment.
I suggested a synopsis change. The returns for each functions should be specified. The last four can be covered with
Return one of the alternatives in the function name after display a box.
What does askquestion return? An answer the user enters? If so, the synopsis needs a further change.
| @@ -33,14 +33,19 @@ alternatives, see the :ref:`other-gui-packages` section. | |||
| .. toctree:: | |||
|
|
|||
| tkinter.rst | |||
| tkinter.colorchooser.rst | |||
There was a problem hiding this comment.
I think it would be better to merge the documentation for all dialog related modules into a single page.
There was a problem hiding this comment.
By these do you mean {commondialog, dialog, simpledialog} only? I feel the filedialog and colorchooser modules each have distinct enough functionality and usage that it would be cleaner to keep them separate.
| Freezing Tkinter applications | ||
|
|
||
|
|
||
| Freezing Tkinter applications |
There was a problem hiding this comment.
3 trailing blank lines that were present in the existing version were removed for consistent formatting. They caused a warning\error in the CI build, being flagged as trailing whitespace.
|
I have made the requested changes; please review again |
1 similar comment
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
There was a problem hiding this comment.
Hi @patel-nikhil! Thanks for this huge piece of documentation. I marked as resolved all previous reviews that you successfully resolved, but ther's still some opened reviews. I also had to replicate some (half-fixed ones typically).
Can you take a look at it?
Doc/library/dialog.rst
Outdated
| **Static factory functions** | ||
|
|
||
| The below functions when called create a modal, native look-and-feel dialog, | ||
| wait for the user's selection, then return the selected value(s) or None to the |
There was a problem hiding this comment.
| wait for the user's selection, then return the selected value(s) or None to the | |
| wait for the user's selection, then return the selected value(s) or `None` to the |
There was a problem hiding this comment.
Am I correct in assuming you mean ``None`` instead of `None` since make check gives me 1 error with severity 2 (reason: default role used) when using single backquotes to surround None
There was a problem hiding this comment.
Yes I meant None, thanks for proofreading my proofreading 👍
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again. @JulienPalard thanks for your efforts! I've read through the remaining previously open reviews, and the changes you highlighted in your comments, and have addressed them with my latest commit. |
|
Thanks for making the requested changes! @terryjreedy, @JulienPalard: please review the changes made to this pull request. |
|
I leave it a final review to Julien |
|
@patel-nikhil thanks for this huge piece of documentation! |
| @@ -0,0 +1,230 @@ | |||
| Tkinter Dialogs | |||
There was a problem hiding this comment.
A bit late to the party, but may I suggest that this file be renamed to tkinter.dialog.rst?
Added documentation for tkinter modules where none existed
https://bugs.python.org/issue25237