bpo-32585: Add ttk::spinbox by alandmoore · Pull Request #5221 · python/cpython

alandmoore · 2018-01-17T17:26:26Z

Added support for ttk::spinbox in Python ttk. Fixes https://bugs.python.org/issue32585.

https://bugs.python.org/issue32585

Added support for ttk::spinbox in Python ttk.

the-knights-who-say-ni · 2018-01-17T17:26:29Z

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!

alandmoore · 2018-01-17T17:50:16Z

I have just signed the CLA, I don't know if the bot will remove that tag or if I need to.

terryjreedy

I would like to see this added unless there is a good reason for the omission. Still needed are patches for doc and tests.

terryjreedy · 2018-01-19T21:52:14Z


+class Spinbox(Entry):
+    """Ttk Spinbox is an Entry with increment and decrement arrows, used
+     for number entry"""


I have the impression that a spinbox 'returns' a string that in normally a number but could be selected from a list of strings.

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.

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.

terryjreedy · 2018-01-19T21:55:24Z

+
+        STANDARD OPTIONS
+
+            class, cursor, style, takefocus


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.

invalidcommand is another standard option. Will correct this in spinbox.

terryjreedy · 2018-01-19T22:03:00Z

+
+        WIDGET-SPECIFIC OPTIONS
+
+            to, from_, increment


Ditto adds
values, wrap, format, command
plus "all the features of the ttk::entry widget", which has a few more specific options not listed.

bedevere-bot · 2018-01-19T22:05:22Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

This enables the functionality necessary when spinbox is passed a list of values rather than a range.

csabella

At the top of ttk.py, there is a __all__ defined, which Spinbox would need to be added to.

csabella · 2018-01-19T23:29:22Z

