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

WIP: ENH: UI Listbox #1355

Merged
merged 3 commits into from Jun 13, 2018

Conversation

Projects
8 participants
@MarcCote
Copy link
Contributor

MarcCote commented Oct 13, 2017

I've refactored @ranveeraggarwal's code for the FileSelectMenu2D and made a ListBox that should be more flexible.

This new component supports multiselection via ctrl+click and shift+click. Also, the example in viz_ui.py shows how to hook a callback function such that it is called whenever the list selection changes.

What's missing is making FileSelectMenu2D use ListBox2D.

Here's a small demo:
listbox
EDIT: Updated demo.

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@e1422d7). Click here to learn what that means.
The diff coverage is 90.41%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1355   +/-   ##
=========================================
  Coverage          ?   87.29%           
=========================================
  Files             ?      246           
  Lines             ?    31458           
  Branches          ?     3239           
=========================================
  Hits              ?    27461           
  Misses            ?     3158           
  Partials          ?      839
Impacted Files Coverage Δ
dipy/viz/interactor.py 96.93% <100%> (ø)
dipy/viz/tests/test_ui.py 87.19% <83.33%> (ø)
dipy/viz/ui.py 90.79% <91.66%> (ø)

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 e1422d7...905b918. Read the comment docs.

self.add_callback(self.up_button.actor, "LeftButtonPressEvent",
self.up_button_callback)
self.add_callback(self.down_button.actor, "LeftButtonPressEvent",
self.down_button_callback)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Oct 17, 2017

Member

In other UI elements I have separated the event handling logic in a separate function. This is just to keep the constructor a little clean.

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented Oct 17, 2017

This is awesome! We can now remove FileSelectMenuText2D and modify FileSelectMenu2D to use this - once this is merged.
This PR looks good to me apart from that one small nitpick.

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented Oct 17, 2017

On a further thought, FileSelectMenu2D is just a very specific use case of the ListBox2D, so we can remove it entirely and build a FileMenu element (a composite element) that uses the ListBox2D element with functions of the existing FileSelectMenu2D.

@skoudoro skoudoro added this to WIP in Dipy 0.14.0 Nov 8, 2017

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

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Dec 12, 2017

Waiting after PR #1385 and PR #1362

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Mar 4, 2018

Rebased on top of #1441.

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Mar 4, 2018

I've updated the demo gif (see the first post).

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

@ranveeraggarwal
Copy link
Member

ranveeraggarwal left a comment

@dmreagan @MarcCote shall we merge this in?
This looks good to me.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Mar 6, 2018

Let me test it quickly on windows @ranveeraggarwal. I will do it right now

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Mar 6, 2018

and should we merge #1441 first?

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented Mar 6, 2018

Yeah, that makes sense @skoudoro

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Mar 6, 2018

After playing with this UI, it works like a charm, +1 for merging when it is possible

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Mar 6, 2018

I'm having some hardware problems, so I'm not sure when I'll be able to look at this. If you all approve, go ahead and merge without me.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 2, 2018

Needs rebase and update @MarcCote !

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_listbox branch from 483c9ea to 5e842f1 Jun 4, 2018

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jun 4, 2018

Hello @MarcCote, Thank you for updating !

Line 2263:81: E501 line too long (81 > 80 characters)

Comment last updated on June 11, 2018 at 21:04 Hours UTC
@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 7, 2018

@MarcCote can you fix the pep8 issues?

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Jun 10, 2018

I have added a scroll bar to this.
scroll

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Jun 10, 2018

@karandeepSJ that looks awesome. I'll finish this PR and you could make a small PR for it (if not already done).

@@ -205,7 +207,7 @@ def _print_nb_selected_elements():
show_manager.ren.azimuth(30)

# Uncomment this to start the visualisation
# show_manager.start()
show_manager.start()

This comment has been minimized.

@dmreagan

dmreagan Jun 11, 2018

Contributor

Tiny nitpick to re-comment this line. Otherwise, this looks good to go.

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Jun 13, 2018

thanks @MarcCote now let's put back the FileSelectMenu

@dmreagan dmreagan merged commit 0bfa3ac into nipy:master Jun 13, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

Viz Module automation moved this from PR needs a review to Done Jun 13, 2018

@MarcCote MarcCote deleted the MarcCote:enh_ui_listbox branch Jun 14, 2018

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

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