-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-32585: Add ttk::spinbox #5221
Conversation
Added support for ttk::spinbox in Python ttk.
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
I have just signed the CLA, I don't know if the bot will remove that tag or if I need to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see this added unless there is a good reason for the omission. Still needed are patches for doc and tests.
Lib/tkinter/ttk.py
Outdated
@@ -1151,6 +1151,24 @@ def __init__(self, master=None, **kw): | |||
Widget.__init__(self, master, "ttk::sizegrip", kw) | |||
|
|||
|
|||
class Spinbox(Entry): | |||
"""Ttk Spinbox is an Entry with increment and decrement arrows, used | |||
for number entry""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the impression that a spinbox 'returns' a string that in normally a number but could be selected from a list of strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I'd forgotten it can take a list of values, just like a combobox. I think the relevant methods are already in ttk.combobox
, would it make sense to inherit from that class or would it be better to reimplement them here? It's not much code, but I don't think there's anything unique to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the tk docs say that ttk spinbox is a ttk entry with added arrow buttons, I would think that Entry rather than Combobox is the correct baseclass.
Lib/tkinter/ttk.py
Outdated
|
||
STANDARD OPTIONS | ||
|
||
class, cursor, style, takefocus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.tcl.tk/man/tcl8.6/TkCmd/ttk_spinbox.htm adds
validate, validatecommand, xscrollcommand
although validate and validatecommand are listed as widget-specific for ttk.Entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalidcommand is another standard option. Will correct this in spinbox.
Lib/tkinter/ttk.py
Outdated
|
||
WIDGET-SPECIFIC OPTIONS | ||
|
||
to, from_, increment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto adds
values, wrap, format, command
plus "all the features of the ttk::entry widget", which has a few more specific options not listed.
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 |
This enables the functionality necessary when spinbox is passed a list of values rather than a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the top of ttk.py, there is a __all__
defined, which Spinbox would need to be added to.
|
||
to, from_, increment, values, wrap, format, command | ||
""" | ||
Entry.__init__(self, master, "ttk::spinbox", **kw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Combobox.__init__
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite right. Is there a particular reason why none of the classes in this library use super()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually going to revert the base class to Entry per @terryjreedy 's comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure super came years later than the original Tkinter module. In 2.x it was a bit harder to use. For single inheritance, I don't think that there is much advantage. (I believe that there are cases of multiple inheritance in tkinter.init, but have not rechecked.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should have a base class of Entry, but with that, I also think current
and set
from Combobox should be included here. It would probably be overkill, but maybe Spinbox and Combobox should both inherit from an Entry subclass that has current
and set
?
The tests will be able to use current
and set
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current
and set
methods correspond the current
and set
subcommands of Ttk widgets. The ttk::entry widget doesn't have these commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant that both Spinbox and Combobox have current
and set
. I didn't know if it was better to implement them both directly from Entry or to have another class in between, that is a subclass of Entry that implements current and set and is the superclass to Combobox and Spinbox.
Lib/tkinter/ttk.py
Outdated
@@ -19,7 +19,7 @@ | |||
__all__ = ["Button", "Checkbutton", "Combobox", "Entry", "Frame", "Label", | |||
"Labelframe", "LabelFrame", "Menubutton", "Notebook", "Panedwindow", | |||
"PanedWindow", "Progressbar", "Radiobutton", "Scale", "Scrollbar", | |||
"Separator", "Sizegrip", "Style", "Treeview", | |||
"Separator", "Sizegrip", "Style", "Treeview", "Spinbox", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look alphabetized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected this
So I've added a But when I call this method Tcl/Tk says "current" is not a valid method. Anyone know what I'm doing wrong here?
|
Currently crashing due to "current" not being a valid ttk::spinbox command, contrary to Ttk documentation.
I've pushed a test class, but there are two problems which I cannot figure out:
If anyone can take a look and help me figure out what's going on here, I would be grateful. EDIT: I can only conclude that the Ttk docs are wrong. I tried this in
Granted, this is my first foray into Tcl/Tk so I may have done something wrong, but it appears that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an entry in the What's New document and your name in Misc/ACKS.
I confirm that current
is not supported.
Doc/library/tkinter.ttk.rst
Outdated
@@ -692,6 +692,92 @@ Bugs | |||
* This widget supports only "southeast" resizing. | |||
|
|||
|
|||
Spinbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the description of Spinbox right after the description of Combobox. And the same for the Spinbox class and the test class.
Doc/library/tkinter.ttk.rst
Outdated
Virtual events | ||
^^^^^^^^^^^^^^ | ||
|
||
The spinbox widget generates a **<<Increment>>** virtual event when the user presses <Up>, and a **<<Decrement>>** virtual event when the user presses <Down>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too long line.
Doc/library/tkinter.ttk.rst
Outdated
@@ -908,19 +994,19 @@ ttk.Treeview | |||
The valid options/values are: | |||
|
|||
* id | |||
Returns the column name. This is a read-only option. | |||
Returns the column name. This is a read-only option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change. Don't use tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, had some bad editor settings.
self.assertEqual(self.spin.get(), '1') | ||
|
||
def test_values(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much empty lines.
@serhiy-storchaka I thought Sorry about that. |
Seems there is just a documentation bug. Tk sources doesn't contain implementation of the |
Removed from code, docs, and tests. This method is documented in Ttk docs, but does not actually exist.
I see. @serhiy-storchaka is it too late for this one in 3.7? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SpinboxTest test is omitted when run the test_ttk_guionly test. After adding it into the tests_gui tuple it produces many failures.
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 |
@serhiy-storchaka Sorry, was not aware of that tuple. I have added to the tuple, but I don't get any failing tests on my system. I notice that appveyor has 1 fail, and that it's executing on Windows (I'm on Linux). This suggests to me that the problem is either platform-related or a race condition. If tests are failing on your system, I would appreciate the error output, since I cannot reproduce. |
Also remove redundant pack() and wait_visibility() calls
… into fix-issue-32585
@serhiy-storchaka: Please replace |
Thanks @alandmoore for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
(cherry picked from commit a48e78a) Co-authored-by: Alan D Moore <me@alandmoore.com>
GH-5592 is a backport of this pull request to the 3.7 branch. |
Thanks @alandmoore for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
Added support for ttk::spinbox in Python ttk. Fixes https://bugs.python.org/issue32585.
https://bugs.python.org/issue32585