-
Notifications
You must be signed in to change notification settings - Fork 186
gettable/settable GuiStyle colors #127
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
Conversation
Great! At first look I looks OK but I will review it in details soon. We had failing builds in the master branch due to some unfortunate docs update (some test are based on documentation) and I missed that. Now I believe that it is fixed. Could you rebase your branch so we could rerun the tests? |
If 'colors' exists, then this can co-exist and not break any existing code (though maybe needs to be deprecated?)
Though now it fails another test, as I haven't made 'colors' directly settable yet.
Now it only fails on my changes (I don't currently have a direct setter for In the meantime, I renamed
|
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.
I did detailed review. There are only two minor things that I would add/change.
Regarding the failing tests. I've checked out locally your branch and run tests. It seems that inspect
module isn't able to properly distinguish data descriptors from non-data data descriptors. I thing you can simply skip that using following patch:
diff --git a/tests/test_gui_style_initialization.py b/tests/test_gui_style_initialization.py
index ed41268..054f287 100644
--- a/tests/test_gui_style_initialization.py
+++ b/tests/test_gui_style_initialization.py
@@ -18,6 +18,8 @@ def context():
@pytest.fixture(params=IMGUI_DATA_DESCRIPTORS)
def data_descriptor(request):
+ if request.param == "colors":
+ pytest.skip("'{}' isn't writable property".format(request.param))
return request.param
--
2.21.0
That's a good change. Definitely don't want to break backwards compatibility now. |
- Indicate that 'Colors' class is for internal use via leading underscore - Added suggested docstring for the 'colors' property
Thanks for reviewing! I've incorporated your changes. |
This has been released in 1.1.0 and is available for download from PyPI. |
This allows getting and setting colors for
GuiStyle
. Currently, I thinkGuiStyle
only allows for retrieving values (not setting them). It changes the API a bit:This is my first foray into cython, so the implementation might not be quite right...
For example, here's the pyglet example + style tweaks: