Skip to content
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

Issue26 #100

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from
Open

Issue26 #100

wants to merge 44 commits into from

Conversation

hyoklee
Copy link
Member

@hyoklee hyoklee commented Oct 13, 2015

Preview of tree view. There’s no deep traversal from a node yet so grandparents or grandchildren will not appear.

@jreadey
Copy link
Member

jreadey commented Oct 13, 2015

@hyoklee - I'm getting an import error with your branch. Do you have files not checked in?

 pythonw HDFCompass.py
Traceback (most recent call last):
  File "HDFCompass.py", line 37, in <module>
    from hdf_compass import compass_viewer
  File "/Users/jreadey/Compass/hdf-compass/hdf_compass/compass_viewer/__init__.py", line 34, in <module>
    from . import container, array, keyvalue, image, frame, text
  File "/Users/jreadey/Compass/hdf-compass/hdf_compass/compass_viewer/container/__init__.py", line 33, in <module>
    from .tree import ContainerTree
  File "/Users/jreadey/Compass/hdf-compass/hdf_compass/compass_viewer/container/tree.py", line 17, in <module>
    import compass_model
ImportError: No module named compass_model

@giumas
Copy link
Collaborator

giumas commented Oct 13, 2015

The issue could be the missing namespace 'hdf_compass'..

@hyoklee did you rebase before commit?

@hyoklee
Copy link
Member Author

hyoklee commented Oct 13, 2015

I ran my branch on Python 2.7 only and can't duplicate the error on Mac and PC. John, do you use Python 3.0?

@jreadey
Copy link
Member

jreadey commented Oct 13, 2015

@hyoklee - I'm running Python 2.7. Any easy way to verify is to do a new git clone (in a temp directory somewhere) and grab your own pull request: $git checkout issue26. As @giumas said, you are probably missing recent changes to the develop branch.

@giumas
Copy link
Collaborator

giumas commented Oct 13, 2015

A branch must be rebased before make a PR to the upstream repository. Otherwise your code will be missing all the other changes already applied...

@hyoklee
Copy link
Member Author

hyoklee commented Oct 13, 2015

@jreadey I could duplicate missing module error from fresh pull. I fixed it by committing 4439379.

@jreadey
Copy link
Member

jreadey commented Oct 14, 2015

@hyoklee - I'm able to run it now.
No deep traversal & clicking the "more" link doesn't do anything yet.
Rather than a "more" button can you have additional nodes filled as the user scrolls down?

@hyoklee
Copy link
Member Author

hyoklee commented Oct 14, 2015

@jreadey, your observation is right. I just want to check how treeview should be implemented further with you.

I can try scroll down event binding to add additional nodes but I haven't tried it before so I can't guarantee that it will work.

For deep traversal, do you want to add all parents and their children or just immediate parents up to root?

@jreadey
Copy link
Member

jreadey commented Oct 14, 2015

