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

[SVCS-580] Collapsible Zip Rendering #331

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

Conversation

cslzchen
Copy link
Contributor

@cslzchen cslzchen commented Apr 16, 2018

Ticket

https://openscience.atlassian.net/browse/SVCS-580

Purpose

Replaces: #299

Improve our zip rendering by adding a neater collapsible hierarchical structure.

Changes

Uses two new JavaScript libraries: jstree and jstree-table. The core algorithm is building the tree from a list of paths.

CR Notes

Most of the files are assets. Please focus on mfr/extensions/zip/render.py and mfr/extensions/zip/templates/viewer.mako for core functionality. The tests and fixtures also helps.

All image assets are optimized so they took 15% less than the original PR. No changes to any scripts and css files from the new jstree and jstree-table library. On exception is the style change for mfr/extensions/zip/static/js/jstreetable.js which was "eslint"ed due to min version not available.

Side effects

No

QA Notes

Upload a complex zip file and click through it contents. You may discover some hidden system files depending on what OS and what zip app you use which is expected.

Deployment Notes

TBD

@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage increased (+0.6%) to 71.776% when pulling 098aa90 on cslzchen:feature/improve-zip-rendering into bf14694 on CenterForOpenScience:develop.

Copy link
Contributor Author

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felliott Ready for CR 🔥

I only pre-reviewed the tree and list structures, and the tree building process. Hopefully this helps.

@@ -0,0 +1,22 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of the filename for each file in ZipFile().filelist. It is confusing since the filename is the full path for a file or a folder. In the code in render.py, I try to make this less confusing by using obj for var names.

@@ -0,0 +1,169 @@
{
"test_file_tree": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of a fully built obj_tree. Due to the requirement of the library we use, the tree is a list which contains only the root node: [root,]. Each node in the tree is a dictionary with four keys: icon, text, data and children. text is the file/folder's name and children is a list of child nodes.


return self.TEMPLATE.render(data=obj_tree, base=self.assets_url)

def obj_list_to_tree(self, obj_list: list) -> List[dict]:
Copy link
Contributor Author

@cslzchen cslzchen Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the comments in the test fixtures for the structure of obj_list and obj_tree. Hopefully the in-code comments in this method provides enough information.

I'd like to mention one quirk in the tree building process. Given an object list of ['A/ab.png', 'A/', ], the algorithm first sees the object 'A/ab.png'. It tries to build the tree by adding node A/ to the root's children list and then adding ab.png to A's children list. However, at this time we don't have details (icon, data) of A/. The solution is to build the structure only in this iteration and update the details in later iterations when A/ is the object.

The best part of this solution is only one pass is needed for building the full tree and update node details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an axample of fixture to the docstring?

Johnetordoff and others added 8 commits August 24, 2018 15:46
Rename image names with shorter length and use dashes and underscores
consistently. Optimize images which saves 15% space in total.
- Update code for previous assets refactor
- Improve import styles and naming for vars/funcs
- Add a sanitizer to remove system files/folders
- Update docstrings, comments and TODOs

Note: there is a bug that the tree building algorithm only works when
      files in the zip file are sorted in increasing alphabetic order
- Handles the case when the generated ``filelist`` is not in order.
- The outer loop: iterate through each file in the list and break the
  full path into segments.
- The inner loop: iterate through each segment, add a new node if not
  found in the child nodes of its parent and update the node with the
  file's details if the segment is the last one.
- The full tree is complete at the end of only a single pass.
- Remove redundant and not-working customization that modifies the
  "jstree-table-wrapper" style. "jstreetable.js" doesn't provide any
  minimum version so that MFR has to use the full one. The file has
  been refactored with eslint es5 plugin and better docstrings.
- Minor updates for the renderer:
  - Enable cache which was disabled for local test
  - Update column width for file size
  - Set `is_folder` optional (`True` as default)
  - Use closed-folder icon
  - Remove unused assets
With a sorted list, new node can be created and updated at the same
time. For example, when adding "/root/a/b/c" to the tree, node "a/"
and node "b/" must exist. Just find the sibling list of node "b/"
and add "c" to the list.
|- root-folder.zip
   |- root-folder/
      |- folder_1/
         |- file_1.a
      |- file_2.b
In addition, remove left-over debug statements
@cslzchen cslzchen force-pushed the feature/improve-zip-rendering branch from 3ff6097 to 098aa90 Compare August 24, 2018 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants