Rewrite the render.py for zip for better clarity

- Update code for previous assets refactor
- Improve import styles and naming for vars/funcs
- Add a sanitizer to remove system files/folders
- Update docstr, comments and TODO
- Discover a bug that the tree building algorithm
  only works when files in the zip file are sorted
  in increasing alphabetic order

Author: cslzchen

mfr/extensions/zip/render.py

@@ -1,79 +1,146 @@
 import os
-import zipfile
+from typing import List
+from zipfile import ZipFile
 
 from mako.lookup import TemplateLookup
 
-from mfr.core import extension
 from mfr.core.utils import sizeof_fmt
+from mfr.core.extension import BaseRenderer
 
 
-class ZipRenderer(extension.BaseRenderer):
+class ZipRenderer(BaseRenderer):
 
     TEMPLATE = TemplateLookup(
         directories=[
             os.path.join(os.path.dirname(__file__), 'templates')
         ]).get_template('viewer.mako')
 
+    @property
+    def file_required(self):
+        return True
+
+    @property
+    def cache_result(self):
+        return False
+
     def render(self):
-        zip_file = zipfile.ZipFile(self.file_path, 'r')
-        files = [file for file in zip_file.filelist if not file.filename.startswith('__MACOSX')]
 
-        data = self.filelist_to_tree(files)
+        zip_file = ZipFile(self.file_path, 'r')
+
+        file_list = self.sanitize_file_list(zip_file.filelist)
+        file_tree = self.file_list_to_tree(file_list)
+
+        return self.TEMPLATE.render(data=file_tree, base=self.assets_url)
 
-        return self.TEMPLATE.render(data=data, base=self.assets_url)
+    def file_list_to_tree(self, file_list: list) -> List[dict]:
+        """Build the file tree and return a "tree".
 
-    def filelist_to_tree(self, files):
+        TODO: Fix this algorithm
+        This algorithm only works when the ``file_list`` are in strict alphabetical order. Here is
+        an example file A.zip where list 1 fails while list 2 succeed.
 
-        self.icons_url = self.assets_url + '/img'
+        A.zip
+        --- A/
+            --- A/aa.png
+            --- B/ab.png
 
-        tree_data = [{
+        File list 1: [ A/, A/B/, A/A/, A/A/aa.png, A/B/ab.png, ]
+
+        File list 2: [ A/, A/A/, A/A/aa.png, A/B/, A/B/ab.png, ]
+
+        :param file_list: the sanitized file list
+        :rtype: ``List[dict]``
+        :return: a "tree" in form of a list which contains one dictionary as the root node
+        """
+
+        icons_url = self.assets_url + '/img'
+
+        # Build the root of the file tree
+        tree_root = [{
             'text': self.metadata.name + self.metadata.ext,
-            'icon': self.icons_url + '/file_extension_zip.png',
+            'icon': icons_url + '/file-ext-zip.png',
             'children': []
         }]
 
-        for file in files:
-            node_path = tree_data[0]
+        # Iteratively build the file tree for each file and folder.egments.
+        for file in file_list:
+
+            node_path = tree_root[0]
+
+            # Split the full path into segments, add each path segment to the tree if the segment
+            # doesn't already exist.  The segments can be either a folder or a file.
             paths = [path for path in file.filename.split('/') if path]
             for path in paths:
-                if not len(node_path['children']) or node_path['children'][-1]['text'] != path:
-                    # Add a child
-                    new_node = {'text': path, 'children': []}
 
-                    if new_node['text']:  # If not a placeholder/"root" directory.
-                        date = '%d-%02d-%02d %02d:%02d:%02d' % file.date_time[:6]
-                        size = sizeof_fmt(int(file.file_size)) if file.file_size else ''
+                # Add a child to the node
+                if not len(node_path['children']) or node_path['children'][-1]['text'] != path:
 
-                        # create new node
-                        new_node['data'] = {'date': date, 'size': size}
+                    new_node = {'text': path, 'children': []}
 
-                        if file.filename[-1] == '/':
-                            new_node['icon'] = self.icons_url + '/folder.png'
+                    date = '%d-%02d-%02d %02d:%02d:%02d' % file.date_time[:6]
+                    size = sizeof_fmt(int(file.file_size)) if file.file_size else ''
+                    new_node['data'] = {'date': date, 'size': size}
+
+                    if file.filename[-1] == '/':
+                        new_node['icon'] = icons_url + '/folder.png'
+                    else:
+                        ext = os.path.splitext(file.filename)[1].lstrip('.')
+                        if ext:
+                            ext = ext.lower()
+                        if self.icon_exists_for_type(ext):
+                            new_node['icon'] = '{}/file-ext-{}.png'.format(icons_url, ext)
                         else:
-                            ext = os.path.splitext(file.filename)[1].lstrip('.')
-                            if check_icon_ext(ext):
-                                new_node['icon'] = \
-                                    self.icons_url + '/file_extension_{}.png'.format(ext)
-                            else:
-                                new_node['icon'] = self.icons_url + '/generic-file.png'
+                            new_node['icon'] = '{}/file-ext-generic.png'.format(icons_url)
 
-                        node_path['children'].append(new_node)
+                    node_path['children'].append(new_node)
 
                     node_path = new_node
+                # Go one level deeper
                 else:
-                    # "go deeper" to get children of children.
                     node_path = node_path['children'][-1]
 
-        return tree_data
+        return tree_root
 
-    @property
-    def file_required(self):
-        return True
+    @staticmethod
+    def icon_exists_for_type(ext: str) -> bool:
+        """Check if an icon exists for the given file type.  The extension string is converted to
+        lower case.
 
-    @property
-    def cache_result(self):
-        return True
+        :param ext: the file extension str
+        :rtype: ``bool``
+        :return: ``True`` if found; ``False`` otherwise
+        """
+
+        return os.path.isfile(os.path.join(
+            os.path.dirname(__file__),
+            'static',
+            'img',
+            'file-ext-{}.png'.format(ext.lower())
+        ))
+
+    @staticmethod
+    def sanitize_file_list(file_list: list) -> list:
+        """Remove macOS system and temporary files.  Current implementation only removes '__MACOSX/'
+        and '.DS_Store'.  If necessary, extend the sanitizer to exclude more file types.
+
+        :param file_list: the list of the path for each file and folder in the zip
+        :rtype: ``list``
+        :return: a sanitized list
+        """
+
+        sanitized_file_list = []
+
+        for file in file_list:
+
+            file_path = file.filename
+            # Ignore macOS '__MACOSX' folder for zip file
+            if file_path.startswith('__MACOSX/'):
+                continue
+
+            # Ignore macOS '.DS_STORE' file
+            if file_path == '.DS_Store' or file_path.endswith('/.DS_Store'):
+                continue
+
+            sanitized_file_list.append(file)
 
-def check_icon_ext(ext):
-    return os.path.isfile(os.path.join(os.path.dirname(__file__), 'static', 'img', 'icons',
-                                'file_extension_{}.png'.format(ext)))
+        return sanitized_file_list

mfr/extensions/zip/templates/viewer.mako

@@ -13,6 +13,7 @@
 </div>
 
 <script>
+
     $('#ziptree').jstree({
         "core" : {
             "data" : ${data},
@@ -22,21 +23,28 @@
             "search"
         ],
         'table': {
-		    'columns': [
-		        {'header': "Name", 'width': '100%'}, // these widths are strange but work.
-                {'header': "Size", 'width': 80, 'value': "size"},
-                {'header': "Date", 'width': 150, 'value': "date"}
-            ],
+            'columns': [
+                {'header': "Name", 'width': '100%'},
+                {'header': "Size", 'width': 100, 'value': "size"},
+                {'header': "Date", 'width': 250, 'value': "date"}
+            ]
         }
     });
+
     var to = false;
     $('#ziptree_search').keyup(function () {
-        if(to) { clearTimeout(to); }
-        to = setTimeout(function () {
-            var v = $('#ziptree_search').val();
-            $('#ziptree').jstree(true).search(v);
-        }, 250);
+        if (to) {
+            clearTimeout(to);
+        }
+        to = setTimeout(
+            function () {
+                var v = $('#ziptree_search').val();
+                $('#ziptree').jstree(true).search(v);
+            },
+            250
+        );
     });
+
 </script>
 
 <script src="/static/js/mfr.js"></script>