Merge notebook into master for improved Notebook widget

[RedFantom]

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

• Widget in a separate file in the appropriate folder
• Widget functions properly on both Windows and Linux
• Widget code includes docstrings with parameter descriptions
• Included an example file in /examples
• Widget is covered by unitttests in /tests
• Widget includes required assets files
• 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-io]

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%)	⬇️
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%)	⬇️
ttkwidgets/font/familylistbox.py	82.75% <0.00%> (-13.08%)	⬇️
ttkwidgets/font/familydropdown.py	64.00% <0.00%> (-11.00%)	⬇️
ttkwidgets/scrolledlistbox.py	85.18% <0.00%> (-10.47%)	⬇️
ttkwidgets/frames/toggledframe.py	90.32% <0.00%> (-9.68%)	⬇️
... 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.

[RedFantom]

TODO List:

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

[RedFantom]

The tests should be extended to be much more extensive and cover more of the notebook.py file. Just testing initialization is not sufficient.

[RedFantom]

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.

[sbordeyne]

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

[RedFantom]

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.

[sbordeyne]

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

[sbordeyne]

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

[sbordeyne]

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)

[RedFantom]

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.

[sbordeyne]

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

[sbordeyne]

pushed a bunch of fixes to notebook-review-fixes

[RedFantom]

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.

[sbordeyne]

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

[RedFantom]

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.

[sbordeyne]

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 ?

[RedFantom]

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.

[sbordeyne]

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

[RedFantom]

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.

[RedFantom]

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.

[sbordeyne]

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'>
>>>

[RedFantom]

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