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 more use of TextBlock2D constructor #1362

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@dmreagan
Contributor

dmreagan commented Oct 20, 2017

No description provided.

@dmreagan dmreagan requested review from ranveeraggarwal and MarcCote Oct 20, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Oct 20, 2017

Codecov Report

Merging #1362 into master will decrease coverage by 8.01%.
The diff coverage is 31.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1362      +/-   ##
==========================================
- Coverage   86.98%   78.97%   -8.02%     
==========================================
  Files         227      227              
  Lines       28946    28919      -27     
  Branches     3117     3115       -2     
==========================================
- Hits        25179    22839    -2340     
- Misses       3062     5501    +2439     
+ Partials      705      579     -126
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 18.25% <0%> (-65.29%) ⬇️
dipy/viz/ui.py 19.82% <33.33%> (-70.05%) ⬇️
dipy/viz/tests/test_window.py 12% <0%> (-81.6%) ⬇️
dipy/viz/widget.py 6.71% <0%> (-81.35%) ⬇️
dipy/viz/interactor.py 19.37% <0%> (-78.75%) ⬇️
dipy/viz/actor.py 5.5% <0%> (-74.11%) ⬇️
dipy/viz/tests/test_interactor.py 18.18% <0%> (-71.6%) ⬇️
dipy/viz/colormap.py 17.85% <0%> (-68.58%) ⬇️
... and 17 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 f3b517b...c0af1b7. Read the comment docs.

self.actor = TextBlock2D(self.text, font_size, font_family,
justification, bold, italic, shadow, color,
position)
if vtk.vtkVersion.GetVTKSourceVersion().split(' ')[-1] <= "6.2.0":

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Oct 24, 2017

Member

Can you create a function _build() and place the background logic inside it? I know it's kind of redundant, but we have been doing things that way for every UI element. Also, in future, if we are to add some more modifications to the text block at the text box level, we might end up complicating the constructor, so a separate build function makes more sense.

This comment has been minimized.

@dmreagan

dmreagan Nov 3, 2017

Contributor

Do you have other changes in mind that are specific to TextBox2D or FileSelect2D? If not, I think we could clean this up better by moving background color, background opacity, and line spacing to the TextBlock2D constructor. Those are probably common enough settings to belong there.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Nov 4, 2017

Member

As of this moment, no.

if vtk.vtkVersion.GetVTKSourceVersion().split(' ')[-1] <= "6.2.0":
pass
else:
text_actor.actor.GetTextProperty().SetBackgroundColor(1, 1, 1)
text_actor.actor.GetTextProperty().SetBackgroundOpacity(1.0)
self.text_actor.actor.GetTextProperty().SetBackgroundColor(1, 1, 1)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Oct 24, 2017

Member

Same as above, can you have the build_actor or _build method do this?
(I guess we should stick to the _build name for the method as it's private)

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 24, 2017

Lgtm apart from a couple of modifications.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 30, 2017

I agree with @ranveeraggarwal let's stick with a _build() method that is responsible for the creation of the "visual" components (vtkActors or UIComponents).

@skoudoro skoudoro added this to Widget in Progress in Viz Module Nov 22, 2017

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Dec 1, 2017

The comments from @ranveeraggarwal and @MarcCote concerned code that sets TextBlock2D properties. 5550e4b cleans up this configuration by adding those properties to the TextBlock2D constructor.

@dmreagan dmreagan moved this from UI PR in Progress to UI PR needs a review in Viz Module Dec 1, 2017

@dmreagan dmreagan referenced this pull request Dec 1, 2017

Merged

Enh textblock #1385

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Dec 2, 2017

I think background color for text was added only in VTK 7 (maybe 6.2 but not sure). That's why I manually add a Rectangle2D behind a TextBlock2D when I detect we are not using VTK7.

I don't mind branching from your PR and adding my stuff. I guess that will depend on what we decide in term of which version of VTKs should we support?

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Dec 3, 2017

This looks good. I would still like to move everything to a _build function rather than the constructor, but I guess in this case it'd be an overkill.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Dec 3, 2017

Vertical justification is awesome, we need to merge this with the other PR.

@MarcCote

Tests are failing because Travis only supports VTK5.8 (at least with our current configuration).

@MarcCote MarcCote referenced this pull request Dec 12, 2017

Merged

WIP: ENH: UI Listbox #1355

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 19, 2018

Should we merge this PR with #1385 (which "supports" VTK5.8)?

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Jan 19, 2018

I was just about to try to port your background hack to this PR. But I'm getting stuck trying to set up an environment on Windows with VTK5 right now. I can go ahead and copy that stuff over, but I won't be able to test until I get this environment worked out.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jan 20, 2018

@dmreagan if you are on Windows 10, you can try setting up the environment on Ubuntu on Windows. I have tried it out and it works well for me.

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Jan 26, 2018

@MarcCote I have a VTK 5.10 environment set up that shows the same results as the Travis bot, and I'm struggling to figure out why that test fails. If your tests are passing on #1385, I think you should just go with that one and I'll add the changes from this PR later. This PR is really just cosmetic, so it shouldn't continue to hold up progress for #1385 and #1355.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 27, 2018

@dmreagan I had to re-record the test_ui_text_block on mine. I think the background actually impact the number of clicks. You might want to pick the .npz from my branch and try it out. If that still doesn't work, go ahead and review/merge PR #1385 .

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

I just merged #1385, should we close this one? let me know @dmreagan.

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Mar 5, 2018

Closing this PR. May return to the idea once #1355 and #1441 are merged.

@dmreagan dmreagan closed this Mar 5, 2018

@dmreagan dmreagan removed this from UI PR needs a review in Viz Module Mar 5, 2018

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