Skip to content
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

PushStyleColor and some other push/pop methods #18

Merged
merged 7 commits into from
Mar 16, 2017

Conversation

supudo
Copy link
Contributor

@supudo supudo commented Mar 2, 2017

Hey :)

Another batch - Includes PopStyleColor with the flags and some other push/pop utils.

Cheers,
S.

@swistakm
Copy link
Member

Sorry, I did not notice that you have included missing flag descriptions in this PR and I have introduced a conflict by fixing this in master branch. Can you rebase your branch so I will be able to merge through GitHub UI? Also we will see if tests pass.

@swistakm swistakm added this to the 0.0.1 - initial release milestone Mar 13, 2017
@supudo
Copy link
Contributor Author

supudo commented Mar 16, 2017

Resolved it via the web interface, let's see if i got it right.

S.

Copy link
Member

@swistakm swistakm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two syntax issues.

imgui/core.pyx Outdated
@@ -4494,7 +4540,8 @@ def is_mouse_hovering_any_window():
return cimgui.IsMouseHoveringAnyWindow()


cpdef push_style_var(cimgui.ImGuiStyleVar variable, value):
cpdef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broken definition (extra newline)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now i see that it should be simply push_style_var

imgui/core.pyx Outdated
@@ -625,7 +671,7 @@ cdef class _IO(object):
return self._ptr.MouseDoubleClickTime

@mouse_double_click_time.setter
def mouse_double_click_time(self, float value):
def mouse_double_click_time(self, float value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing colon

@supudo
Copy link
Contributor Author

supudo commented Mar 16, 2017

I'm gonna wait for any new changes until i resolve all those git issue and everything is good and merged :)

S.

Copy link
Member

@swistakm swistakm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional improvement ideas

imgui/core.pyx Outdated
.. wraps::
PushStyleColor(ImGuiCol idx, const ImVec4& col)
"""
cimgui.PushStyleColor(variable, _cast_args_ImVec4(r, g, b, a))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add something that ensures variable<ImGuiCol_COUNT and raises ValueError otherwise. See: push_style_var().

imgui/core.pyx Outdated
float r,
float g,
float b,
float a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not in the core lib but we could add default alpha value for users wanting to use the rgb-only tuples.

imgui/core.pyx Outdated
@@ -4494,7 +4540,8 @@ def is_mouse_hovering_any_window():
return cimgui.IsMouseHoveringAnyWindow()


cpdef push_style_var(cimgui.ImGuiStyleVar variable, value):
cpdef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now i see that it should be simply push_style_var

@swistakm
Copy link
Member

OK. Let's wait until you fix your git issues.

Now I have idea that we could also add some more extra context managers similar to the styled function, so we won't have to manually do color/wrapping/width pops and pushes. But maybe let's leave it as a separate issue to solve because I would like to have a consistent naming of such context managers and now I'm not sure if styled/istyled are good names. For color it is not a problem because we can have colored/icolored. Item width is more problematic. Also the i suffix may not be intuitive enough. I wonder if the following api would be better:

  • style/styles,
  • color/colors
  • item_width

Also we may want to move all extra utility functions and context managers to the separate module. The core.pyx file is already huge.

@supudo
Copy link
Contributor Author

supudo commented Mar 16, 2017

Fixes are pushed.

Not sure how it will fit the general idea of the API, but we could split as you said the public functions and the utilities, so that all functions that are usable by a user like begin(), combo(), etc. are in one file and the rest of the stuff like wrappers and helper function in another. This will break a bit the ImGui general architecture and porting, but might be better when used as source code.

Cheers,
S.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.1%) to 43.375% when pulling 61d1a1c on supudo:StylesUtils into eeb616b on swistakm:master.

@swistakm
Copy link
Member

Oh I was referring only to these functions:

styled(cimgui.ImGuiStyleVar variable, value)
istyled(*variables_and_values)
vertex_buffer_vertex_pos_offset()
vertex_buffer_vertex_uv_offset()
vertex_buffer_vertex_col_offset()
vertex_buffer_vertex_size()
index_buffer_index_size()

These functions are not the part of core C++ library. They are just some extra utilities or high level interfaces that are meant to make working with imgui easier and/or more pythonic. If we had some extra module for such utilities we could e.g. add more utilities to make some tedious C-style convetions more "pythonic".

For instance the:

if imgui.begin_menu('File', True):
    imgui.menu_item('New', 'Ctrl+N', False, True)
    imgui.menu_item('Open ...', 'Ctrl+O', False, True)

    if imgui.begin_menu('Open Recent', True):
        imgui.menu_item('doc.txt', None, False, True)
        imgui.end_menu()

    imgui.end_menu()

Could be:

with imgui.menu('File', True):
    imgui.menu_item('New', 'Ctrl+N', False, True)
    imgui.menu_item('Open ...', 'Ctrl+O', False, True)

    with imgui.menu('Open Recent', True):
        imgui.menu_item('doc.txt', None, False, True)

Such approach makes the code shorter, more readable and less error prone. I'm not a fan of complex package and module hierarchies and definitely don't want to split any of C++ mapping module. The content of such "extra" submodule would be of course imported in imgui/__init__.py so from the user's perspective nothing will change. We will at least separate the core mapping code from anything extra we would like to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants