Tkinter.Scrollbar: the activate method needs to return a value, and set should take only two args

[gpolo]

BPO	6167
Nosy	@terryjreedy, @asvetlov, @JimJJewett, @serhiy-storchaka
Files	Scrollbar_fixes.diff Scrollbar_fixes_2.diff tkinter_Scrollbar_fixes_3.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:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2014-07-23.19:23:52.507>
created_at = <Date 2009-06-01.20:11:31.614>
labels = ['type-feature', 'expert-tkinter']
title = 'Tkinter.Scrollbar: the activate method needs to return a value, and set should take only two args'
updated_at = <Date 2014-07-23.19:23:52.506>
user = 'https://bugs.python.org/gpolo'

bugs.python.org fields:

activity = <Date 2014-07-23.19:23:52.506>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2014-07-23.19:23:52.507>
closer = 'serhiy.storchaka'
components = ['Tkinter']
creation = <Date 2009-06-01.20:11:31.614>
creator = 'gpolo'
dependencies = []
files = ['14147', '32477', '35461']
hgrepos = []
issue_num = 6167
keywords = ['patch']
message_count = 14.0
messages = ['88670', '112865', '157707', '180037', '201484', '201487', '219598', '219621', '219626', '219629', '219638', '219645', '223766', '223767']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'gpolo', 'asvetlov', 'python-dev', 'Jim.Jewett', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue6167'
versions = ['Python 3.5']

[gpolo]

Hi,

I've noticed some minor problems in Tkinter.Scrollbar that would be good
to be addressed. The activate method never returns a value and it also
doesn't accept to be called without an element -- which is accepted by
tcl. When an element is not especified, the current active element
should be returned. It's signature is also a bit strange, I don't see
why it has a parameter named "index" while it never takes an index but
an element.

The second problem is about the set method. It accepts any amount of
arguments, but it only needs to accept two. Passing a tuple in the form
of (number, number) to it isn't accepted, so I don't see much reason to
continue with an *args there.

[BreamoreBoy]

A tiny patch, can someone take a look with a view to committing, thanks.

[jimjjewett]

(1) Why did you change the name of the parameter from index to element? If the underlying engine also accepts indices (e.g., self.activate(4) ) then the name should stay. If it really is only meaningful to use the name of an element -- maybe it should still stay for backwards compatibility. Or at least accept the old name too for a release.

Either way, please provide a test case showing that it works under the new name; there may also be doc fixes. (I'm not sure there is documentation for this widget, though, and providing some in the first place is good, but perhaps a different task.)

FWIW,
http://docs.python.org/dev/library/tkinter.html#the-index-parameter
suggests that the name should stay index, and can be far more than an element; migrating some of that to the docstring might be helpful.

(2) It looks like the set command took *args to give some freedom. There may be extensions that take a third argument. It may well be valid to call it without any arguments, or with only one. So the signature may turn into set(first=0, last=1, *args) or some such. Whatever the answer about what arguments are really needed, there should be test cases to demonstrate this before the API is changed.

[serhiy-storchaka]

In versions of Tk before 4.0, the set command accepted 4 arguments.

I think this is a new feature and can be applied only to 3.4. Agree with Jim that for backward compatibility we should keep name "index" and arbitrary number of arguments at least for one release (with deprecation warnings etc).

[serhiy-storchaka]

I withdraw my comment about arbitrary number of arguments. Tk itself raises an exception if the number of arguments is wrong.

[serhiy-storchaka]

The patch updated to tip and added deprecation warning for the "index" keyword argument.

[jimjjewett]

I'm still not seeing why these changes are sufficiently desirable to justify the code churn. Nor am I seeing test or doc changes that would explain the advantages of the new way, and prevent future regressions.

I agree that the changes would make the signatures better for the typical use cases for this particular widget -- but wouldn't they also break the common interface for the "set" and "activate" methods across several types of tkinter widget?

If so, then instead of changing or restricting the method, it would be better to add examples (and maybe even an explanation) to the documentation (including the docstring).

In particular:

(1) Why change actrivate's parameter from "index" to "element"? I agree that "element" is a better name for the normal case, but
https://docs.python.org/dev/library/tkinter.html#the-index-parameter
strongly suggests that "index" is more consistent with the rest of tkinter, and that there are use cases wehre "index" is the right name. If that is not true, please say so explicitly, at least in comments.

(2) Why change the "set" method? I understand that a more specific signature is desirable, and I assume that other values would be ignored (or raise an exception), but the set method seems to have an existing API across several widgets -- and that shouldn't be broken lightly.

[terryjreedy]

1. In activate, change parameter 'index' to 'element'. I agree with Jim about rejecting this. A (specific). 'index' is routinely used to point to an item in a sequence; "arrow1", "slider", and "arrow2" are visually sequenced. The doc string is clear on the possible indexes Text also uses words for indexes. B (general). we don't break code by renaming arguments; I am pretty sure that any exception one might raise does not apply to this issue.

2. Give index a default of None and return the result of calling tk with None, instead of tossing it. I believe this enhancement would make activate more consistent with other methods. If so, do it -- with an added test.

3. Give .set() specific parameters. I think the current docstring is a bit confusing and should be revised. Am I correct in thinking that on a vertical slider, the upper end get the lower value, whereas the lower end gets the higher value? And that one should call bar.set(.3, .6) rather than bar.set(.6, .3)? If so, calling the parameters 'lowval' and 'hival' might be clearer.

Does msg201484 mean that tk requires exactly 2 args? If so, some change seems ok. Deleting 'args' cannot in itself break code as 'args' cannot be used as a keyword. I agree with not adding defaults,

[serhiy-storchaka]

There is no the common interface for the "set" and "activate" methods.

Listbox.activate(index) - mandatory argument is an index (an integer, "active", "anchor", "end", or @x,y form).
Menu.activate(index) - mandatory argument is an index.
Scrollbar.activate(element=None) - optional argument is element identifier, one of "arrow1", "slider" or "arrow2".
Listbox.selection_set(self, first, last=None) - arguments are indices, first argument is mandatory.
Scale.set(value) - mandatory argument is a number between specified limits.
Scrollbar.set(first, last) - mandatory arguments are numbers between 0 and 1.

[serhiy-storchaka]

Here is a patch with added tests.