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

Enh ui components positioning (with code refactoring) #1492

Merged
merged 14 commits into from Jun 1, 2018

Conversation

Projects
8 participants
@MarcCote
Copy link
Contributor

MarcCote commented Apr 5, 2018

This PR supersedes both #1441 and #1448. This PR is meant to reduce changes in #1355 (but it's not the case anymore :P).

Changes similar to #1441

  • Remove unused attributes in UI.
  • Add a better way of handling absolute/relative coordinates in Panel2D.
  • Remove FileSelectMenu2D that will be replaced with ListBox2D (will be added in #1355).

New changes

  • Move the logic for positioning UI component into the abstract class UI.
  • Every UI component needs to implement _set_position(coords) which is responsible for positioning the lower-left corner of the component (conventional way of positioning actors in VTK) and _setup() which is responsible for creating underlying VTK actors.
  • The UI components' constructor all have an optional position argument instead of center.
  • In order for property center and method set_center to work, UI components needs to implement _get_size().
  • New UI component Disk2D which is used for the sliders' handles. This allows for better and easier handling of the component positioning and events management.

@MarcCote MarcCote requested review from dmreagan and ranveeraggarwal and removed request for dmreagan Apr 5, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 5, 2018

Codecov Report

Merging #1492 into master will decrease coverage by 0.07%.
The diff coverage is 90.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1492      +/-   ##
==========================================
- Coverage   87.55%   87.48%   -0.08%     
==========================================
  Files         246      242       -4     
  Lines       31370    30648     -722     
  Branches     3434     3300     -134     
==========================================
- Hits        27467    26812     -655     
+ Misses       3085     3068      -17     
+ Partials      818      768      -50
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 88.17% <100%> (+2.8%) ⬆️
dipy/viz/ui_utils.py 3.7% <3.7%> (ø)
dipy/viz/ui.py 90.25% <96.08%> (+0.95%) ⬆️
dipy/direction/tests/test_pmf.py 90.47% <0%> (-5.61%) ⬇️
dipy/segment/clustering.py 93.66% <0%> (-0.98%) ⬇️
dipy/tracking/streamline.py 95.11% <0%> (-0.96%) ⬇️
dipy/reconst/csdeconv.py 89.28% <0%> (-0.65%) ⬇️
dipy/core/geometry.py 90.29% <0%> (-0.38%) ⬇️
dipy/segment/tests/test_qbx.py 96.89% <0%> (-0.23%) ⬇️
dipy/tracking/tests/test_streamline.py 98.24% <0%> (-0.01%) ⬇️
... and 13 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 804f4e8...6fdbb63. Read the comment docs.

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Apr 5, 2018

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_components_positioning2 branch from fa7a1da to 5def13a Apr 5, 2018

@MarcCote MarcCote changed the title Enh ui components positioning2 Enh ui components positioning (with code refactoring and documentation) Apr 7, 2018

@MarcCote MarcCote changed the title Enh ui components positioning (with code refactoring and documentation) Enh ui components positioning (with code refactoring) Apr 7, 2018

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_components_positioning2 branch from 9f25cc2 to 1d39d73 Apr 7, 2018

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Apr 13, 2018

@dmreagan @ranveeraggarwal I'm not planning on making any more modification. I think this PR clean a lot of the UI code we had and centralized a bunch of functionalities (e.g. position, size, center, _setup, ...).

It also shows how event callbacks should be set and used (e.g. component.on_left_mouse_button_clicked = callback, component.on_left_mouse_button_clicked = callback, ...).

Lastly, it favors the use of simpler UI components to build more complicated one (e.g. LineSlider2D which uses a flat Rectangle2D as the line of the slider and Disk2D for its handle).

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @MarcCote, Looks good to me, all tests pass on windows too. I just made a couple of remarks below

def center(self):
return self.position + self.size / 2.

def set_center(self, coords):

This comment has been minimized.

@skoudoro

skoudoro Apr 19, 2018

Member

Why you do not use @center.setter but you have done it for all the other attributes ?

This comment has been minimized.

@MarcCote

MarcCote Apr 20, 2018

Contributor

At first, I was trying to not break backward compatibility.... but at some point, I gave up :P. I will change that to be more pythonic.

lower_left_corner = self._points.GetPoint(0)
upper_right_corner = self._points.GetPoint(2)
size = np.array(upper_right_corner) - np.array(lower_left_corner)
return (abs(size[0]), abs(size[1]))

This comment has been minimized.

@skoudoro

skoudoro Apr 19, 2018

Member

PEP8 redundant parenthesis


def _get_size(self):
diameter = 2 * self.outer_radius
return (diameter, diameter)

This comment has been minimized.

@skoudoro

skoudoro Apr 19, 2018

Member

PEP8 redundant parenthesis

click_position[1] - panel2d_object.ui_param[1]))
if panel2d_object._drag_offset is not None:
click_position = np.array(i_ren.event.position)
new_position = click_position - panel2d_object._drag_offset

This comment has been minimized.

@skoudoro

skoudoro Apr 19, 2018

Member

Since you need _drag_offset here, Why you keep it as protected variable and not public one ?

@dmreagan
Copy link
Contributor

dmreagan left a comment

I'm not done looking through everything, but so far so good. In particular, I really like the new way callbacks are handled.

@@ -0,0 +1,37 @@

def get_bounding_box(component, color=(1, 0, 0)):

This comment has been minimized.

@dmreagan

dmreagan Apr 19, 2018

Contributor

Needs some documentation. What is it? What is component? When and how is it used?

This comment has been minimized.

@MarcCote

MarcCote Apr 20, 2018

Contributor

Right, I wasn't sure to leave that function but it is useful. I've added a proper docstring.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

My recommendation is to remove this function.

This comment has been minimized.

@MarcCote

MarcCote May 28, 2018

Contributor

ok

self.disk_move_callback)
self.track.on_left_mouse_button_pressed = self.slider_track_click_callback
self.track.on_left_mouse_button_dragged = self.handle_move_callback
self.handle.on_left_mouse_button_dragged = self.handle_move_callback


class DiskSlider2D(UI):

This comment has been minimized.

@dmreagan

dmreagan Apr 20, 2018

Contributor

While we're making major changes, should we consider renaming this to something like RingSlider2D or CircleSlider2D? I know it's based on the VTK disk primitive, but it looks more like a ring or circle. Similarly, we have a LineSlider2D even though it uses a rectangle underneath.

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2018

Contributor

I like RingSlider2D. @ranveeraggarwal and @Garyfallidis any opinions on that?

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

I like RingSlider too!

"""
msg = "Subclasses of UI must implement `_setup(self)`."
raise NotImplementedError(msg)

def get_actors(self):

This comment has been minimized.

@dmreagan

dmreagan Apr 20, 2018

Contributor

I'm not sure I understand the relationship between get_actors() and add_to_renderer(). It seems that simple classes like Rectangle2D which are built with a single VTK actor return that actor from get_actors() and use the add_to_renderer() from UI. More complex classes which are composites of simple classes, like LineSlider2D, return nothing from get_actors() and instead override add_to_renderer(). But then there's TextBox2D, which is a complex composite class in that it is built on top of TextBlock2D instead of a VTK actor, but it uses get_actors() more like a simple class. Can we simplify this somehow, perhaps by forcing subclasses to implement add_to_renderer() instead of get_actors()? Or by forcing composite classes to return their constituent actors in get_actors() like TextBox2D does?

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2018

Contributor

I think add_to_renderer could work. Maybe there will be some issues with the callbacks but I'll try it out. If we can avoid dealing with VTK actors as much as we can that's nice I found.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

These are all good points @dmreagan we need to homogenize the API for all UI elements.

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented May 16, 2018

This is lovely! I don't know how I missed this.
I will finish my review in a couple of days and then we can merge this, enabling #1355

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented May 26, 2018

@MarcCote you have removed the FileSelectMenu2D that means that after this is merged we will have to merge the ListBox2D fast otherwise people will not have this functionality in master. And possibly keep both names too.

I only have a few comments. Please make the corrections and will merge asap to facilitate the GSoC projects.

@@ -19,6 +20,9 @@

TWO_PI = 2 * np.pi

# Set to True or 1 to display bounding box around UI components.
SHOW_BOUNDING_BOX = os.environ.get("DIPY_VIZ_DEBUG", False)

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Hi @MarcCote This here is unexpected. Please remove. Also remember that VTK has it's own system for generating aabbs etc. It will be better to use the underlying utilities mostly for speed.
https://www.vtk.org/Wiki/VTK/Examples/Cxx/PolyData/Outline
https://www.vtk.org/Wiki/VTK/Examples/Cxx/Utilities/BoundingBox

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Also I am not sure why we need an environment variable for this feature. Perhaps that could be a parameter in UI. But to have an environment variable for showing the outline seems unnecessary.

This comment has been minimized.

@MarcCote

MarcCote May 28, 2018

Contributor

It was only for debugging purpose. Checking the position of all the (sub)components and props. I can remove everything that's fine.

"""
msg = "Subclasses of UI must implement `_setup(self)`."
raise NotImplementedError(msg)

def get_actors(self):

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

These are all good points @dmreagan we need to homogenize the API for all UI elements.

@@ -96,6 +113,12 @@ def add_to_renderer(self, ren):
"""
ren.add(*self.get_actors())

if SHOW_BOUNDING_BOX:

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

This section I feel is not necessary.

Button size (width, height) in pixels.
"""
# Update actor.

This comment has been minimized.

@Garyfallidis
----------
alignment : [left, right]
Alignment of the panel with respect to the overall screen.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Remove empty line.

between [0,1].
If int, pixels coordinates are assumed and it must fit within the
panel's size.

This comment has been minimized.

@Garyfallidis
self.disk_move_callback)
self.track.on_left_mouse_button_pressed = self.slider_track_click_callback
self.track.on_left_mouse_button_dragged = self.handle_move_callback
self.handle.on_left_mouse_button_dragged = self.handle_move_callback


class DiskSlider2D(UI):

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

I like RingSlider too!


# Update text.
text = self.format_text()
self.text.message = text

def get_actors(self):

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Okay now get_actors returns an empty list for this class. We should make a decision if we are going to keep that method or not. And do similar everywhere in ui.py.
I think that having get_actors implemented is actually quite useful not so much for adding actors to the renderer, but more for changing properties of the UIs such as color, opacity etc that may not be options in the UI itself. Also we will need access to the actors if we want to add shaders to any of the UI elements.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Food for thought. No need to change this in this PR.

This comment has been minimized.

@MarcCote

MarcCote May 28, 2018

Contributor

I agree. I did try to remove get_actors but I found out that is was very useful for setting the visibility and adding event listeners (as used in the unit tests). I might define get_actors as the method that returns all VTK props, including all sub VTK props (from sub components). @dmreagan @Garyfallidis I'll be implementing that tonight or tomorrow, let me know if you think it's not a good idea.

@@ -0,0 +1,37 @@

def get_bounding_box(component, color=(1, 0, 0)):

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

My recommendation is to remove this function.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 29, 2018

Hello @MarcCote, Thank you for updating !

Line 1582:24: E127 continuation line over-indented for visual indent

Comment last updated on June 01, 2018 at 12:33 Hours UTC

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_components_positioning2 branch from a88af43 to 3c4eaf0 May 29, 2018

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 1, 2018

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Jun 1, 2018

@Garyfallidis I pushed the test files I renamed. Good catch.

same time as this one.
parent_ui: UI
Reference to the parent UI element. This is useful of there is a parent
UI element and its reference needs to be passed down to the child.

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 1, 2018

Contributor

I am using parent_ui in the new elements I have implemented. Checkbox is a combination of Button2D and TextBox2D objects and when an event in a textbox occurs, I need to access its corresponding button which I cannot do without parent_ui because the event handlers do not take self as a parameter.

This comment has been minimized.

@MarcCote

MarcCote Jun 1, 2018

Contributor

Looking at your Checkbox branch, wouldn't it be that making the key_press method a member of the Checkbox class rather than a staticmethod solves the problem? You would be able to use self. instead of textbox_obj.parent_ui.

That said, it is clear that the old way of dealing with events is broken. For instance, your Checkbox UI component needed to capture the on_key_press event and essentially re-implement the textbox behavior (i.e. the same code is used in the TextBox and Checkbox component).

Also, why does the Checkbox need a TextBox rather than a TextBlock (i.e. label)?

This comment has been minimized.

@MarcCote

MarcCote Jun 1, 2018

Contributor

Also, should we make the a distinction between Checkbox and GroupOfCheckboxes?

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 1, 2018

Contributor

Yeah self. works.

I wanted to keep the options editable so I used a TextBox. I can change it to TextBlock if required.

Individual checkboxes are present in self.options as tuples. Checkbox is just a better name than GroupOfCheckboxes. I'll mention in the docstring that this implements a collection of checkboxes and not a single option.

This comment has been minimized.

@MarcCote

MarcCote Jun 1, 2018

Contributor

What I meant is we should have an atomic Checkbox UI component. Like

class Checkbox(UI):

    def __init__(self, label):
        """
        Parameters
        ----------
        label : str
            Checkbox's label.

        """
        super(Checkbox, self).__init__()
        self.text.message = label

    def _setup(self):
        """ Setup this UI component.

        TODO
        """
        # Checkbox's button
        self.button_icons = {}
        self.button_icons['unchecked'] = read_viz_icons(fname="stop2.png")
        self.button_icons['checked'] = read_viz_icons(fname="checkmark.png")
        self.button = Button2D(icon_fnames=self.button_icons)

        self.text = TextBlock2D()

        # Add default events listener for this UI component.
        self.button.on_left_mouse_button_pressed = self.toggle_check

    def _get_actors(self):
        """ Get the actors composing this UI component.
        """
        return self.button.actors + self.text.actors

    def _add_to_renderer(self, ren):
        """ Add all subcomponents or VTK props that compose this UI component.

        Parameters
        ----------
        ren : renderer
        """
        self.button.add_to_renderer(ren)
        self.text.add_to_renderer(ren)

    def _get_size(self):
        # Consider the handle's size when computing the slider's size.
        width = self.button.size[0] + 5 + self.text.size[0]
        height = max(self.button.size[1], self.text.size[1])
        return np.array([width, height])

    def _set_position(self, coords):
        """ Position the lower-left corner of this UI component.

        Parameters
        ----------
        coords: (float, float)
            Absolute pixel coordinates (x, y).
        """
        # [x] Label
        self.button.position = coords
        offset = (self.button.size[0] + 5, 0)
        self.text.position = self.button.position + offset

    def toggle_check(self, i_ren, obj, button_object):
        self.button.next_icon()
        i_ren.force_render()

Then, have another UI Component that have a group of Checkboxes if desired which should create instances of SingleCheckbox and add them to a Panel2D.
EDIT: this is based on my enh_ui_component2 branch (this PR).

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 1, 2018

Contributor

Yeah this is pretty good.

But for RadioButton we can't have a toggle_check for each option. That we will have to add in the GroupOfRadioButton class. So maybe, I can make an Option class like yours without the toggle_check function and use this class for both GroupOfCheckbox and GroupOfRadioButton

This comment has been minimized.

@MarcCote

MarcCote Jun 1, 2018

Contributor

A radio button component could be seen as a "GroupOfCheckBox" with some restrictions on how many "Checkbox" can be selected. Also, each Checkbox should have a "on_change" callback function that gets triggered whenever a Checkbox changes state (similar to https://github.com/nipy/dipy/pull/1492/files#diff-fa451eb394ebc769b7b376b567c756b5R149)

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 1, 2018

@MarcCote, @karandeepSJ is saying that you removed some parameters that he really needs. Check above!

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Jun 1, 2018

@dmreagan @ranveeraggarwal @Garyfallidis I think all of your comments were addressed. If it's not the case let me know.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 1, 2018

Okay looks good. Merging... Now @karandeepSJ, @ranveeraggarwal and @MarcCote we will need to rebase and update our PRs! GGT! Thanks @MarcCote!

@Garyfallidis Garyfallidis merged commit d8fb000 into nipy:master Jun 1, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

Viz Module automation moved this from UI PR needs a review to Done Jun 1, 2018

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Jun 1, 2018

🎉

@MarcCote MarcCote deleted the MarcCote:enh_ui_components_positioning2 branch Jun 1, 2018

@Garyfallidis Garyfallidis referenced this pull request Jun 1, 2018

Closed

Plans for viz module #1544

2 of 11 tasks complete

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1492 from MarcCote/enh_ui_components_position…
…ing2

Enh ui components positioning (with code refactoring)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment