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

Enh ui components positioning #1448

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@MarcCote
Contributor

MarcCote commented Mar 7, 2018

This PR supersedes #1441 and exists to reduce changes in #1355.

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

New changes

  • Move the logic for positioning UI component into the abstract class UI.
  • Every UI component needs to define the attribute size and implement _set_position(coords) which is responsible for positioning the lower-left corner of the component (conventional way of positioning actors in VTK).
  • The UI components' constructor all have an optional position argument instead of center.
@codecov-io

This comment has been minimized.

codecov-io commented Mar 7, 2018

Codecov Report

Merging #1448 into master will increase coverage by <.01%.
The diff coverage is 93.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   87.41%   87.41%   +<.01%     
==========================================
  Files         239      239              
  Lines       30579    30380     -199     
  Branches     3291     3263      -28     
==========================================
- Hits        26730    26558     -172     
+ Misses       3078     3060      -18     
+ Partials      771      762       -9
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 86.59% <100%> (+1.16%) ⬆️
dipy/viz/ui.py 89.47% <91.93%> (+0.18%) ⬆️

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 2665eee...a3e4fed. Read the comment docs.

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_components_positioning branch from 954872f to a3e4fed Mar 7, 2018

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Mar 7, 2018

@skoudoro

Apart from a small comment below, it is good for me

@@ -60,10 +54,15 @@ class UI(object):
"""
def __init__(self):

This comment has been minimized.

@skoudoro

skoudoro Mar 7, 2018

Member

size and position are defined in docstring but it is weird to not see them as parameters in the __init__ function. Indeed, they are already in the class docstring.

Why do we not initialize them here?

This comment has been minimized.

@MarcCote

MarcCote Mar 8, 2018

Contributor

Oops some left over code. I originally wanted to initialize these two attributes there (in the parent UI class) but doing so would have called _set_position(coords) of the subclass which might not have created all the needed actors (to position or resize).

This comment has been minimized.

@dmreagan

dmreagan Mar 9, 2018

Contributor

I agree that it would be nice to have these in __init__, but if it's a problem then the other way is fine. I'm ready to approve when these are removed.

This comment has been minimized.

@MarcCote

MarcCote Apr 5, 2018

Contributor

I did the refactoring and created a new PR #1492

@skoudoro

Hi @MarcCote, I think this PR is ready to go. Can you fix the small changes below then we can merge it.

Thanks!

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

This comment has been minimized.

@skoudoro

skoudoro Mar 12, 2018

Member

it must fit within the panel's size

Not necessary, I think you can remove this part of the doc.

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.
position : (float, float)
Absolute coordinates (x, y) in pixels the lower-left corner.

This comment has been minimized.

@nilgoyette

nilgoyette Apr 2, 2018

Contributor

'of the'? In fact, you already wrote a more complete sentence below Absolute coordinates (x, y) of the lower-left corner of this UI component. It would be better to settle for one!

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 5, 2018

I think I made a better version in PR #1492. It introduces more changes but I think it is for the best.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 7, 2018

Closed in favor of #1492.

@MarcCote MarcCote closed this Apr 7, 2018

@MarcCote MarcCote removed this from UI PR needs a review in Viz Module Apr 7, 2018

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