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.

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

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

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

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

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

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.

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,

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.

Here is a patch with added tests.

Indices have special meaning in Tk.

INDICES
       Many of the widget commands for listboxes take one or more indices as
       arguments.  An index specifies a particular element of the listbox, in
       any of the following ways:

       number      Specifies the element as a numerical index, where 0
                   corresponds to the first element in the listbox.

       active      Indicates the element that has the location cursor.  This
                   element will be displayed as specified by -activestyle when
                   the listbox has the key‐board focus, and it is specified with
                   the activate widget command.

       anchor      Indicates the anchor point for the selection, which is set
                   with the selection anchor widget command.

       end         Indicates the end of the listbox.  For most commands this
                   refers to the last element in the listbox, but for a few
                   commands such as index and insert it refers to the element
                   just after the last one.

       @x,y        Indicates the element that covers the point in the listbox
                   window specified by x and y (in pixel coordinates).  If no
                   element covers that point, then the closest element to that
                   point is used.

       In the widget command descriptions below, arguments named index, first,
       and last always contain text indices in one of the above forms.

An argument of Scrollbar.activate() obviously is not an index. On other hand, the "element" parameter is used consistently in other methods: Spinbox.invoke() and Spinbox.selection_element().


About the set method Tk inter documentation says:

       pathName set first last
              This command is invoked by the scrollbar's associated widget to
              tell the scrollbar about the current view in the widget.  The
              command takes two arguments, each of which is a real fraction
              between 0 and 1.  The fractions describe the range of the
              document that is visible in the associated widget.  For example,
              if first is 0.2 and last is 0.4, it means that the first part of
              the document visible in the window is 20% of the way through the
              document, and the last visible part is 40% of the way through.

It would be better to use same parameter names as in Tk.

I am planning to update all Tkinter docstrings from more detailed Tk documentation in separate issue.

A Spinbox is not a Listbox. The common feature of the activate methods you listed is that the parameter is called 'index'. But I think this is a moot point. 

To the best of my knowledge, casually changing parameter names for no functional benefit is against policy. The case for doing so is much weaker than for the re method parameter mess (correct module?). The current discussion about turtle.py on pydev reinforces my impression. Please drop the idea or ask for policy clarification on pydev.

New changeset 6ae34a948cb4 by Serhiy Storchaka in branch 'default':
Issue #6167: Scrollbar.activate() now returns the name of active element if
http://hg.python.org/cpython/rev/6ae34a948cb4

New changeset f5df571bfe1d by Serhiy Storchaka in branch '2.7':
Issue #6167: Backported tests for Scrollbar.activate() and Scrollbar.set()
http://hg.python.org/cpython/rev/f5df571bfe1d

New changeset 2cac1e3f825a by Serhiy Storchaka in branch '3.4':
Issue #6167: Backported tests for Scrollbar.activate() and Scrollbar.set()
http://hg.python.org/cpython/rev/2cac1e3f825a

Committed without change activate() parameter name. I still think this parameter name is wrong.