Merge lp:~abentley/launchpad/hide-numbers into lp:launchpad

[Aaron Bentley]

Allow hiding diff line numbers.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
Partial fix for #382561: Provide the ability to show and hide line
numbers in the preview diff, using a checkbox.

Note that a fully-functional workaround for #382561 is to use the
"Download diff" link.

== Proposed fix ==

== Pre-implementation notes ==
Pre-implementation was with Thumper

== Implementation details ==
The config and Makefile changes were done so that it was possible to
generate a diff in the developer environment.

The checkbox is implemented as a Widget called NumberToggle.

== Tests ==
None

== Demo and Q/A ==
 Create a merge proposal with a diff. Click the checkbox (or the
phrase) "Show line numbers".

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/templates/branchmergeproposal-index.pt
  lib/canonical/launchpad/javascript/code/codereview.js
  Makefile
  configs/development/launchpad-lazr.conf

== JSLint notices ==
No handlers could be found for logger "bzr"
jslint: Lint found in
'/home/abentley/launchpad/hide-numbers/lib/canonical/launchpad/javascript/bugs/bugtask-index.js':
Line 584 character 31: ['api_uri'] is better written in dot notation.
        var branch_url = data['api_uri'];

^^^ I didn't change this file!

jslint: No problem found in
'/home/abentley/launchpad/hide-numbers/lib/canonical/launchpad/javascript/code/codereview.js'.

jslint: 2 files to lint.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrl+fcACgkQ0F+nu1YWqI2KtACePSXRjpjFE1lfIwppl6QLmBe2
gmoAnRNlwDgwMDWrxXsiGRCPimxEwgHp
=5eM5
-----END PGP SIGNATURE-----

[Brad Crittenden (bac)]

Looks good Aaron.

Please change line 8 so the comment has a period.

review: Approve

[Paul Hummer (rockstar)]

review: Approve (ui)

[Curtis Hovey (sinzui)]

Hi Aaron.

I create two branches in dev, then proposed a merge. I ran make sync_branches. The toggle works well, but I an unhappy with the spacing between it and the download link. We use <ul class="horizontal"> in many places to normalise the space between user actions on a page (1.5em). I am not sure that this can be reused in the <div>. Can you experiment with the markup/CSS to make the spacing consistent with the rest of Launchpad?

review: Needs Information (ui)

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Curtis Hovey wrote:
> We use <ul class="horizontal"> in many places to normalise the
> space between user actions on a page (1.5em).
> Can you experiment with the markup/CSS to make the
> spacing consistent with the rest of Launchpad?

I've done this, and attached an incremental diff. I agree this is a bit
nicer.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrodzUACgkQ0F+nu1YWqI13qACfR6567uVNgrnaUUD40lxXMlXl
euEAn2CeMazyoVCY9PSMRe53YN57PMeQ
=BdH5
-----END PGP SIGNATURE-----

[Curtis Hovey (sinzui)]

Thanks for exploring this. I too think it looks nicer.

review: Approve (ui)

=== modified file 'Makefile'
--- Makefile	2009-10-28 11:05:01 +0000
+++ Makefile	2009-10-28 16:53:12 +0000
@@ -170,6 +170,10 @@
 ftest_inplace: inplace
 	bin/test -f $(TESTFLAGS) $(TESTOPTS)
 
+mpcreationjobs:
+	# Handle merge proposal creations.
+	$(PY) cronscripts/mpcreationjobs.py
+
 run: inplace stop
 	$(RM) thread*.request
 	bin/run -r librarian,google-webservice -i $(LPCONFIG)
@@ -206,7 +210,8 @@
 	# Scan branches from the filesystem into the database.
 	$(PY) cronscripts/branch-scanner.py
 
-sync_branches: pull_branches scan_branches
+
+sync_branches: pull_branches scan_branches mpcreationjobs
 
 $(BZR_VERSION_INFO):
 	scripts/update-bzr-version-info.sh

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2009-10-20 11:29:26 +0000
+++ configs/development/launchpad-lazr.conf	2009-10-28 16:53:12 +0000
@@ -182,6 +182,10 @@
 soft_max_size: 40000
 hard_max_size: 1000000
 
+[mpcreationjobs]
+error_dir: /var/tmp/codehosting.test
+oops_prefix: DMPCR
+
 [personalpackagearchive]
 root: /var/tmp/ppa/
 private_root: /var/tmp/ppa

=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js	2009-08-03 20:11:00 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js	2009-10-28 16:53:12 +0000
@@ -124,4 +124,37 @@
 }
 
 
-}, '0.1', {requires: ['base', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});
+var NumberToggle = function () {
+        NumberToggle.superclass.constructor.apply(this, arguments);
+};
+
+
+var update_nos = function(){
+    var new_display = 'none';
+    if (this.get('checked')) {
+        new_display = 'block';
+    }
+    Y.all('td.line-no').setStyle('display', new_display);
+};
+
+
+NumberToggle.NAME = 'numbertoggle';
+
+NumberToggle.ATTRS = {
+};
+Y.extend(NumberToggle, Y.Widget, {
+    renderUI: function() {
+        var ui = Y.Node.create('<li><label>' +
+            '<input type="checkbox" checked="checked" id="show-no"/>' +
+            '&nbsp;Show line numbers</label></li>');
+        var ul = Y.get('#review-diff div div ul.horizontal');
+        ul.appendChild(ui);
+    },
+    bindUI: function(){
+        Y.get('#show-no').on('click', update_nos);
+    }
+});
+
+Y.codereview.NumberToggle = NumberToggle;
+
+}, '0.1', {requires: ['base', 'widget', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});

=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2009-10-23 02:41:08 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2009-10-28 16:53:12 +0000
@@ -145,10 +145,14 @@
        tal:define="attachment view/preview_diff/diff_text">
     <tal:real-diff condition="attachment">
       <div class="boardCommentDetails filename">
-        <a tal:attributes="href attachment/getURL">
-          <img src="/@@/download"/>
-          Download diff
-        </a>
+        <ul class="horizontal">
+          <li>
+            <a tal:attributes="href attachment/getURL">
+              <img src="/@@/download"/>
+              Download diff
+            </a>
+          </li>
+        </ul>
       </div>
       <div class="boardCommentBody attachmentBody">
         <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
@@ -181,6 +185,8 @@
       Y.code.codereview.connect_links();
   });
 
+  (new Y.codereview.NumberToggle()).render();
+
   });
   -->
 <tal:script replace="structure string:&lt;/script&gt;" />