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

A lightweight UI for medical visualizations #2: TextBox #1183

Merged
merged 14 commits into from Mar 23, 2017

Conversation

Projects
None yet
6 participants
@ranveeraggarwal
Member

ranveeraggarwal commented Mar 2, 2017

After introducing the base UI class, the button, and the panel in #1140 this is the next update in the series of PRs meant to incorporate changes of #1111.
This PR includes a TextActor2D element that subclasses vtkTextActor and a TextBox2D element that gives you an editable multi-line text area.

@ranveeraggarwal ranveeraggarwal requested review from Garyfallidis and MarcCote Mar 2, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 2, 2017

Coverage Status

Coverage increased (+0.07%) to 88.493% when pulling 062558f on ranveeraggarwal:ui into 32ed56b on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 2, 2017

Codecov Report

Merging #1183 into master will increase coverage by 0.08%.
The diff coverage is 94.39%.

@@            Coverage Diff             @@
##           master    #1183      +/-   ##
==========================================
+ Coverage   85.93%   86.01%   +0.08%     
==========================================
  Files         219      219              
  Lines       26383    26613     +230     
  Branches     2706     2737      +31     
==========================================
+ Hits        22671    22890     +219     
- Misses       3052     3055       +3     
- Partials      660      668       +8
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 93.8% <100%> (+2.77%)
dipy/viz/ui.py 93.11% <93.36%> (+0.95%)

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 32ed56b...a7a68f1. Read the comment docs.

"""
return self.GetInput()

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2017

Member

I wonder why you do not use property decorator ? This is a more pythonic way for doing Get/Set operation. Is it a design choice ? Same question for the attribute "justification", "font_size", "color", "position", "center".

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 3, 2017

Member

Agreed, it's a more Pythonic way. I gave it a try but the unit testing framework errored, probably because of some VTK support. I can try doing so in a later version.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

Ranveer is right, subclassing VTK class (thought their Python API) is really limited. For instance, it doesn't support property and multiple inheritances (and probably staticmethod). Maybe it has changed in VTK 7 but we are supporting VTK 5.8 (Travis version) and VTK 5.10 (still the default in a lot of distros, e.g. Ubuntu 16.04) and VTK 6 (manual installation).

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

@ranveeraggarwal that said, a more pythonic way would be to use composition over inheritance. @Garyfallidis, I would like your input on that matter.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 4, 2017

Member

@skoudoro @MarcCote this has now been completely modified. It uses composition over inheritance and properties.

@MarcCote

I started with docstring and PEP8 stuff. I'll have a second pass to check the code's logic.

I see you had to redo the test_ui.log.gz file. I think it is too much hassle to redo it teach time we add one UI element. I would propose to split the function test_ui into multiple smaller test functions (testing one UI element at the time). That way each test function would have its own .log.gz.

"""
return self.GetInput()

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

Ranveer is right, subclassing VTK class (thought their Python API) is really limited. For instance, it doesn't support property and multiple inheritances (and probably staticmethod). Maybe it has changed in VTK 7 but we are supporting VTK 5.8 (Travis version) and VTK 5.10 (still the default in a lot of distros, e.g. Ubuntu 16.04) and VTK 6 (manual installation).

"""
return self.GetInput()

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

@ranveeraggarwal that said, a more pythonic way would be to use composition over inheritance. @Garyfallidis, I would like your input on that matter.

@@ -10,14 +10,14 @@
from dipy.viz import window
from dipy.data import DATA_DIR
from dipy.viz.ui import UI, TextBox2D

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

More for the aesthetic, any reason why you needed to import TextBox2D explicitly instead of using UI.TextBox2D like you did for the other UI elements?

----------
text : string
The message to be set.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

PEP8: No empty line here.

----------
text : string
The new message.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

PEP8

""" Moves the caret towards right.
"""
self.caret_pos += 1

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

Could use self.caret_pos = min(self.caret_pos + 1, len(self.text))

self.caret_pos = 0
def right_move_right(self):
""" Moves right window right.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

When docstring is really short put everything on one line:
""" Moves right window right. """

This comment has been minimized.

if self.caret_pos < 0:
self.caret_pos = 0
def right_move_right(self):

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

Not sure I understand the name of this method.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 4, 2017

Member

It's basically moving the right boundary of the window to the right. I'll add it in the description.

Parameters
----------
character : string

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

str

return ret_text
def render_text(self, show_caret=True):
""" Finally renders text.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2017

Contributor

Just "Renders text"

@coveralls

This comment has been minimized.

coveralls commented Mar 4, 2017

Coverage Status

Coverage increased (+0.09%) to 88.511% when pulling ca5cd65 on ranveeraggarwal:ui into 32ed56b on nipy:master.

self.actor.GetTextProperty().SetBold(flag)
@property
def italics(self):

This comment has been minimized.

@MarcCote

MarcCote Mar 5, 2017

Contributor

Shouldn't it be italic instead of italics ?

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 5, 2017

Member

@MarcCote yep, typo. Fixed.

@coveralls

This comment has been minimized.

coveralls commented Mar 5, 2017

Coverage Status

Coverage increased (+0.09%) to 88.511% when pulling a7a68f1 on ranveeraggarwal:ui into 32ed56b on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 20, 2017

The only thing that is bugging me right now is the unusual behavior of the textbox. It doesn't feel like, let's say, a <textarea> in a browser. Specifically, in a <textarea> when it's full and needs to move the focus, a whole line is scrolled up instead of only one character at the time. Since it is usable as-is, I don't think we have to fix it this PR but we should at least flag it as an improvement issue.

@Garyfallidis: I think we could merge it and move to the next UI component. If we realize we something is not working as expected we can always fix it in a subsequent PR.
EDIT: we do have some refactoring planned in @ranveeraggarwal next PR.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Mar 20, 2017

@MarcCote you're right about the <textarea>. I made it more like a multi-line box in HTML. Sure, we can probably have an issue (enhancement) to add that feature to it. It can also be flagged as a easy/first-commit kind of thing.

@Garyfallidis with the next PR, we'll have some refactoring in the base UI element, since we found an easier way of adding events to elements.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 23, 2017

Sounds good! Merged! Keep bringing those nice PRs in!

@Garyfallidis Garyfallidis merged commit 8aa690b into nipy:master Mar 23, 2017

4 checks passed

codecov/patch 94.39% of diff hit (target 85.93%)
Details
codecov/project 86.01% (+0.08%) compared to 32ed56b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 88.511%
Details

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

Merge pull request nipy#1183 from ranveeraggarwal/ui
A lightweight UI for medical visualizations nipy#2: TextBox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment