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

VIZ: A lightweight UI for medical visualizations #6: 2D File Selector #1262

Merged
merged 31 commits into from Oct 11, 2017

Conversation

Projects
None yet
6 participants
@ranveeraggarwal
Member

ranveeraggarwal commented Jun 17, 2017

Continuing from #1222 and the sixth installment of #1111, this PR adds a File Selector element to the Viz module.

ToDo

  • Fix events
  • Write tests
  • Find and fix PEP8 issues
  • Record new tests

@ranveeraggarwal ranveeraggarwal changed the title from A lightweight UI for medical visualizations #6: 2D File Selector to WIP: A lightweight UI for medical visualizations #6: 2D File Selector Jun 17, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jun 17, 2017

Codecov Report

Merging #1262 into master will decrease coverage by 0.04%.
The diff coverage is 88.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1262      +/-   ##
==========================================
- Coverage   87.02%   86.98%   -0.05%     
==========================================
  Files         228      228              
  Lines       28852    29068     +216     
  Branches     3101     3131      +30     
==========================================
+ Hits        25108    25284     +176     
- Misses       3038     3072      +34     
- Partials      706      712       +6
Impacted Files Coverage Δ
dipy/viz/interactor.py 98.12% <100%> (+0.07%) ⬆️
dipy/viz/tests/test_ui.py 83.53% <81.81%> (-1.7%) ⬇️
dipy/viz/ui.py 90.24% <89%> (-2.11%) ⬇️

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 33c78bd...622e37e. Read the comment docs.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jun 20, 2017

@MarcCote can you clone this and see if it works for you?

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jun 22, 2017

Okay, this is really weird.

  1. The FileSelectMenu2D panel isn't moving like it's supposed to (on click and drag) and the events on the FileSelectMenu2D (again, click and drag) are getting carried over to the cubes behind (except for the up and down buttons).
  2. The text actors for the file menu are unresponsive despite adding callbacks.

I don't get why this is happening.

@MarcCote @dmreagan any ideas here?

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Jun 23, 2017

@ranveeraggarwal I can confirm that the panel and text actors show the behavior you describe on my system as well (Windows 10, Python 3, VTK 7.1). I don't have any leads on a solution yet, though.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jul 11, 2017

@MarcCote @dmreagan the events have been fixed, do have a look at the file menu behavior.

@coveralls

This comment has been minimized.

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.093% when pulling 584a383 on ranveeraggarwal:ui into 736cbb5 on nipy:master.

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@nipy nipy deleted a comment from coveralls Jul 25, 2017

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 26, 2017

I've started looking at this PR. Really awesome 👍 . Here are some weird/missing behaviors I've found while playing with the example viz_ui.py. I'll do a more thorough review of the code later (hopefully by tomorrow).

The filedialog (this is also true for the UI Panel) needs to be clicked once before we are able to move it around. This happens with VTK 5.8 and VTK 6.0.0. With VTK 7.0.0, the clicked events are buggy as you mentioned.

Would it be easy to support scrolling with the mouse wheel?

Is it easy to support long click on the "scroll bar" arrows?

I think in the example we should be able to see when a file is selected. Can we change the text color? Or put a rectangle of a different color underneath it? Maybe for the moment, we could just display it somewhere (console or use another Textbox). Edit: Ok, I see the coloring seems to only work with VTK > 6.2. I still think we should find a way of showing the selected file. Do you know if we can put a border around a text actor?

It will be nice to be able to manually specify the current directory of the filedialog. Speaking of the current directory, I don't think we should modify the actual "current directory" of the program (which happens when using os.chdir). We should handle the current directory with an internal variable and fetch the files/folders using something similar to glob.glob(self.current_directory).

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jul 31, 2017

The filedialog (this is also true for the UI Panel) needs to be clicked once before we are able to move it around. This happens with VTK 5.8 and VTK 6.0.0. With VTK 7.0.0, the clicked events are buggy as you mentioned.

This is indeed weird and shouldn't happen. I will look into this.

Would it be easy to support scrolling with the mouse wheel?

Not sure. I guess we'll need to have an invisible(?) rectangle between the two arrows and implement a scroll listener on it?

Is it easy to support long click on the "scroll bar" arrows?

Again, we'll need to look into this. We'll also need to have a variable to determine the speed of scrolling (or, increase the speed as the button is kept pressed).

I think in the example we should be able to see when a file is selected. Can we change the text color? Or put a rectangle of a different color underneath it? Maybe for the moment, we could just display it somewhere (console or use another Textbox). Edit: Ok, I see the coloring seems to only work with VTK > 6.2. I still think we should find a way of showing the selected file. Do you know if we can put a border around a text actor?

I am currently storing it in a variable, so it should be easy to display it. Adding a border? Let me see if that can happen. Worst case, we can hide/unhide a rectangle actor under the text.

I'll also need to work a bit more on the tests, the default way won't work.

@coveralls

This comment has been minimized.

coveralls commented Aug 21, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.068% when pulling 2abfc48 on ranveeraggarwal:ui into c268bd2 on nipy:master.

@@ -36,6 +36,13 @@ def abort(self):
""" Aborts the event i.e. do not propagate it any further. """
self._abort_flag = True
def done(self):

This comment has been minimized.

@dmreagan

dmreagan Aug 25, 2017

Contributor

I suggest renaming this to something like reset. Event's other methods are verbs.

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

I agree, reset would be a better name.

@nipy nipy deleted a comment from coveralls Sep 1, 2017

@nipy nipy deleted a comment from coveralls Sep 1, 2017

@nipy nipy deleted a comment from coveralls Sep 1, 2017

@MarcCote

Some last comments. Should be good after that.

class FileSelectMenu2D(UI):
""" A menu to select files in the current folder.
Can go to new folder, previous folder and select a file and keep in a variable.

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

typo: keep it in a variable.

Current selected file.
text_item_list: list(:class:`FileSelectMenuText2D`)
List of FileSelectMenuText2Ds - both visible and invisible.
window: int

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

maybe offset or window_offset ?

List of FileSelectMenuText2Ds - both visible and invisible.
window: int
Used for scrolling.
Basically, tells you the index of the first visible

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

No need to put "Basically".

"""
def __init__(self, size, font_size, position, parent,
extensions, line_spacing=1.4):

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

I think we should be able to provide the "current" folder.

@@ -36,6 +36,13 @@ def abort(self):
""" Aborts the event i.e. do not propagate it any further. """
self._abort_flag = True
def done(self):

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

I agree, reset would be a better name.

# Allot file names as in the above list
i = 0
for file_name in clipped_file_names:

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

Use for i, file_name in enumerate(clipped_file_names): instead.

"""
# A list of file names with extension in the current directory
file_names = []
for extension in self.extensions:

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

This should be tested to make sure it returns only files with the desired extensions. Thinking about it, maybe these methods (that one and get_directory_names) should be standalone functions in some utils module and tested separately.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Sep 27, 2017

Member

Which file in the utils module should this go into? Or do I create another file?

This comment has been minimized.

@MarcCote

MarcCote Sep 28, 2017

Contributor

Hmm, I thought more about it and we don't have to test the behavior of glob :P. It should be fine like this.

self.file_type = ""
self.file_select = file_select

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

Remove empty lines that don't help readability.

position: (float, float)
The text position (x, y) in pixels.
color: (float, float, float)
Values must be between 0-1.

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

Mention it is a RGB color.

return [self.text_actor.get_actor()]
def set_attributes(self, file_name, file_type):
""" This function is for use by a FileSelectMenu2D to set the

This comment has been minimized.

@MarcCote

MarcCote Sep 27, 2017

Contributor

PEP8: short title on the first line.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 2, 2017

@MarcCote I have addressed your comments.
We will fix the tests (re-record and note down) through a different PR.
Do let me know if there's anything else to be done for this one.

@MarcCote

Other than the small comment, looks good to me.

file_select_menu.add_callback(text_ui.text_actor.get_actors()[0],
with InTemporaryDirectory() as tmpdir:
for i in range(10):
_ = open("test" + str(i) + ".txt", 'wt').write('some text')

This comment has been minimized.

@MarcCote

MarcCote Oct 5, 2017

Contributor

What does t mean in wt?

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Oct 5, 2017

Member

t is text mode. We could do without it - we're not using the file contents to test anyway.

This comment has been minimized.

@MarcCote

MarcCote Oct 5, 2017

Contributor

Oh, good to know. It is fine, then.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 5, 2017

@Garyfallidis, @dmreagan this is all good on my end. Let me know if we can go ahead with the merge.

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Oct 6, 2017

I see a few problems here:

  1. If you click on a file name, then scroll up or down, the file no longer appears selected.
  2. No scrollbar?
  3. If you scroll down so that ../ is no longer visible, then click on a directory name to enter it, ../ will not be visible in that lower directory, so you cannot return to the parent.
  4. I'd like to see an example of using the menu in viz_ui.py. Right now you can select a file in the menu, but it doesn't affect anything outside of itself. You should be able to select a file and assign it to some variable in the example, for instance. Then you could print the filename or show it in a text block. That would probably require select and cancel buttons like a proper dialog.
@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 7, 2017

@dmreagan some of these are bugs I need to work on before merging, other enhancements.

If you click on a file name, then scroll up or down, the file no longer appears selected.

Yep, this is a persistence bug. Just got to know, working on it.

No scrollbar?

In the future, as an enhancement maybe?

If you scroll down so that ../ is no longer visible, then click on a directory name to enter it, ../ will not be visible in that lower directory, so you cannot return to the parent.

This is a serious bug that needs to be explored. On it.

I'd like to see an example of using the menu in viz_ui.py. Right now you can select a file in the menu, but it doesn't affect anything outside of itself. You should be able to select a file and assign it to some variable in the example, for instance. Then you could print the filename or show it in a text block. That would probably require select and cancel buttons like a proper dialog.

This will be the next PR. Menus like save/open are very common, so we will add them in the default API.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 7, 2017

@dmreagan thanks for reporting the bugs. I have fixed 1, 3. 2 is an enhancement we can work on later and the PR for 4 is up next.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 7, 2017

Nice. While I was reviewing the changes I asked myself "why is the name of the class FileSelectMenu2D?" Are we planning on having 3D file dialog?

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 7, 2017

@MarcCote I don't think that's happening soon. But this is just in line with all the other 2D elements -- a naming convention of sorts. This distinguishes it from the 3D elements and tells you that this is not to be used in a 3D sense.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2017

One of the bots is failing because the PR hasn't been rebased. Please do rebase.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Oct 11, 2017

@Garyfallidis Garyfallidis merged commit 2d0a115 into nipy:master Oct 11, 2017

3 checks passed

codecov/patch 88.31% of diff hit (target 87.02%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +1.28% compared to 33c78bd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request nipy#1262 from ranveeraggarwal/ui
VIZ: A lightweight UI for medical visualizations nipy#6: 2D File Selector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment