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 File Menu element to viz.ui #1592

Merged
merged 2 commits into from Aug 22, 2018

Conversation

Projects
5 participants
@karandeepSJ
Copy link
Contributor

karandeepSJ commented Jul 24, 2018

This PR reopens #1570
Added a FileMenu2D that uses a ListBox2D to display all files and directories in the file system. Files are shown with Blue color and directories with Green. Clicking on a directory will take the user inside it.
filemenu

@skoudoro skoudoro added the gsoc2018 label Jul 24, 2018

@dmreagan dmreagan added this to PR needs a review in Viz Module Jul 26, 2018

@dmreagan dmreagan requested a review from MarcCote Jul 26, 2018

Parameters
----------
extensions: list(string)
List of extensions to be shown as files.

This comment has been minimized.

@dmreagan

dmreagan Jul 26, 2018

Contributor

This needs to be more descriptive. What format is it expecting the list to be in? What should I put if I want all files to be shown?

Container for the menu.
"""

def __init__(self, extensions, directory_path, position=(0, 0),

This comment has been minimized.

@dmreagan

dmreagan Jul 26, 2018

Contributor

extensions should default to showing all files

break

file_names = []
if "*" in self.extensions:

This comment has been minimized.

@dmreagan

dmreagan Jul 26, 2018

Contributor

It should also probably treat an empty string "" the same as "*". Then it can accept None for directories-only.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 28, 2018

Codecov Report

Merging #1592 into master will decrease coverage by <.01%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
- Coverage   87.34%   87.34%   -0.01%     
==========================================
  Files         246      246              
  Lines       31811    31945     +134     
  Branches     3451     3473      +22     
==========================================
+ Hits        27785    27902     +117     
- Misses       3204     3214      +10     
- Partials      822      829       +7
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 83.61% <82.5%> (-0.12%) ⬇️
dipy/viz/ui.py 89% <86.45%> (+0.1%) ⬆️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 393350c...cd28e16. Read the comment docs.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:FileMenu branch from 06c6ea2 to 3b59425 Aug 1, 2018

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:FileMenu branch from 3b59425 to cd28e16 Aug 3, 2018

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 9, 2018

Looks good. Any thoughts, @ranveeraggarwal and @MarcCote ?

@MarcCote
Copy link
Contributor

MarcCote left a comment

I've added some of my concerns but it seems to work.

# 4. Shift + Click on 'test6.txt'.
# 5. Click on '../'.
# 2. Click on 'testfile.txt'.
show_manager.play_events_from_file(recording_filename)

This comment has been minimized.

@MarcCote

MarcCote Aug 9, 2018

Contributor

Where does the recording file come from? I mean how do you record it? I don't see any EventCounter.save.

This comment has been minimized.

@karandeepSJ

karandeepSJ Aug 10, 2018

Contributor

The events are already recorded and saved. I removed the recording part because in the earlier implementation, there was no interactive parameter and if a user wanted to interact with the elements, they would have to set recording = True which changes the log files.
Maybe we can have two parameters recording and interactive?

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

That's a good idea. We could have a single parameter mode=None|'interactive|'recording'. I found the recording useful when we change some core functionality of the UI framework and have to redo the recording. But to be fair that happens less often nowadays.

This comment has been minimized.

@karandeepSJ

karandeepSJ Aug 10, 2018

Contributor

I'll do that in another PR since all the tests will have to be changed.
@dmreagan @Garyfallidis @ranveeraggarwal should I go ahead with this?

This comment has been minimized.

@MarcCote

MarcCote Aug 10, 2018

Contributor

Fair enough.

@@ -3230,8 +3231,10 @@ def _setup(self):
vertical_justification="middle")

# Add default events listener for this UI component.
self.textblock.on_left_mouse_button_clicked = self.left_button_clicked
self.background.on_left_mouse_button_clicked = self.left_button_clicked
self.add_callback(self.textblock.actor, "LeftButtonPressEvent",

This comment has been minimized.

@MarcCote

MarcCote Aug 9, 2018

Contributor

Is this really necessary, we should avoid directly adding callbacks to VtkProp whenever is possible.

self.scroll_callback)

# Handle mouse wheel events on the slots.
for slot in self.listbox.slots:

This comment has been minimized.

@MarcCote

MarcCote Aug 9, 2018

Contributor

Still not a fan of reimplementing things that are already in ListBox2D (see #1570 (comment) and #1570 (comment)) but I don't have time to think it through and already block you for long enough.

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Aug 14, 2018

I agree that this one has been held up long enough. I'll merge in 48 hours if there are no further objections. @MarcCote can you make an issue with your remaining concerns so we can remember to address them later?

@MarcCote MarcCote referenced this pull request Aug 15, 2018

Closed

viz.ui.FileMenu2D #1618

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Aug 15, 2018

@dmreagan sure (see #1618).

@dmreagan dmreagan merged commit 641f3a2 into nipy:master Aug 22, 2018

1 of 3 checks passed

codecov/patch 85.29% of diff hit (target 87.34%)
Details
codecov/project 87.34% (-0.01%) compared to 393350c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from PR needs a review to Done Aug 22, 2018

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