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

Checkbox and RadioButton elements for viz.ui #1559

Merged
merged 16 commits into from Aug 22, 2018

Conversation

Projects
7 participants
@karandeepSJ
Copy link
Contributor

karandeepSJ commented Jun 10, 2018

This PR implements 3 classes.
Need to merge #1547 first

Option

This is a set of a Button2D and a TextBlock2D to act as a single option for check-boxes and radio buttons.
It aligns the button with the label using the number of \n in the label.

Checkbox

This class uses a list of objects of Option class to implement the checkbox element. It is a group of options where multiple options can be checked.

checkbox

RadioButton

This class implements the radio button element where only a single option can be selected at a time.
It inherits from the Checkbox class with a different toggle function to uncheck all other options when one is clicked.

radiobutton

@dmreagan dmreagan added this to PR needs a review in Viz Module Jun 11, 2018

@skoudoro skoudoro added the gsoc2018 label Jun 13, 2018

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:Checkbox branch from 21fa815 to b46fd81 Jun 14, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.01%.
The diff coverage is 85.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   87.32%   87.31%   -0.02%     
==========================================
  Files         246      246              
  Lines       31814    32000     +186     
  Branches     3451     3470      +19     
==========================================
+ Hits        27783    27942     +159     
- Misses       3208     3230      +22     
- Partials      823      828       +5
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 82.54% <77.52%> (-1.2%) ⬇️
dipy/viz/ui.py 89.12% <92%> (+0.28%) ⬆️

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 405ab40...ece81d2. Read the comment docs.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 15, 2018

@karandeepSJ please resolve the conflicts here.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:Checkbox branch from b46fd81 to d9104fa Jun 16, 2018

@MarcCote MarcCote self-requested a review Jun 17, 2018

@dmreagan
Copy link
Contributor

dmreagan left a comment

Please also add examples.

The label for the option.
font_size : int
The size of the font for the label.
font_size : int

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Duplicate attribute

Parameters
----------
label : str
Option's label.

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Be more descriptive. Something like, "The text to be displayed next to the button"

Absolute coordinates (x, y) of the lower-left corner of
the button of the option.
font_size : int
Font Size of the label.

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Needs a lower-case 's' in Size

self.text.add_to_renderer(ren)

def _get_size(self):
width = self.button.size[0] + 10 + self.text.size[0]

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

10 is a magic number here and in line 3035. It should be pulled out into a variable so the two don't get out of sync.

num_newlines = self.label.count('\n')
self.button.position = coords + \
(0, num_newlines * self.font_size * 0.5)
offset = (self.button.size[0] + 10, 0)

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Other instance of magic number.

font_size=self.font_size,
position=(self.position[0], button_y))
button_y = button_y + self.font_size * \
(self.labels[option_no].count('\n') + 1) * 1.2 + self.padding

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Do something about magic number 1.2, which also appears on lines 2987 and 3149.

"""
self.options = []
button_y = self.position[1]
for option_no in range(self.num_options):

This comment has been minimized.

@dmreagan

dmreagan Jul 3, 2018

Contributor

Is num_options necessary in this class? It seems like you could write this loop as

for label in self.labels

Otherwise, it is only used in _get_size()

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:Checkbox branch from 5e88a7f to 716627d Jul 3, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 24, 2018

Hi @karandeepSJ,

As requested before, can you rebase this PR and add an example. #1547 has been merged.

Thank you

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:Checkbox branch from 716627d to c663ba5 Jul 24, 2018

@dmreagan
Copy link
Contributor

dmreagan left a comment

In the future, I'd like to see the option labels respond to clicks the same as the button does, but that can wait for another PR.

if option.checked is True:
event.append(option.label)
i_ren.force_render()
print(event)

This comment has been minimized.

@dmreagan

dmreagan Jul 26, 2018

Contributor

Remove prints

option.button.next_icon()

i_ren.force_render()
print(event)

This comment has been minimized.

@dmreagan

dmreagan Jul 26, 2018

Contributor

Remove prints

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:Checkbox branch from 8e9a208 to 95a4be8 Aug 1, 2018

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 2, 2018

Can we change this so the options are drawn starting from the top instead of the bottom? Right now, the first option is on the bottom and the last option is on the top.

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 9, 2018

@MarcCote any thoughts on this before it goes in?

@MarcCote
Copy link
Contributor

MarcCote left a comment

I don't want to hold this PR any longer. It seems to be working well. Except for the weird events handling (which is a problem bigger with the framework not this PR), this PR LGTM.

@@ -2952,6 +2952,283 @@ def set_img(self, img):
self.texture = set_input(self.texture, img)


class Option(UI):

This comment has been minimized.

@MarcCote

MarcCote Aug 9, 2018

Contributor

I was expecting the Option class to have a select() and a deselect() methods which would take care of changing the option's appearance accordingly.

(label.count('\n') + 1) * (line_spacing + 0.1) + self.padding
option.button.on_left_mouse_button_pressed = self.toggle_check
self.options.append(option)
option.button.add_callback(option.text.actor,

This comment has been minimized.

@MarcCote

MarcCote Aug 9, 2018

Contributor

I think we can all agree on the way we are handling events is unintuitive (and probably bogus) right? @Garyfallidis @dmreagan @ranveeraggarwal
In this particular instance, we are adding a callback (which is a method of a CheckBox object) to the VTKActor of a TextBlock2D object using the add_callback of a Button2D object!

I think the proper way would have been something like:
option.on_change = self._handle_new_selection.

This comment has been minimized.

@karandeepSJ

karandeepSJ Aug 10, 2018

Contributor

I did this to avoid looping through all the options in the callback. This sends the button of the text clicked as an argument to the add_callback() function, so that its icon can be changed directly, without searching for the Button2D corresponding to that TextBlock2D

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

I see. Wouldn't it be better if the Option object was handling that for you? To me, whether the TextBlock2D is associated with a particular Button2D shouldn't influence the behavior of CheckBox and RadioBox classes.

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

See karandeepSJ#1 for a way of doing what I described above.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Aug 10, 2018

Hello @karandeepSJ, Thank you for updating !

Line 460:1: W293 blank line contains whitespace
Line 2998:1: W293 blank line contains whitespace
Line 3053:1: W293 blank line contains whitespace
Line 3066:1: W293 blank line contains whitespace
Line 3128:1: W293 blank line contains whitespace

Comment last updated on August 16, 2018 at 20:57 Hours UTC
test_ui_checkbox(interactive=False)

if len(sys.argv) <= 1 or sys.argv[1] == "test_ui_radio_button":
test_ui_radio_button(interactive=False)

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

It should be interactive=True though. When the script is called directly, interactive should be true.

@MarcCote
Copy link
Contributor

MarcCote left a comment

See comment about interactive=True when test script is called directly.

@MarcCote
Copy link
Contributor

MarcCote left a comment

LGTM

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Aug 20, 2018

@dmreagan feel free to merge this one whenever you want.

@dmreagan dmreagan merged commit b1ec68b into nipy:master Aug 22, 2018

1 of 3 checks passed

codecov/patch 85.18% of diff hit (target 87.32%)
Details
codecov/project 87.31% (-0.02%) compared to 405ab40
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from PR needs a review to Done Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment