-
-
Notifications
You must be signed in to change notification settings - Fork 674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optionscontainer abilities #1117
Add optionscontainer abilities #1117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned on Gitter, something about this was grating against me - and I think I've worked out what it is.
It's the _index
suffix, and having that index suffix as an attribute that runs in parallel to a current_tab
attribute. To me, having 2 attributes for one "concept" (the currently selected tab) is messy.
My preference here would be to have a single current_tab
attribute, defined as a descriptor of the "current tab" concept.
This would make the end-user API something like:
# increment tab
myoptioncontainer.current_tab += 1
# decrement tab
myoptioncontainer.current_tab -= 1
# set index of current tab
myoptioncontainer.current_tab = 1
# Get the current tab widget
content = myoptioncontainer.current_tab
# Get the current tab index
index = myoptioncontainer.current_tab.index
# Set index of current tab explicitly;
myoptioncontainer.current_tab.index = 1
# Get current tab, the long way
content = myoptioncontainer.content[myoptioncontainer.current_tab.index]
Going down this path also allows for current_tab
to expose a richer API in future. For example, we could allow setting the current tabs by label:
myoptioncontainer.current_tab = "Important"
To be clear - I'm not suggesting this as a requirement for this iteration of the patch; only that it's an option if we make the current tab a rich descriptor, rather than exposing 2 different attributes.
Fixed the issues presented in the code review 💪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now, and works well for the Windows case; however it's causing a hard crash on startup with the OptionContainer example on Linux and macOS. It looks like it's a bootstrapping problem with the value of 'current tab' when the app is starting up; I had a quick poke around and couldn't find an obvious fix, though. I'll try to take a closer look later today after I've unpacked a few more boxes :-)
@saroad2 Finally got to the bottom of the problem with Cocoa, and I've fleshed out the GTK implementation as well. If you get a chance to verify that I haven't messed up anything you're relying on, let me know; otherwise I'll let this sit for a day or two, test it again, then merge. |
Hey @freakboy3742 , I checked in Windows and everything seems to be working! |
Added missing abilities in the
OptionContainer
class:Implemented those abilities in WIndows only.
Also added missing unit tests in order to increase coverage in optioncontainer.py. I wanted to get to 100% coverage, but it seems like nobody uses lines 69 and 93. If it's not in use, we can delete those methods.
PR Checklist: