Enh ui components positioning (with code refactoring) #1492

MarcCote · 2018-04-05T11:39:25Z

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 WIP: ENH: UI Listbox #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.

codecov-io · 2018-04-05T12:54:01Z

Codecov Report

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

@@            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.

MarcCote · 2018-04-13T01:16:56Z

@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

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

skoudoro · 2018-04-16T22:02:00Z

dipy/viz/ui.py

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


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

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.

skoudoro · 2018-04-16T22:06:10Z

dipy/viz/ui.py

+        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]))


PEP8 redundant parenthesis

skoudoro · 2018-04-16T22:06:29Z

dipy/viz/ui.py

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


PEP8 redundant parenthesis

skoudoro · 2018-04-19T14:14:27Z

dipy/viz/ui.py

-                                       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


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

dmreagan

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

dmreagan · 2018-04-19T21:36:01Z

dipy/viz/ui_utils.py

@@ -0,0 +1,37 @@
+
+def get_bounding_box(component, color=(1, 0, 0)):


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

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

My recommendation is to remove this function.

dmreagan · 2018-04-20T20:26:02Z

dipy/viz/ui.py

-                          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):


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.

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

I like RingSlider too!

dmreagan · 2018-04-20T21:03:16Z

dipy/viz/ui.py

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


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?

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.

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

ranveeraggarwal · 2018-05-16T18:36:19Z

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 · 2018-05-26T23:02:11Z

@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.

Garyfallidis · 2018-05-26T21:31:20Z

dipy/viz/ui.py

@@ -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)


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

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.

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

Garyfallidis · 2018-05-26T21:36:35Z

dipy/viz/ui.py

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


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

Garyfallidis · 2018-05-26T21:40:57Z

dipy/viz/ui.py

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

+        if SHOW_BOUNDING_BOX:


This section I feel is not necessary.

Garyfallidis · 2018-05-26T21:57:40Z

dipy/viz/ui.py

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


Garyfallidis · 2018-05-26T22:20:46Z

dipy/viz/ui.py

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


Remove empty line.

Garyfallidis · 2018-05-26T22:22:41Z

dipy/viz/ui.py

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


Garyfallidis · 2018-05-26T22:33:43Z

dipy/viz/ui.py

-                          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):


I like RingSlider too!

Garyfallidis · 2018-05-26T22:44:05Z

dipy/viz/ui.py


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

-    def get_actors(self):


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.

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

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.

Garyfallidis · 2018-05-26T22:49:23Z

dipy/viz/ui_utils.py

@@ -0,0 +1,37 @@
+
+def get_bounding_box(component, color=(1, 0, 0)):


My recommendation is to remove this function.

pep8speaks · 2018-05-29T11:17:25Z

Hello @MarcCote, Thank you for updating !

In the file dipy/viz/ui.py, following are the PEP8 issues :

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

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

…e/relative coordinates in Panel2D.

Garyfallidis · 2018-06-01T01:42:49Z

@MarcCote can you look at this error? https://travis-ci.org/nipy/dipy/jobs/385383361#L5024

MarcCote · 2018-06-01T12:34:34Z

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

karandeepSJ · 2018-05-31T08:44:53Z

dipy/viz/ui.py

-        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.


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.

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)?

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

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.

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).

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

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 · 2018-06-01T13:43:15Z

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

MarcCote · 2018-06-01T16:55:54Z

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

Garyfallidis · 2018-06-01T17:42:51Z

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

MarcCote · 2018-06-01T17:56:24Z

🎉

…ing2 Enh ui components positioning (with code refactoring)

MarcCote requested review from dmreagan and ranveeraggarwal and removed request for dmreagan April 5, 2018 11:39

MarcCote mentioned this pull request Apr 5, 2018

Enh ui components positioning #1448

Closed

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

MarcCote force-pushed the enh_ui_components_positioning2 branch from fa7a1da to 5def13a Compare April 5, 2018 20:15

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

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

MarcCote force-pushed the enh_ui_components_positioning2 branch from 9f25cc2 to 1d39d73 Compare April 7, 2018 02:26

skoudoro reviewed Apr 19, 2018

View reviewed changes

dmreagan reviewed Apr 19, 2018

View reviewed changes

dmreagan reviewed Apr 20, 2018

View reviewed changes

ranveeraggarwal mentioned this pull request May 16, 2018

Image element in dipy.viz.ui #1522

Merged

ranveeraggarwal approved these changes May 20, 2018

View reviewed changes

Garyfallidis reviewed May 28, 2018

View reviewed changes

MarcCote added 9 commits May 29, 2018 16:27

Remove unused attributes in UI and add better way of handling absolut…

54823e7

…e/relative coordinates in Panel2D.

Remove FileMenu2D in favor of ListBox2D.

50acf99

BF: button placement in viz_ui.py.

5e061da

Rebased + update viz examples

ee6eee3

Add Disk2D and use it as sliders' handle

8d29466

Do not start session in viz example

f370c80

Addressed @skoudoro comments

98b6565

Renamed DiskSlider2D to RingSlider2D

bbd9808

Refactor get_actors

afab328

MarcCote added 4 commits May 29, 2018 16:31

Rename DiskSlider to RingSlider

c17a642

Removed debugging BoundingBox

c67a2df

Remove empty line at the end of dosctring

866e26c

PEP8

3c4eaf0

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

Add tests files

6fdbb63

karandeepSJ reviewed Jun 1, 2018

View reviewed changes

Garyfallidis merged commit d8fb000 into dipy:master Jun 1, 2018

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

MarcCote deleted the enh_ui_components_positioning2 branch June 1, 2018 17:56

Garyfallidis mentioned this pull request Jun 1, 2018

Plans for viz module #1544

Closed

11 tasks

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

Merge pull request dipy#1492 from MarcCote/enh_ui_components_position…

c718f91

…ing2 Enh ui components positioning (with code refactoring)

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

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

Enh ui components positioning (with code refactoring) #1492

Enh ui components positioning (with code refactoring) #1492

Conversation

MarcCote commented Apr 5, 2018 • edited

codecov-io commented Apr 5, 2018 • edited

Codecov Report

MarcCote commented Apr 13, 2018

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmreagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranveeraggarwal commented May 16, 2018

Garyfallidis commented May 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented May 29, 2018 • edited

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

Garyfallidis commented Jun 1, 2018

MarcCote commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcCote Jun 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garyfallidis commented Jun 1, 2018

MarcCote commented Jun 1, 2018

Garyfallidis commented Jun 1, 2018

MarcCote commented Jun 1, 2018

MarcCote commented Apr 5, 2018 •

edited

codecov-io commented Apr 5, 2018 •

edited

pep8speaks commented May 29, 2018 •

edited

MarcCote Jun 1, 2018 •

edited