PR Details:

• Widget name: Notebook
• Author: @j4321, with modifications by @Dogeek

Description

A Notebook widget implementation with functionality to reorder, close and detach tabs. In addition, when there are too many tabs, buttons appear to navigate through them. A Menu is also available to allow quick jumping to a specific tab.

Checklist

• [x] Widget in a separate file in the appropriate folder
• [x] Widget functions properly on both Windows and Linux
• [x] Widget code includes docstrings with parameter descriptions
• [x] Included an example file in /examples
• [x] Widget is covered by unitttests in /tests
• [x] Widget includes required assets files
• [x] Reference to widget in AUTHORS.md
• [ ] Entry in sphinx documentation

This PR contains much the same changes as #43 , but rebased onto master, changing commit messages and squashing commits into larger ones. This branch contains only changes required for the widget and no other commits.

Codecov Report

Merging #48 into master will decrease coverage by 3.94%. The diff coverage is 51.12%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   89.76%   85.82%   -3.95%     
==========================================
  Files          36       59      +23     
  Lines        3751     6312    +2561     
==========================================
+ Hits         3367     5417    +2050     
- Misses        384      895     +511     

Impacted Files	Coverage Δ
ttkwidgets/notebook.py	28.83% <28.83%> (ø)
ttkwidgets/utilities.py	95.94% <95.58%> (-4.06%)	:arrow_down:
tests/test_utilities.py	98.36% <98.36%> (ø)
tests/test_notebook.py	100.00% <100.00%> (ø)
ttkwidgets/__init__.py	100.00% <100.00%> (ø)
ttkwidgets/font/sizedropdown.py	80.00% <0.00%> (-14.45%)	:arrow_down:
ttkwidgets/font/familylistbox.py	82.75% <0.00%> (-13.08%)	:arrow_down:
ttkwidgets/font/familydropdown.py	64.00% <0.00%> (-11.00%)	:arrow_down:
ttkwidgets/scrolledlistbox.py	85.18% <0.00%> (-10.47%)	:arrow_down:
ttkwidgets/frames/toggledframe.py	90.32% <0.00%> (-9.68%)	:arrow_down:
... and 46 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7dc4bf6...7dc4bf6. Read the comment docs.

TODO List:

• [x] Extend the tests to properly cover the functionality of the widget
• [x] Test the widget on Windows
• [x] Test the widget on Linux
• [ ] Create an entry in the sphinx documentation

Thank you for these commits, @Dogeek . I have cherry-picked them, which yields an easier and cleaner solution than merging into this branch. In the future, just mention the branch your commits are on, then I can continue cherry-picking them.

The CI is now failing because of issues with copying the widgets. If no value is given for an option, the default option (an empty value) appears to not always be valid. This will require some investigation.

Also, I would like to test the bindings in more detail, verifying that the actual functions are still bound. I will work on it when I can.

@RedFantom I've fixed the copy_widget and move_widget functions in the notebook-review-fixes branch

Thank you for taking a look at it! When I cherry-picked your commit and tested the solution against the unit tests, the issue still occurred however. I believe this is because the instantiation of an empty widget still leaves the options unspecified and rather than returning the defaults, cget will return an empty value.

These empty values must be filtered out. This does mean that if a widget is instantiated using an option that is an empty string, this will fail. However, I do not see any other option to do this properly.

The new issue is unrelated to the previous one. This one has to do with the name of the widget. As the widget is newly created, the names do not necessarily match. The equalness of the widgets must be verified differently.

@RedFantom Added the fix with runtime error (same branch as before) and some more tests for utilities functions cause why not

forgot to update the docstring for copy_widget (to signal possible raise of RuntimeError), docstring of move_widget could be updated as well

I checked your commit for the get_widget_options thing, and my implementation is way better (dict comprehensions are about 10X faster than dict mutations, and I actually check if the option is at its default value instead of if the option is '' or None)

I checked your commit for the get_widget_options thing, and my implementation is way better (dict comprehensions are about 10X faster than dict mutations, and I actually check if the option is at its default value instead of if the option is '' or None)

While a comprehension may be faster, the solution you provide actually does not work, as the default values of the widget options are also empty. I checked by cherry-picking your commit and running the tests against it. My implementation might still be rewritten as a comprehension though.

@RedFantom I've tested it myself, and it does work. it returns a dictionary of all the diverging keys from the default values. That means that if you do this :

import tkinter as tk
root = tk.Tk()
lbl = tk.Label(root, takefocus=True)
print(get_widget_options(lbl))
# {'takefocus': 1}

you can see it works. If all the values are the default, it will return an empty dict, which is also fine.

pushed a bunch of fixes to notebook-review-fixes

My apologies, I must have cherry-picked and then merged the changes wrongly, as you are correct: Your implementation does indeed work. However, I decided to profile the function performance, and the version with dictionary modifications is significantly faster than the one with the dictionary comprehension. Even after taking out duplicate function calls and changing it so that the base widget object is only created once, the function with the 'normal' for loop yields a running time of 0ms (the profiler doesn't measure smaller intervals) and for the dictionary comprehension I got it down to 2ms.

I ran the test multiple times with multiple variations, but it was always slower. This is an interesting result that I had actually not expected. Perhaps the slow down is due to the added if-statement in the comprehension, I'm not sure.

Regardless, your implementation does work, but the one on the branch right now is a tad bit faster.

Interesting. Though speed is not the only thing that matters in this case. I implemented it that way to take full advantage of Python's capabilities. My implementation is better in that regard, if the tkinter API ever changes, with the defaults changing, it's much better to do it my way.

Regardless, dict comprehensions are actually faster anyways, the only reason my implementation is slower is because there's 2 comprehensions, filtering 2 times. A single comprehension like :

return {key: widget.cget(key) for key in widget.keys() if widget.cget(key)}

is surely faster than dict mutating.

Also did you test with a widget with a bunch of options changed ? The more the options, the greater the gains of using a comprehension over mutating the dict

The tests added in commit a69f41f illustrate my concerns about the move_widget function, and why I thought more testing was needed. Due to the way that commands and bound functions are referenced and executed in Tkinter, I thought that this would yield significant problems. It does indeed yield these problems. The only way I can see to circumvent the problem of function referencing in Tkinter is to somehow get the object instances of the bound functions and rebind them after the move is done. This is a critical thing: This way, when a widget is moved, then any bindings or commands on it are lost. This would mean that Buttons also lose their functionality.

Of course, the widget can be made to not have the functionality of moving tabs to a new parent and in this way be merged.

The problem is illustrated by following code snippet:

>>> import tkinter as tk
>>> window = tk.Tk()
>>> button = tk.Button(window, command=window.destroy
>>> button.cget("command")
'140439709217672destroy'

I cannot cherry-pick all of your changes, @Dogeek . The branches have diverged too far and it would basically mean using copy-paste on all the changed lines would be faster than using git, which renders the whole point of using git moot. Could you make your changes on a branch that stems from this one or is rebased onto the notebook branch please? That would also remove the need for me to force-push and make the divergence even worse.

The problem is that I don't know how to grab the reference to the functions.

The only solution I can see right now would be to hook into the ttk::Widget and tkinter::Widget classes to append the function references to a list.

It should be that 'tk::Widget().bind(key)' returns the contents of the script but it doesn't.

@j4321 do you have any insight on this ?

I thought I'd take a look at the Tkinter source code to figure out how the functions are called from the Tcl-interpreter. My research has so far lead to the following, for the Python 3.6 source tree:

• The bind() function is defined in the Misc class in __init__.py.
• The bind() function calls the _bind() function on the same object.
• When the object given to bind to an event is actually a callable thing, it is registered using an ID with the function _register
• The _register function creates a new Tcl command (the text references that keep popping up in the errors) that calls the Python function using the CallWrapper class, which basically restores arguments and catches errors.
• The wrapped __call__ is passed to a function _tk.createcommand(cbname, f), where cbname is the name that is returned by the bind function and the thing that keeps appearing.

This is where it gets... Complicated with the _tkinter.c module. Let's continue.

• I believe the function called is _tkinter_tkapp_createcommand_impl which matches the required signature. This function actually creates a function using the Tcl_CreateCommand function.
• The Tcl_CreateCommand is a function included from tcl.h and is not part of the Python source code.
• The function passed to Tcl_CreateCommand is PythonCmd, together with a PythonCmd_ClientData, which contains the Python function, and other arguments.
• PythonCmd unpacks the function and actually calls it with its arguments. Keep in mind that this is a CallWrapper.__call__, which unpacks the arguments and calls the function from Python.
• Critical is the destroy function of the Misc function. When this function is called, it also deletes all commands that were part of the widget and for which the needcleanup kwarg of _register was set to True, or 1 by default.

Conclusion

My conclusion is that once a binding is created, it can no longer be retrieved as a Python function, as it only still exists in the memory of the Python interpreter but there are no direct references to it from the Python. The Python interpreter simply keeps the memory as there is still a non-zero reference count to the created objects and the Tcl-interpreter manages its own memory, in which the binding is stored.

There are two ways I can see to change this behaviour:

• Override the Misc.bind function so that a separate reference is kept to the functions as well as the events they are bound to and by what arguments, so that they may be retrieved in a different format and rebound exactly as they were.
• Write a C-module extension that can take a Tcl-interpreter and so move the widget from there, also fetching commands and rebinding them. This would be a lot of work and pretty complex, if it is even possible at all to hook into Tkinter in such a way as none of its objects are declared in the header.

Note that I have not yet researched how the command kwarg for i.e. Buttons is handled. I suspect these use the _register function directly, and moving them would require hooking into that function also.

I found out that binding through that return value actually works for some reason. Like :

import tkinter as tk


def callback(ev):
    print('Entered ' + str(ev.widget))


root = tk.Tk()
root.title('root')
frame = tk.Frame(root, width=300, height=300)
frame.bind('<Enter>', callback)
frame.pack()
print(frame.bind())  # ('<Enter>', )
print(frame.bind('<Enter>'))  # 'if {"[64351688callback %# %b %f %h %k %s %t %w %x %y %A %E %K %N %W %T %X %Y %D]" == "break"} break\n'
tl = tk.Toplevel(root)
tl.title('tl')
other = tk.Frame(tl, width=300, height=300)
other.bind('<Enter>', frame.bind('<Enter>'))
other.pack()
root.mainloop()

The callback is still properly called, even though a string is passed as the callback argument. So I don't know why there is an error here

Ah, of course. As you are invoking the bind function with string argument, that argument is directly regarded as a Tcl command and thus bound to that. I hadn't thought of that. That would fix the whole problem, as long as the bound function then has its reference count increased in the Tcl-interpreter and the Python-interpreter. This is something that has to be tested, but it could yield a viable solution as well.

So then the idea is to setup the bindings of the new widget to the string representations of the old widget's bindings. I'll try building an implementation.

The solution for the bindings is actually simpler still. As the commands and their wrappers as far as I know do not specifically depend on the widgets that they are created for, they may be preserved by not destroying them when the widget is destroyed. The easiest way to do this is to make sure that the Python Misc.destroy function doesn't know about them. This way, they may still be invoked by the new widget and it does not create a memory leak as when the functions are unbound from the new widget or the new widget is destroyed, the wrappers are still destroyed.

Next up: Commands and the place geometry manager. I don't think I'll have any more time this weekend though.

The place geometry manager is actually handled already.

The widgets are copied as is, with all their options. The command is actually behaving in the same way as the bind callbacks. I suppose every tkinter callback works that way (like after, bind, bind_all, command etc).

>>> import tkinter as tk
>>> def cb():
...     print('callback')
...
>>> but = tk.Button(command=cb, text='ok')
>>> but.pack()
>>> callback

>>> but['command']
<string object: '44558152cb'>
>>> dir(but['command'])
['__add__', '__class__', '__contains__', '__delattr__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getitem__', '__getnewargs__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__iter__', '__le__', '__len__', '__lt__', '__mod__', '__mul__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__rmod__', '__rmul__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', 'capitalize', 'casefold', 'center', 'count', 'encode', 'endswith', 'expandtabs', 'find', 'format', 'format_map', 'index', 'isalnum', 'isalpha', 'isascii', 'isdecimal', 'isdigit', 'isidentifier', 'islower', 'isnumeric', 'isprintable', 'isspace', 'istitle', 'isupper', 'join', 'ljust', 'lower', 'lstrip', 'maketrans', 'partition', 'replace', 'rfind', 'rindex', 'rjust', 'rpartition', 'rsplit', 'rstrip', 'split', 'splitlines', 'startswith', 'strip', 'swapcase', 'title', 'translate', 'upper', 'zfill']
>>> but2 = tk.Button(command=but['command'], text='2')
>>> but2.pack()
>>> callback

>>> but2['command']
<string object: '44558152cb'>
>>>

While it does appear to work the same way, the created Tcl function is not destroyed using the same function. If I run the following snippet, it still yields an error as the command callback has been destroyed.

>>> import tkinter as tk
>>> def cb():
...     print("callback")
...
>>> but = tk.Button(command=cb, text="1")
>>> but2 = tk.Button(command=but.cget("command"), text="2")
>>> but.destroy()
>>> but2.invoke()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.6/tkinter/__init__.py", line 2388, in invoke
    return self.tk.call(self._w, 'invoke')
_tkinter.TclError: invalid command name "140345702173384cb"

This is also what the CI results show, on both Windows and Linux. So what must be identified is where the Tcl-command is destroyed for Buttons. This lies again in the Tkinter source code, and I will venture into it when I have the time to do so.

As for the place-geometry manager, an error was still occurring on the CI, so I thought I'd take a look at the tests. I extended them and discovered the issue with the place geometry manager. When the place() function is called with no arguments, it actually doesn't do anything. It doesn't place the widget in the parent window and it does not add it to the place_slaves() result. I added positional arguments, which fixed the problem.

I also implemented a preserve_geometry keyword argument, set to False by default, which makes the function automatically also position the new widget into its parent using the same geometry information as it was in the old parent with.

Last but not least, I considered the possibility that a widget might not be configured using a geometry manager at all, and implemented a detection for that. It does require catching an error, which is slow, but the whole function is already slow anyway. The behaviour of pack_info is just not consistent with grid_info and place_info...

@Dogeek : Please let me know whether you agree with the changes I made to the copy_widget and move_widget functions.

Great job!

For the bind callback issue, I don't see a solution really, besides not destroying the widget, and using pack/grid/place_forget to just not show the widget which is not really a proper solution either because it would cause a memory leak if a widget is moved back and forth. Hooking into the bind function seems to be the only way to solve the issue. Then again, I hoped we'd find another solution, since hooks needs to be installed, and I hoped to not have to do that for obvious reasons (one being compatibility with other libraries, and the other being performance issues, even if the copy is slow in of itself, every millisecond matters)

I wholeheartedly agree with those changes. A couple of notes though ;

• You made the code python 2 compatible, and since we're dropping support for it, there's really no need

• You're importing TclError from tkinter, but the actual class in defined in the _tkinter module, which is the C extension module. It's closer to the source so it should be a tiny bit faster, with no downsides really.

Also, in the docstrings, it's very minor, but the type is tkinter.Widget not tk.Widget It doesn't make much of a difference, but tk is an alias to tkinter, it would be better for the docs to refer to the actual module name, in my opinion.

So, it turns out that the command kwarg value does indeed also get passed to the _register function through wrappers in the Misc-class. However, I had put the _tclCommands = None in a place so that it would only be executed when a binding is present. By moving this so that it is always executed, no commands are destroyed and thus now the command kwarg does work.

I'd rather keep referring as tk.Widget, as the tkinter module also uses that abbreviation for itself. Also, accessing a protected module such as _tkinter is bad practice, so I'd rather avoid and take the hit of accessing an additional pointer.

Now the only thing left to do here, I believe, is extend the tests of the Notebook-widget itself.

What are the tests that you have in mind for the Notebook widget ? AFAIK there's not that much stuff to test for. If you provide a list, I can write those tests.

Also, do you have a better name for the bbox_is_x1y1x2y2 keyword argument in the coords_in_box() function ? Couldn't find anything better when I wrote it but it's quite the mouthful.

The name of the keyword argument bbox_is_x1y1x2y2 is indeed a bit unfortunate, but even after giving it some thought, I haven't been able to think of a good replacement name. You could use x1y2x2y2, but that would be pretty unclear.

Most if not all of the functionality of the widget should be tested, insofar that is possible. So that would include:

• Adding tabs
• Removing tabs
• Moving/Dragging tabs
• The functions insert and index
• Adding too many tabs so the buttons appear
• The functionality of the buttons
• Detaching a tab
• Forgetting a tab
• Changing the options of an existing tab

Pretty much all the public functions at least. Some functionality such as dragging the tabs and such will be a bit complicated, so maybe not everything can be tested. The goal is to do as much as possible though.

Okay, I'll start writing tests for these this weekend, or as soon as I figure out how to make my goddamn docs for my yandex python wrapper build on readthedocs.io. Whichever comes first.

The widget now looks like this, with the black theme applied: