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

Added scroll bar to ListBox2D #1564

Merged
merged 8 commits into from Sep 11, 2018

Conversation

Projects
7 participants
@karandeepSJ
Copy link
Contributor

karandeepSJ commented Jun 14, 2018

This PR adds a scroll bar to the ListBox2D in viz.ui. The scroll bar can be moved at different speeds with the mouse. It also moves up or down when the scrolling occurs through the mouse wheel.
scroll

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jun 14, 2018

Hello @karandeepSJ, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 23, 2018 at 01:01 Hours UTC

@dmreagan dmreagan requested review from dmreagan and ranveeraggarwal Jun 14, 2018

@dmreagan dmreagan added this to PR needs a review in Viz Module Jun 14, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jun 14, 2018

Hi @karandeepSJ, Thanks for this cool features! Can you check this error on travis?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #1564 into master will increase coverage by <.01%.
The diff coverage is 90.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1564      +/-   ##
==========================================
+ Coverage   87.34%   87.35%   +<.01%     
==========================================
  Files         246      246              
  Lines       32266    32319      +53     
  Branches     3504     3509       +5     
==========================================
+ Hits        28184    28231      +47     
- Misses       3247     3250       +3     
- Partials      835      838       +3
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 81.83% <46.15%> (-0.31%) ⬇️
dipy/viz/ui.py 89.31% <97.64%> (+0.1%) ⬆️

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 606739b...d869a6f. Read the comment docs.

# Check if the right values were selected.
expected = [[1], [2], [2], [21], [1], [42]]
assert len(selected_values) == len(expected)
assert_arrays_equal(selected_values, expected)

This comment has been minimized.

@skoudoro

skoudoro Jun 14, 2018

Member

Why are you removing this test?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 15, 2018

Same here @karandeepSJ . Needs rebasing.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jun 15, 2018

@karandeepSJ please resolve the conflicts of this PR.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from e796da7 to 25c2e24 Jun 16, 2018

@MarcCote MarcCote self-requested a review Jun 17, 2018

@skoudoro skoudoro added the gsoc2018 label Jun 26, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 24, 2018

Hi @karandeepSJ,

As requested before, can you rebase this PR? Thank you

@MarcCote @dmreagan @ranveeraggarwal, Can it be merged after rebasing? any other comments?

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from 25c2e24 to 5ee3752 Jul 24, 2018

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Jul 26, 2018

This looks pretty nice, but I've observed some small quirks in the scrollbar behavior. I found that if you drag the bar downward in small motions, it scrolls up instead of down. The reverse is also true. I could only trigger this behavior when the scrollbar is large.

scrollbar_bug

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from c25e39e to 31c93b5 Aug 6, 2018

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 9, 2018

The bug is fixed, so this PR looks good to me. How difficult would it be to change the color of the bar when it is grabbed like we do with sliders? It could wait for another PR. Any thoughts here, @ranveeraggarwal and @MarcCote ?

@MarcCote
Copy link
Contributor

MarcCote left a comment

The scrollbar looks awesome. Thanks @karandeepSJ

@@ -539,13 +539,38 @@ def test_ui_range_slider(interactive=False):

@npt.dec.skipif(not have_vtk or skip_it)
@xvfb_it
def test_ui_listbox_2d(recording=False):
def test_ui_listbox_2d(interactive=False):

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

Like we discussed in PR #1559 , we may want a mode parameter that can either accept None, interactiveorrecording`.

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

This should be done in a new PR to refactor the tests.

pos = (pos[0], self.up_button.size[1] // 2 + margin)
self.panel.add_element(self.down_button, pos, anchor="center")
# arrow_up = read_viz_icons(fname="arrow-up.png")
# self.up_button = Button2D([("up", arrow_up)])

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

Oh, let's remove those commented lines.

item.textblock.font_size = font_size
item.textblock.color = (0, 0, 0)
self.slots.append(item)
self.panel.add_element(item, (x, y + margin))

# Add default events listener for this UI component.
self.up_button.on_left_mouse_button_pressed = self.up_button_callback
self.down_button.on_left_mouse_button_pressed = self.down_button_callback
# self.up_button.on_left_mouse_button_pressed = self.up_button_callback

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

Remove commented lines.

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Aug 10, 2018

@dmreagan This is how it looks like when the color of the scroll bar changes on grabbing it. Currently, I have only made it darker.

scroll_color

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Aug 11, 2018

@dmreagan @skoudoro @Garyfallidis There were some connection problems during the travis build of this PR and of #1559.

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 14, 2018

Travis seems happy, and so am I. I'll merge this in 48 hours if there are no objections.

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 22, 2018

@karandeepSJ please rebase

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from 88ca252 to 546d594 Aug 23, 2018

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Aug 23, 2018

Rebased. To adjust the scroll bar size and step length on changing a directory, I added a function update_scrollbar to ListBox2D. I also added the update_element function to Panel2D to update the position of the scroll bar in the panel

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 23, 2018

@MarcCote please take a look at @karandeepSJ's latest update. It looks like update_element is repeating a lot of code from add_element. Is there a better way to do this?

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Aug 28, 2018

@dmreagan @karandeepSJ we could refactor the update_element code like this

    
    def remove_element(self, element):
        idx = self._elements.index(element)
        del self._elements[idx]
        del self.element_offsets[idx]

    def update_element(self, element, coords, anchor="position"):
        """ Updates the position of a UI component in the panel.

        Parameters
        ----------
        element : UI
            The UI item to be updated.
        coords : (float, float) or (int, int)
            New coordinates.
            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.
        """
        self.remove_element(element)
        self.add_element(element, coords, anchor)

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from 546d594 to e49d601 Sep 6, 2018

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ScrollBar branch from e49d601 to 6481c76 Sep 8, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 8, 2018

Hi @karandeepSJ, Any chance to have the improvement above proposed by @MarcCote before merging this PR?

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Sep 8, 2018

Yes, I am working on it

@karandeepSJ

This comment has been minimized.

Copy link
Contributor

karandeepSJ commented Sep 8, 2018

Done. Also, there was an error being thrown when I tried to open a directory without read permissions from the filemenu. I got rid of the error. Now the user just stays in the same directory

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 8, 2018

Great! It is ready to be merged. Can you do it @dmreagan?

Thank you @karandeepSJ!

@dmreagan dmreagan merged commit 4cad756 into nipy:master Sep 11, 2018

3 checks passed

codecov/patch 90.81% of diff hit (target 87.34%)
Details
codecov/project 87.35% (+<.01%) compared to 606739b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from PR needs a review to Done Sep 11, 2018

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