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

Make TextActor2D extend UI instead of object #1298

Merged
merged 4 commits into from Aug 6, 2017

Conversation

dmreagan
Copy link
Contributor

Previously, TextActor2D was used by other classes like TextBox2D and LineSlider2D, but could not be added to the ShowManager nor to a Panel2D as a standalone UI element. However, TextActor2D can be useful as a text label which is not editable by users, so this PR promotes it to a full member of viz.ui.

@dmreagan
Copy link
Contributor Author

What do you think, @ranveeraggarwal , @Garyfallidis ? Do I need to update the UI tests?

@ranveeraggarwal
Copy link
Contributor

ranveeraggarwal commented Jul 11, 2017

Actually, TextActor2D has had quite a history. We initially inherited it from the vtk text actor and then later chose composition over inheritance and made everything a property. After that, we never really thought about it. This sounds like a good idea to me. In fact, I had been meaning to do so in my current dangling PR on FileSelect2D.
Let's call it TextBlock2D instead of TextActor2D then, because it's not just an actor anymore, it's a full fledged UI element (yep, this is inspired from WPF, where you have textboxes and textblocks).

Edit: We'll also need to fix the existing UI elements that use this.

dipy/viz/ui.py Outdated
position : (float, float)

"""
self.actor.SetPosition(*position)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the property instead.

self.position = position

@ranveeraggarwal
Copy link
Contributor

For the tests, you can add a textblock to an existing panel's test so that the get_actor and set_center methods are covered.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 85.429% when pulling 7089578 on dmreagan:textactor2d into 736cbb5 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 85.429% when pulling 7089578 on dmreagan:textactor2d into 736cbb5 on nipy:master.

@codecov-io
Copy link

codecov-io commented Jul 11, 2017

Codecov Report

Merging #1298 into master will increase coverage by <.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1298      +/-   ##
==========================================
+ Coverage   87.02%   87.03%   +<.01%     
==========================================
  Files         228      228              
  Lines       28812    28821       +9     
  Branches     3094     3094              
==========================================
+ Hits        25075    25085      +10     
  Misses       3034     3034              
+ Partials      703      702       -1
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 85.23% <100%> (+0.28%) ⬆️
dipy/viz/ui.py 92.34% <85.71%> (+0.05%) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 a0df597...f92f06b. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 85.427% when pulling 4b9ff4a on dmreagan:textactor2d into 736cbb5 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 85.427% when pulling 4b9ff4a on dmreagan:textactor2d into 736cbb5 on nipy:master.

text_block_test = ui.TextBlock2D()
text_block_test.message = 'TextBlock'
text_block_test.color = (0, 0, 0)
# /TextBlock
Copy link
Contributor

Choose a reason for hiding this comment

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

Are comments like this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just copied the style I saw for other tests. I'll remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just added them for easy readability, since we're putting in all components in a single file. They can be removed, no problem.

npt.assert_equal((1, 0, 0), text_block.color)
text_block.position = (2, 3)
npt.assert_equal((2, 3), text_block.position)
# /TextBlock2D
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@Garyfallidis
Copy link
Contributor

I tried the code in my computer. Seems working well. Apart from some minor comments, seems this is ready to be merged.

@ranveeraggarwal
Copy link
Contributor

ranveeraggarwal commented Aug 4, 2017

@dmreagan do verify that the other dependent UI components (TextBox, LineSlider and Circular Slider) work with these changes. PR #1262 will need fixes with these changes. Otherwise this PR looks good to me 😄

@dmreagan
Copy link
Contributor Author

dmreagan commented Aug 4, 2017

@ranveeraggarwal Those components look good to me in the viz_ui.py example, so I think it is good to go. I'm pretty sure I just had to switch the name.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 85.329% when pulling f92f06b on dmreagan:textactor2d into a0df597 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 85.329% when pulling f92f06b on dmreagan:textactor2d into a0df597 on nipy:master.

@Garyfallidis Garyfallidis merged commit 5595842 into dipy:master Aug 6, 2017
@Garyfallidis
Copy link
Contributor

Thank you @dmreagan

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
Make TextActor2D extend UI instead of object
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

5 participants