By deep traversal I was thinking that the user could expand or collapse any group node that has children. We want to stay away from anything that would involve a potentially arbitrary amount of work (e.g. puling in 1000's of nodes).

@giumas
Copy link
Collaborator

giumas commented Oct 17, 2015

@hyoklee Did you notice the conflicts on your branch that stops from merging your PR?

@hyoklee
Copy link
Member Author

hyoklee commented Oct 26, 2015

@jreadey OK. I'll work on improving tree view for the case like tall.h5 as you suggested.

For countries.h5, that's the best I could get on Mac. Mac's scroll down event fires rapidly in succession while Window's event fires one at a time. wxPython's behavior is unpredictable per platform.

@hyoklee
Copy link
Member Author

hyoklee commented Oct 27, 2015

@jreadey I don't know whether it's wxPython problem or hdf_compass problem but the same code behaves differently on Win7 and Mac. See the attached screen shot. The selection() function in tree.py is called only once on Mac/Linux but the same function is called twice on Windows when I open the same file.

hdf_compass_mac_fires_once
hdf_compass_win7_fires_event_twice

@jreadey
Copy link
Member

jreadey commented Oct 28, 2015

This is looking much better (at least on the mac).

Is recursive_walk dead code now? If so, remove it.

I think the on_bottom handler is not doing quite the right thing... Nodes should only be appended if the scroll exposes nodes that have not been exposed before. With the group1k.h5 file if I wiggle the scroll up and down (but stay in the range of say g0000 to g00030) nodes get added on every scroll down even though I'm not seeing any nodes I haven't seen before.

Can the size of the scroll button be made to be proportional to the percent of nodes visible? E.g. if there are 1000 children, and I see 20 nodes, the scroll button should be very small to reflect there's a limited window on the larger set of nodes. (the ArrayFrame class does this with the grid scroll bars). As it is now, I can't scroll down to the end of the nodes if they are off screen by dragging the scroll button.

@hyoklee
Copy link
Member Author

hyoklee commented Oct 28, 2015

I removed recursive part.

I think I addressed your problem by scrolling to the last item added. Please test it again see if it's not what you expected. I also improved tree control for Linux so moving scroll up and down at the end of the tree view can append more items. Thus, "more" is gone for Linux.

Does scroll button appear larger / smaller on your Yosemite mac? I don't see such effect on my Mavericks Mac. You can explain it to me by capturing screenshots.

@giumas
Copy link
Collaborator

giumas commented Oct 28, 2015

@hyoklee I am curious about ID_VIEW_MENU_GRAPH: https://github.com/HDFGroup/hdf-compass/blob/issue26/hdf_compass/compass_viewer/container/frame.py#L60
What will it display? How will it be different from ID_VIEW_MENU_TREE?

@hyoklee
Copy link
Member Author

hyoklee commented Oct 29, 2015

@giumas That's provisional to handle loops among nodes. Tree view is not sufficient for HDF5 in general.

@jreadey
Copy link
Member

jreadey commented Oct 29, 2015

@hyoklee - see the screenshot here:
screen shot 2015-10-29 at 8 54 47 am

The scrollbar on the right is taking 80% of the length even though only a small fraction of the links are displayed.

@hyoklee
Copy link
Member Author

hyoklee commented Oct 29, 2015

@Jready I see what you mean now. I can't control the size of scroll bar. There are only 20 items on the tree view and next 20 will be loaded when you page down the scroll bar. Thus, 80% size is right (17/20).

Please let me know if you want to load all 1000K nodes from the beginning like Icon/List/Grid view does. Then, you'll get a small scroll bar.

@jreadey
Copy link
Member

jreadey commented Oct 29, 2015

@hyoklee - The List view is using wx.LC_VIRTUAL so it can handle an arbitrary number of items without actually loading them in memory. Does the TreeCtl have equivalent functionality?

If not, you can explore expanding the number of nodes you load on activate - say 1000. That way it should load reasonably fast and have a scroll button that is at the minimal size (I think). Next when the user scrolls to the bottom you can load an additional 1000 nodes and so on.

@hyoklee
Copy link
Member Author

hyoklee commented Oct 29, 2015

@jreadey No, tree doesn't have LC_VIRTUAL as far as I know. From Oct. 2 email exchange regarding tree control, @mfolk suggested to set the default limit to 20 so I followed his suggestion. If you like 1,000, you can easily change it by modifying the code self.limit=20 to self.limit=1,000 in tree.py.

I think the limiting number should be an user option. What would be the best way to save such preference in Compass?

@jreadey
Copy link
Member

jreadey commented Oct 29, 2015

I don't think it should be a user option - experiment and see which works best. 20 is too small because scrolling to the bottom is not very effective & the scroll button size is too large. A 1000 may take some time to load - see how it goes.

Regardless, we do want to enable the user to see all the links regardless of low many there are. I.e. not the HDFView-style limitation on the number of links.

@giumas giumas mentioned this pull request Aug 5, 2016
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants