Merge lp:~gary/juju-gui/bug1203583 into lp:juju-gui/experimental

[Gary Poster]

Make the charm source tab work better

This was a quick hack I did that I wanted to land rather than throw away. Quick hacks are a lot less quick once you throw in tests. :-/

https://codereview.appspot.com/14565044/

[Gary Poster (gary)]

Reviewers: mp+189970_code.launchpad.net,

Message:
Please take a look.

Description:
Make the charm source tab work better

This was a quick hack I did that I wanted to land rather than throw
away. Quick hacks are a lot less quick once you throw in tests. :-/

https://code.launchpad.net/~gary/juju-gui/bug1203583/+merge/189970

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14565044/

Affected files (+100, -2 lines):
   A [revision details]
   M app/models/charm.js
   M app/subapps/browser/views/bundle.js
   M app/subapps/browser/views/charm.js
   M test/test_browser_charm_details.js
   M test/test_model.js

[Gary Poster (gary)]

https://codereview.appspot.com/14565044/diff/1/app/subapps/browser/views/bundle.js
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14565044/diff/1/app/subapps/browser/views/bundle.js#newcode169
app/subapps/browser/views/bundle.js:169: 'subapp-browser-entitybaseview'
I discovered that the file needed these in order for some test files to
be run individually.

https://codereview.appspot.com/14565044/

[Jeff Pihach (hatch)]

LGTM QA OK with trivial

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode466
app/models/charm.js:466: setter: function(value) {
you can quote this instead of commenting it if you like.

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode468
app/models/charm.js:468: value.sort(function(a, b) {
This all looks great but I'm curious what this sort method gets you over
the standard lexicographic sort. You may also want to add this comment
to the code so others know when looking at this.

https://codereview.appspot.com/14565044/

[Gary Poster (gary)]

*** Submitted:

Make the charm source tab work better

This was a quick hack I did that I wanted to land rather than throw
away. Quick hacks are a lot less quick once you throw in tests. :-/

R=jeff.pihach
CC=
https://codereview.appspot.com/14565044

https://codereview.appspot.com/14565044/

[Gary Poster (gary)]

On 2013/10/08 21:03:10, jeff.pihach wrote:
> LGTM QA OK with trivial

> https://codereview.appspot.com/14565044/diff/1/app/models/charm.js
> File app/models/charm.js (right):

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode466
> app/models/charm.js:466: setter: function(value) {
> you can quote this instead of commenting it if you like.

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode468
> app/models/charm.js:468: value.sort(function(a, b) {
> This all looks great but I'm curious what this sort method gets you
over the
> standard lexicographic sort. You may also want to add this comment to
the code
> so others know when looking at this.

Thank you for the review and comments, Jeff. I added the following
explanation to the code.

             // This sort has several properties that are different than
a
             // standard lexicographic sort.
             // * Filenames in the root are grouped together, rather than
             // sorted along with the names of directories.
             // * Sort ignores case, unless case is the only way to
             // distinguish between values, in which case it is honored
             // per-directory. For example, this is sorted: "a", "b/c",
             // "b/d", "B/b", "B/c", "e/f". Notice that "b" directory
values
             // and "B" directory values are grouped together, where
they
             // would not necessarily be in a simpler case-insensitive
             // lexicographic sort.
             // If you rip this out for something different and simpler,
that's
             // fine; just don't tell me about it. :-) This seemed like
a good
             // approach at the time.

https://codereview.appspot.com/14565044/

=== modified file 'app/models/charm.js'
--- app/models/charm.js	2013-09-24 18:56:05 +0000
+++ app/models/charm.js	2013-10-08 20:50:46 +0000
@@ -457,7 +457,46 @@
         }
       },
       files: {
-        value: []
+        value: [],
+        /**
+         * Sort files when they are set.
+         *
+         * @method files.setter
+         */
+        setter: function(value) {
+          if (Y.Lang.isArray(value)) {
+            value.sort(function(a, b) {
+              var segments = [a, b].map(function(path) {
+                var segs = path.split('/');
+                if (segs.length === 1) {
+                  segs.unshift('');
+                }
+                return segs;
+              });
+              var maxLength = Math.max(segments[0].length, segments[1].length);
+              for (var i = 0; i < maxLength; i += 1) {
+                var segmentA = segments[0][i];
+                var segmentB = segments[1][i];
+                if (segmentA === undefined) {
+                  return -1;
+                } else if (segmentB === undefined) {
+                  return 1;
+                } else {
+                  var result = (
+                      // Prefer case-insensitive sort, but honor case when
+                      // string match in a case-insensitive comparison.
+                      segmentA.localeCompare(
+                          segmentB, undefined, {sensitivity: 'accent'}) ||
+                      segmentA.localeCompare(segmentB));
+                  if (result) {
+                    return result;
+                  }
+                }
+              }
+              return 0;
+            });
+          }
+        }
       },
       full_name: {
         /**

=== modified file 'app/subapps/browser/views/bundle.js'
--- app/subapps/browser/views/bundle.js	2013-10-07 20:45:48 +0000
+++ app/subapps/browser/views/bundle.js	2013-10-08 20:50:46 +0000
@@ -161,8 +161,11 @@
 
 }, '', {
   requires: [
+    'browser-overlay-indicator',
+    'juju-view-utils',
     'view',
     'juju-env-fakebackend',
-    'juju-view-bundle'
+    'juju-view-bundle',
+    'subapp-browser-entitybaseview'
   ]
 });

=== modified file 'app/subapps/browser/views/charm.js'
--- app/subapps/browser/views/charm.js	2013-10-02 17:37:08 +0000
+++ app/subapps/browser/views/charm.js	2013-10-08 20:50:46 +0000
@@ -331,6 +331,13 @@
       tplData.isFullscreen = isFullscreen;
       tplData.isLocal = tplData.scheme === 'local';
       tplData.forInspector = this.get('forInspector');
+      if (tplData.files) {
+        // Exclude svg files from the source view.
+        var regex = /\.svg$/;
+        tplData.files = tplData.files.filter(function(name) {
+          return !regex.test(name);
+        });
+      }
       if (!tplData.forInspector) {
         tplData.sourceLink = this._getSourceLink();
         tplData.prettyCommits = this._formatCommitsForHtml(

=== modified file 'test/test_browser_charm_details.js'
--- test/test_browser_charm_details.js	2013-10-02 16:26:20 +0000
+++ test/test_browser_charm_details.js	2013-10-08 20:50:46 +0000
@@ -211,6 +211,27 @@
           view._getRevnoLink(url, 1));
     });
 
+    it('excludes source svg files from the source tab', function() {
+      view = new CharmView({
+        entity: new models.Charm({
+          files: [
+            'hooks/install',
+            'icon.svg',
+            'readme.rst'
+          ],
+          id: 'precise/ceph-9',
+          code_source: { location: 'lp:~foo'}
+        }),
+        container: utils.makeContainer()
+      });
+      view.render();
+      var options = Y.one('#bws-code').all('select option');
+      assert.equal(options.size(), 3);
+      assert.deepEqual(
+          options.get('text'),
+          ['Select --', 'readme.rst', 'hooks/install']);
+    });
+
     it('can generate useful display data for commits', function() {
       view = new CharmView({
         entity: new models.Charm({

=== modified file 'test/test_model.js'
--- test/test_model.js	2013-09-27 16:26:18 +0000
+++ test/test_model.js	2013-10-08 20:50:46 +0000
@@ -667,6 +667,32 @@
       assert.isFalse(unapproved_without_icon.get('shouldShowIcon'));
     });
 
+    it('sorts files', function() {
+      // Because we rely on localeCompare, and this has different
+      // implementations and capabilities across browsers, we don't include
+      // any capital letters in this test.  They sort reliably within a given
+      // browser, but not across browsers.
+      instance = new models.Charm({
+        id: 'cs:precise/mysql-2',
+        files: [
+          'alpha/beta/gamma',
+          'alpha/beta',
+          'alpha/aardvark',
+          'zebra',
+          'yam'
+        ]
+      });
+      assert.deepEqual(
+          instance.get('files'),
+          [
+            'yam',
+            'zebra',
+            'alpha/aardvark',
+            'alpha/beta',
+            'alpha/beta/gamma'
+          ]);
+    });
+
     it('tracks recent commits in the last 30 days', function() {
       instance = new models.Charm(data.charm);
       var commits = instance.get('recent_commits'),