+
+            to, from_, increment, values, wrap, format, command
+        """
+        Entry.__init__(self, master, "ttk::spinbox", **kw)


Should this be Combobox.__init__ now?

Quite right. Is there a particular reason why none of the classes in this library use super()?

I'm actually going to revert the base class to Entry per @terryjreedy 's comment.

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

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.

The current and set methods correspond the current and set subcommands of Ttk widgets. The ttk::entry widget doesn't have these commands.

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.

csabella · 2018-01-20T14:54:48Z

           "Labelframe", "LabelFrame", "Menubutton", "Notebook", "Panedwindow",
           "PanedWindow", "Progressbar", "Radiobutton", "Scale", "Scrollbar",
-           "Separator", "Sizegrip", "Style", "Treeview",
+           "Separator", "Sizegrip", "Style", "Treeview", "Spinbox",


These look alphabetized.

corrected this

alandmoore · 2018-01-20T22:32:22Z

So I've added a .current() method, following the same approach used in Combobox, since this is supposed to be a valid method on ttk::spinbox (c.f. https://www.tcl.tk/man/tcl/TkCmd/ttk_spinbox.htm )

But when I call this method Tcl/Tk says "current" is not a valid method. Anyone know what I'm doing wrong here?

 File "/home/alanm/src/cpython/Lib/tkinter/ttk.py", line 1181, in current
    return self.tk.getint(self.tk.call(self._w, "current"))
_tkinter.TclError: bad command "current": must be bbox, cget, configure, delete, get, icursor, identify, index, insert, instate, selection, state, set, validate, or xview

Currently crashing due to "current" not being a valid ttk::spinbox command, contrary to Ttk documentation.

alandmoore · 2018-01-21T02:04:18Z

I've pushed a test class, but there are two problems which I cannot figure out:

The test suite is complaining that 'exportselection', 'font', and 'foreground' are not in the OPTIONS list, but these are not valid Spinbox arguments.
As mentioned in a previous comment, trying to use Spinbox.current is throwing a TclError saying that current is not a valid method. This is contrary to the Ttk documentation.

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

$ tclsh
% package require Tk
8.6.8
% package require Ttk
8.6.8
% grid [ttk::spinbox .myspinbox]
% .myspinbox current
bad command "current": must be bbox, cget, configure, delete, get, icursor, identify, index, insert, instate, selection, state, set, validate, or xview

Granted, this is my first foray into Tcl/Tk so I may have done something wrong, but it appears that current is not a valid spinbox command. Can anyone confirm?

serhiy-storchaka

Add an entry in the What's New document and your name in Misc/ACKS.

I confirm that current is not supported.

serhiy-storchaka · 2018-01-21T06:08:10Z

 * This widget supports only "southeast" resizing.


+Spinbox


I would add the description of Spinbox right after the description of Combobox. And the same for the Spinbox class and the test class.

serhiy-storchaka · 2018-01-21T06:11:10Z

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


Too long line.

serhiy-storchaka · 2018-01-21T06:13:22Z


      * id
-         Returns the column name. This is a read-only option.
+	 Returns the column name. This is a read-only option.


Unrelated change. Don't use tabs.

Sorry, had some bad editor settings.

serhiy-storchaka · 2018-01-21T06:18:24Z

+        self.assertEqual(self.spin.get(), '1')
+
+    def test_values(self):
+


Too much empty lines.

csabella · 2018-01-21T14:15:07Z

@serhiy-storchaka I thought current was supported because of this page: https://www.tcl.tk/man/tcl/TkCmd/ttk_spinbox.htm

Sorry about that.

serhiy-storchaka · 2018-01-21T19:02:51Z

Seems there is just a documentation bug. Tk sources doesn't contain implementation of the current subcommand of the ttk::spinbox widget.

Removed from code, docs, and tests. This method is documented in Ttk docs, but does not actually exist.

terryjreedy · 2018-02-06T21:22:55Z

3.7.0b1 was released a week ago, so I believe we already missed the window for 3.7 new features.

Serhiy is the tkinter expert, so he is the one who will have to do the final review and merge.

alandmoore · 2018-02-07T16:58:15Z

I see. @serhiy-storchaka is it too late for this one in 3.7?

serhiy-storchaka

The SpinboxTest test is omitted when run the test_ttk_guionly test. After adding it into the tests_gui tuple it produces many failures.

bedevere-bot · 2018-02-07T17:34:12Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

alandmoore · 2018-02-07T19:43:49Z

@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

bedevere-bot · 2018-02-09T00:03:57Z

@serhiy-storchaka: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2018-02-09T00:03:57Z

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 <[email protected]>

bedevere-bot · 2018-02-09T00:05:08Z

GH-5592 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-02-09T11:33:12Z

Thanks @alandmoore for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

(cherry picked from commit a48e78a) Co-authored-by: Alan D Moore <[email protected]>

Add ttk::spinbox

a2a46f3

Added support for ttk::spinbox in Python ttk.

the-knights-who-say-ni added the CLA not signed label Jan 17, 2018

bedevere-bot added the awaiting review label Jan 17, 2018

Added NEWS.d/next entry

505c77d

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 18, 2018

Removed double colon that was causing a warning/error during build.

c5cbac7

terryjreedy requested a review from serhiy-storchaka January 19, 2018 21:38

terryjreedy requested changes Jan 19, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 19, 2018

alandmoore added 3 commits January 19, 2018 16:26

Add more arguments to ttk.Spinbox docstring

7b8219e

Make spinbox inherit Combobox rather than Entry

826f4e3

This enables the functionality necessary when spinbox is passed a list of values rather than a range.

Expand on usage information in Spinbox docstring

3228370

csabella reviewed Jan 19, 2018

View reviewed changes

Revert Spinbox to Entry baseclass, add set/current methods

af4e1a1

csabella reviewed Jan 20, 2018

View reviewed changes

alandmoore added 2 commits January 20, 2018 19:11

Keep __all__ alphabetzied

16a5b98

Test class for ttk.Spinbox

a56a970

Currently crashing due to "current" not being a valid ttk::spinbox command, contrary to Ttk documentation.

Documentation for ttk.Spinbox

1a3b7c1

serhiy-storchaka reviewed Jan 21, 2018

View reviewed changes

Remove nonexistent "current" method

a933457

Removed from code, docs, and tests. This method is documented in Ttk docs, but does not actually exist.

serhiy-storchaka requested changes Feb 7, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Feb 7, 2018

add SpinboxTest to test_gui tuple

2df569d

serhiy-storchaka and others added 4 commits February 7, 2018 23:04

Fix tests on HiDPI displays.

8387517

Add missing options, update_idletasks to Spinbox tests

1ef7ca0

Also remove redundant pack() and wait_visibility() calls

Merge branch 'fix-issue-32585' of https://github.com/alandmoore/cpython…

bf8bd5c

… into fix-issue-32585

add update() to spinbox tests to ensure callbacks are called

92ecb6c

serhiy-storchaka approved these changes Feb 8, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 8, 2018

Use update() after setting a command in the test.

53046dc

serhiy-storchaka added the needs backport to 3.7 label Feb 9, 2018

serhiy-storchaka merged commit a48e78a into python:master Feb 9, 2018

bedevere-bot removed the awaiting merge label Feb 9, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 9, 2018

bpo-32585: Add tkinter.ttk.Spinbox. (pythonGH-5221)

b072e72

(cherry picked from commit a48e78a) Co-authored-by: Alan D Moore <[email protected]>

bedevere-bot removed the needs backport to 3.7 label Feb 9, 2018

serhiy-storchaka added type-feature A feature request or enhancement needs backport to 3.7 labels Feb 9, 2018

serhiy-storchaka removed the needs backport to 3.7 label Feb 9, 2018

serhiy-storchaka pushed a commit that referenced this pull request Feb 9, 2018

bpo-32585: Add tkinter.ttk.Spinbox. (GH-5221) (GH-5592)

105fcbf

(cherry picked from commit a48e78a) Co-authored-by: Alan D Moore <[email protected]>

alandmoore deleted the fix-issue-32585 branch February 20, 2018 22:24

		self.assertEqual(self.spin.get(), '1')

		def test_values(self):

Uh oh!

Conversation

alandmoore commented Jan 17, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jan 17, 2018

Uh oh!

alandmoore commented Jan 17, 2018

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 19, 2018

Uh oh!

csabella left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alandmoore commented Jan 20, 2018

Uh oh!

alandmoore commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Jan 21, 2018

Uh oh!

terryjreedy commented Feb 6, 2018

Uh oh!

alandmoore commented Feb 7, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 7, 2018

Uh oh!

alandmoore commented Jan 17, 2018 •

edited by bedevere-bot

Loading

csabella left a comment •

edited

Loading

alandmoore commented Jan 21, 2018 •

edited

Loading

csabella commented Jan 21, 2018 •

edited

Loading