Merge vnotebook into master to add VNotebook widget #53
Conversation
Rename TestVNotebook.add_test_frame() to prevent this method to be executed as part of the tests with no arguments which lead to the previous error.
|
I think the error in the python 3.4 test with Appveyor is because ttkthemes is used in the example_vnotebook.py and probably the older version of pip or setuptools does not recognize requirements properly so we should add it explicitly in .appveyor.yml. However I am reluctant to add ttkthemes as a dependency just for the sake of one example, we could use the 'clam' theme instead. |
| :return: ID for the new Tab | ||
| :rtype: int | ||
| """ | ||
| tab_id = kwargs.pop("id", hash(child)) |
There was a problem hiding this comment.
I have noticed a weird behavior with the add() method: When adding a widget which is already inside the notebook, a new tab is created, but the old one is not deleted and they are both activated simultaneously. I think we should add code to check whether the widget is already in the notebook and just update its properties with kwargs like the ttk.Notebook does.
There was a problem hiding this comment.
just fixed this
check my latest commit
ttkwidgets/frames/vnotebook.py
Outdated
| :param compound: Location of the Buttons | ||
| :type compound: str | ||
| :param callback: Callback called upon change of Frame | ||
| :type callback: callable |
There was a problem hiding this comment.
The callback option does not seem to have been implemented yet, is that so?
There was a problem hiding this comment.
By the way, wouldn't it be more consistent with the ttk.Notebook widget to use a virtual event, e.g. <<VNotebookTabChanged>> instead of a callback?
There was a problem hiding this comment.
I agree. A virtual event should be the way to go as it's more consistent with the rest of tkinter and ttk. It's also much easier to implement, and easier to document. Now, what data should the event argument hold in that case?
There was a problem hiding this comment.
When I make tkinter print the <<NotebookTabChanged>> event, I get <VirtualEvent event x=0 y=0> so I think we don't have to pass any specific argument to the event, the new tab id and everything else can be recovered from the notebook's methods.
There was a problem hiding this comment.
I removed the 'callback' argument from the documentation, there didn't seem to be any code to remove with that change. I'll let you handle the event generation since you seem to be doing that.
ttkwidgets/frames/vnotebook.py
Outdated
| for option in self.options: | ||
| attr = "_{}".format(option) | ||
| setattr(self, attr, kwargs.pop(option, getattr(self, attr))) | ||
| return ttk.Frame.configure(**kwargs) |
There was a problem hiding this comment.
When using .configure() I get the error
Traceback (most recent call last):
File "/media/DATA/Divers/ttkwidgets/tests/test_vnotebook.py", line 77, in test_configure
notebook.config(padding=6)
File "/media/DATA/Divers/ttkwidgets/ttkwidgets/frames/vnotebook.py", line 264, in configure
setattr(self, attr, kwargs.pop(option, getattr(self, attr)))
AttributeError: 'VNotebook' object has no attribute '_takefocus'I think we need to had the line
self._takefocus = kwargs.pop("takefocus", False)
in __init__() to fix that. And the one for callback is missing as well but the option is not implemented yet.
Moreover, there is a self missing at l. 265: return ttk.Frame.configure(self, **kwargs)
Finally, if all arguments are in self.options, then kwargs is emptied and therefore the dict of all the frame options is returned which is a bit weird.
There was a problem hiding this comment.
for the self missing, shouldn't we just use super() instead of calling ttk.Frame directly ? It's much cleaner after all.
There was a problem hiding this comment.
Yes we can use super() though I have not taken the habit to use it yet, but this does not solve the issue of the behavior of the function. I don't think that many people use .configure("<key>") since the output is not really usable in python (e.g. ('width', 'width', 'Width', 0, 0)). So maybe we can return nothing and just use the method for its main purpose which is to change the options of the widget.
There was a problem hiding this comment.
I removed the call to ttk.Frame, in favor of a super() call. We could just return whatever ttk.Frame.configure returns, that way it stays consistent with the rest of the tkinter framework.
| from tkinter import ttk | ||
| # Module to test | ||
| from ttkwidgets.frames import VNotebook | ||
|
|
There was a problem hiding this comment.
I think we can add some more tests, like for the configure() method. I can write some.
PR Details:
Description
A Notebook with vertical tabs
Checklist
/examples/testsAUTHORS.md