[extensions-web] review: Make binary files unselectable

[Jasper St. Pierre <jstpierre src gnome org>]

commit 1b636c8af89b24386c46f1152e985043e995059d
Author: Jasper St. Pierre <jstpierre mecheye net>
Date:   Tue Feb 7 13:42:18 2012 -0500

    review: Make binary files unselectable
    
    Rather than have binary files just show a dumb view, highlight them
    in the file list and make them unclickable

 sweettooth/review/views.py       |   41 ++++++++++++-------------------------
 sweettooth/static/css/review.css |    5 ++++
 sweettooth/static/js/review.js   |   32 +++++++++++++++++++++--------
 3 files changed, 41 insertions(+), 37 deletions(-)
---
diff --git a/sweettooth/review/views.py b/sweettooth/review/views.py
index cd25d01..5bb519f 100644
--- a/sweettooth/review/views.py
+++ b/sweettooth/review/views.py
@@ -35,6 +35,7 @@ IMAGE_TYPES = {
     '.svg':  'image/svg+xml',
 }
 
+# Keep this in sync with the BINARY_TYPES list at the top of review.js
 BINARY_TYPES = set(['.mo', '.png', ',jpg', '.jpeg', '.gif', '.bmp'])
 
 # Stolen from ReviewBoard
@@ -83,20 +84,17 @@ def highlight_file(filename, raw, formatter):
 
     return pygments.highlight(raw, lexer, formatter)
 
-def html_for_file(filename, version, raw):
+def html_for_file(filename, raw):
     base, extension = os.path.splitext(filename)
 
-    if extension in IMAGE_TYPES:
+    if extension in BINARY_TYPES:
+        return None
+    elif extension in IMAGE_TYPES:
         mime = IMAGE_TYPES[extension]
         raw_base64 = base64.standard_b64encode(raw)
-        return dict(raw=True, binary=False,
-                    html='<img src="data:%s;base64,%s">' % (mime, raw_base64,))
-    elif extension in BINARY_TYPES:
-        download_url = reverse('review-download', kwargs=dict(pk=version.pk))
-        return dict(raw=False, binary=True, url=download_url)
+        return dict(raw=True, html='<img src="data:%s;base64,%s">' % (mime, raw_base64,))
     else:
-        return dict(raw=False, binary=False,
-                    lines=split_lines(highlight_file(filename, raw, code_formatter)))
+        return dict(raw=False, lines=split_lines(highlight_file(filename, raw, code_formatter)))
 
 def get_zipfiles(version, old_version_number=None):
     extension = version.extension
@@ -113,20 +111,6 @@ def get_zipfiles(version, old_version_number=None):
 
     return old_zipfile, new_zipfile
 
-def get_filelist(zipfile, disallow_binary):
-    for name in zipfile.namelist():
-        if name.endswith('/'):
-            # There's no directory flag in the info, so I'm
-            # guessing this is the most reliable way to do it.
-            continue
-
-        if disallow_binary:
-            base, extension = os.path.splitext(name)
-            if extension in BINARY_TYPES:
-                continue
-
-        yield name
-
 def get_diff(old_zipfile, new_zipfile, filename):
     old, new = old_zipfile.open(filename, 'r'), new_zipfile.open(filename, 'r')
     oldcontent, newcontent = old.read(), new.read()
@@ -148,6 +132,9 @@ def get_diff(old_zipfile, new_zipfile, filename):
                 oldlines=oldlines,
                 newlines=newlines)
 
+def get_filelist(zipfile):
+    return set(n for n in zipfile.namelist() if not n.endswith('/'))
+
 @ajax_view
 @model_view(models.ExtensionVersion)
 def ajax_get_file_list_view(request, obj):
@@ -157,9 +144,7 @@ def ajax_get_file_list_view(request, obj):
 
     old_zipfile, new_zipfile = get_zipfiles(version, old_version_number)
 
-    disallow_binary = json.loads(request.GET['disallow_binary'])
-
-    new_filelist = set(get_filelist(new_zipfile, disallow_binary))
+    new_filelist = get_filelist(new_zipfile)
 
     if old_zipfile is None:
         return dict(unchanged=[],
@@ -167,7 +152,7 @@ def ajax_get_file_list_view(request, obj):
                     added=sorted(new_filelist),
                     deleted=[])
 
-    old_filelist = set(get_filelist(old_zipfile, disallow_binary))
+    old_filelist = get_filelist(old_zipfile)
 
     both    = new_filelist & old_filelist
     added   = new_filelist - old_filelist
@@ -227,7 +212,7 @@ def ajax_get_file_view(request, obj):
         raise Http404()
 
     raw = f.read()
-    return html_for_file(filename, obj, raw)
+    return html_for_file(filename, raw)
 
 def download_zipfile(request, pk):
     version = get_object_or_404(models.ExtensionVersion, pk=pk)
diff --git a/sweettooth/static/css/review.css b/sweettooth/static/css/review.css
index 241816a..a35241e 100644
--- a/sweettooth/static/css/review.css
+++ b/sweettooth/static/css/review.css
@@ -91,6 +91,11 @@
     color: #747474;
 }
 
+.fileselector.binary {
+    color: #AA0000;
+    cursor: auto;
+}
+
 .fileselector.selected {
     cursor: auto;
     color: #2e3436;
diff --git a/sweettooth/static/js/review.js b/sweettooth/static/js/review.js
index ea47f97..3cc04e3 100644
--- a/sweettooth/static/js/review.js
+++ b/sweettooth/static/js/review.js
@@ -4,16 +4,21 @@ define(['jquery', 'diff'], function($, diff) {
 
     var REVIEW_URL_BASE = '/review/ajax';
 
+    var BINARY_TYPES = {};
+    $.each(['mo', 'png', 'jpg', 'jpeg', 'gif', 'bmp'], function() {
+        BINARY_TYPES[this] = true;
+    });
+
+    function isBinary(filename) {
+        var parts = filename.split('.');
+        var ext = parts[parts.length - 1];
+        return BINARY_TYPES.hasOwnProperty(ext);
+    }
+
     function buildFileView(data) {
         if (data.raw)
             return $(data.html);
 
-        if (data.binary)
-            return $("<p>").
-                append("This file is binary. Please ").
-                append($("<a>", {'href': data.url}).text("download the zipfile")).
-                append("to see it.");
-
         var $table = $('<table>', {'class': 'code'});
 
         $.each(data.lines, function(i) {
@@ -67,7 +72,6 @@ define(['jquery', 'diff'], function($, diff) {
         var req = $.ajax({
             type: 'GET',
             dataType: 'json',
-            data: { disallow_binary: diff },
             url: REVIEW_URL_BASE + '/get-file-list/' + pk,
         });
 
@@ -99,6 +103,18 @@ define(['jquery', 'diff'], function($, diff) {
 
                 var $file = null;
 
+                if (diff && isBinary(filename)) {
+                    // We don't show binary files in the diff view.
+                    return;
+                }
+
+                $('<li>').append($selector).appendTo($fileList);
+
+                if (isBinary(filename)) {
+                    $selector.addClass('binary');
+                    return;
+                }
+
                 $selector.click(function() {
                     if ($selector.hasClass('selected'))
                         return;
@@ -117,8 +133,6 @@ define(['jquery', 'diff'], function($, diff) {
                     }
 
                 });
-
-                $('<li>').append($selector).appendTo($fileList);
             }
 
             $.each(files.changed, function() { createFileSelector('changed', this); });