Bug#761103: debsources: highlight #Lxxx lines by default
On Thu, Nov 06, 2014 at 08:09:15PM -0500, Jason Pleau wrote:
The attached patch implements your previous suggestions :
Awesome, thanks! Last comments:
1) I've added to the new .js file a copyright header pointing to
you. Can you confirm you're OK with contributing your code under
AGPLv3, as the rest of Debsources?
You can find attached a version of your patch that includes the
copyright header (and fixes some whitespace issues that make Git
cry :-))
+change_hash_without_scroll(callerElement, L +
callerElement.getAttribute('data-line'));
+} else {
+var first_line =
parseInt(last_clicked.getAttribute('data-line'));
+var second_line =
parseInt(callerElement.getAttribute('data-line'));
[...]
+a id=L{{ i }} href=#L{{ i }} data-line={{ i }}{{ i }}/abr
/
2) In the same anti-bloat vein of my previous comments, do we really
need to another attribute here, given that its content is precisely
the same of the text child of a? Can't you just use the data
content of that text node?
(Yes, I understand that in the future the a node might contain more
complex markup, but there will always be a text leaf in the DOM tree
that we can use; if not directly a DOM method that return the textual
value of the whole sub-tree.)
What do you think?
Cheers.
--
Stefano Zacchiroli . . . . . . . z...@upsilon.cc . . . . o . . . o . o
Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o
Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
signature.asc
Description: Digital signature
Bug#761103: debsources: highlight #Lxxx lines by default
On Fri, Nov 07, 2014 at 06:24:58PM +0100, Stefano Zacchiroli wrote:
You can find attached a version of your patch that includes the
copyright header (and fixes some whitespace issues that make Git
cry :-))
Now with an attachment.
--
Stefano Zacchiroli . . . . . . . z...@upsilon.cc . . . . o . . . o . o
Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o
Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
From eaa36cc8aa2ba936932ec4b2f99ba1b9fef41982 Mon Sep 17 00:00:00 2001
From: Jason Pleau ja...@jpleau.ca
Date: Sat, 1 Nov 2014 10:44:24 -0400
Subject: [PATCH] web app: automatically highlight #Lxx lines
Add automatic line numbers from #Lxx location.hash
It supports #L50 and #L50-L150 (The latter will highlight from line 50 to line
150).
It also changes the hash if we click on a line, and if we click on a second
line holding the SHIFT key, it will highlight the whole range, and update the
hash as well.
Closes: #761103
Signed-off-by: Stefano Zacchiroli z...@upsilon.cc
---
debsources/app/static/javascript/debsources.js | 137 +
debsources/app/templates/source_file.html | 1 +
debsources/app/templates/source_file_code.inc.html | 7 +-
3 files changed, 140 insertions(+), 5 deletions(-)
create mode 100644 debsources/app/static/javascript/debsources.js
diff --git a/debsources/app/static/javascript/debsources.js b/debsources/app/static/javascript/debsources.js
new file mode 100644
index 000..2a382bb
--- /dev/null
+++ b/debsources/app/static/javascript/debsources.js
@@ -0,0 +1,137 @@
+/* Copyright (C) 2014 Jason Pleau ja...@jpleau.ca
+ *
+ * This file is part of Debsources.
+ *
+ * Debsources is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, either version 3 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+/*
+ * Highlight line numbers according to data received in the anchor
+ *
+ * Example: file.cpp#L50-L275 will highlight lines 50 to 275.
+ *
+ * There's also support to select one line or a range of lines (by clicking on
+ * a line or shift-clicking on a range of lines). The URL will be updated with
+ * the selection.
+ *
+ */
+
+(function() {
+function highlight_lines(start, end) {
+// First, remove the highlight class from elements that might already have it
+var elements = document.querySelectorAll(span.highlight);
+for (i = 0; i elements.length; ++i) {
+var element = elements[i];
+element.className = element.className.replace(/\bhighlight\b/, '');
+}
+
+// Then, add the highlight class to elements that contain the lines we want to highlight
+for (i = start; i = end; ++i) {
+var element = document.getElementById(line + i);
+element.className = element.className + highlight ;
+}
+}
+
+var hash_changed = function(event, scroll) {
+
+event = typeof event !== 'undefined' ? event: null;
+scroll = typeof scroll !== 'undefined' ? scroll: false;
+
+// Will match strings like #L15 and #L15-L20
+var regex = /#L(\d+)(-L(\d+))*$/;
+
+var match = regex.exec(window.location.hash);
+if (match != null) {
+var first_line = second_line = null;
+first_line = parseInt(match[1]);
+
+if (typeof match[3] !== 'undefined' match[3].length 0) {
+second_line = parseInt(match[3]);
+} else {
+second_line = first_line;
+}
+
+// If we get something like #L20-L15, just swap the two line numbers so the loop will work
+if (second_line first_line) {
+var tmp = first_line;
+first_line = second_line;
+second_line = tmp;
+}
+
+highlight_lines(first_line, second_line);
+
+if (scroll) {
+window.scroll(0, document.getElementById(L+first_line).offsetTop);
+}
+}
+}
+
+
+function change_hash_without_scroll(element, hash) {
+// This is necessary because when changing window.location.hash, the window will
+// scroll to the element's id if it matches the hash
+var id = element.id;
+element.id = id+'-tmpNoScroll';
+window.location.hash = hash;
+element.id = id;
+}
+
+var
Bug#761103: debsources: highlight #Lxxx lines by default
Hello Stefano,
On 07/11/14 12:24 PM, Stefano Zacchiroli wrote:
On Thu, Nov 06, 2014 at 08:09:15PM -0500, Jason Pleau wrote:
The attached patch implements your previous suggestions :
Awesome, thanks! Last comments:
1) I've added to the new .js file a copyright header pointing to
you. Can you confirm you're OK with contributing your code under
AGPLv3, as the rest of Debsources?
You can find attached a version of your patch that includes the
copyright header (and fixes some whitespace issues that make Git
cry :-))
I'm OK with my code being under this license ! I'll use this copyright
header if I ever need to create new files in the Debsources project.
+change_hash_without_scroll(callerElement, L +
callerElement.getAttribute('data-line'));
+} else {
+var first_line =
parseInt(last_clicked.getAttribute('data-line'));
+var second_line =
parseInt(callerElement.getAttribute('data-line'));
[...]
+a id=L{{ i }} href=#L{{ i }} data-line={{ i }}{{ i
}}/abr /
2) In the same anti-bloat vein of my previous comments, do we really
need to another attribute here, given that its content is precisely
the same of the text child of a? Can't you just use the data
content of that text node?
(Yes, I understand that in the future the a node might contain more
complex markup, but there will always be a text leaf in the DOM tree
that we can use; if not directly a DOM method that return the textual
value of the whole sub-tree.)
What do you think?
Cheers.
I usually prefer to not rely on elements' innerHTML/innerText content
(that's just a personal style). But I also always only work in LAN
environments, where speed isn't really an issue. I agree that if we're
browsing a 10k+ line files, those extra bytes can add up quickly.
Please see the attached patch for the fixed version.
Thanks for the feedback !
Jason
From c1a218f019368d6ea6cedab2d813073ade685a32 Mon Sep 17 00:00:00 2001
From: Jason Pleau ja...@jpleau.ca
Date: Sat, 1 Nov 2014 10:44:24 -0400
Subject: [PATCH] web app: automatically highlight #Lxx lines
Add automatic line numbers from #Lxx location.hash
It supports #L50 and #L50-L150 (The latter will highlight from line 50 to line
150).
It also changes the hash if we click on a line, and if we click on a second
line holding the SHIFT key, it will highlight the whole range, and update the
hash as well.
Closes: #761103
Signed-off-by: Stefano Zacchiroli z...@upsilon.cc
---
debsources/app/static/javascript/debsources.js | 137 +
debsources/app/templates/source_file.html | 1 +
debsources/app/templates/source_file_code.inc.html | 5 +-
3 files changed, 139 insertions(+), 4 deletions(-)
create mode 100644 debsources/app/static/javascript/debsources.js
diff --git a/debsources/app/static/javascript/debsources.js b/debsources/app/static/javascript/debsources.js
new file mode 100644
index 000..38e3647
--- /dev/null
+++ b/debsources/app/static/javascript/debsources.js
@@ -0,0 +1,137 @@
+/* Copyright (C) 2014 Jason Pleau ja...@jpleau.ca
+ *
+ * This file is part of Debsources.
+ *
+ * Debsources is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, either version 3 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+/*
+ * Highlight line numbers according to data received in the anchor
+ *
+ * Example: file.cpp#L50-L275 will highlight lines 50 to 275.
+ *
+ * There's also support to select one line or a range of lines (by clicking on
+ * a line or shift-clicking on a range of lines). The URL will be updated with
+ * the selection.
+ *
+ */
+
+(function() {
+function highlight_lines(start, end) {
+// First, remove the highlight class from elements that might already have it
+var elements = document.querySelectorAll(span.highlight);
+for (i = 0; i elements.length; ++i) {
+var element = elements[i];
+element.className = element.className.replace(/\bhighlight\b/, '');
+}
+
+// Then, add the highlight class to elements that contain the lines we want to highlight
+for (i = start; i = end; ++i) {
+var element = document.getElementById(line + i);
+element.className = element.className + highlight ;
+}
+}
+
+var hash_changed = function(event, scroll) {
+
+event = typeof event !== 'undefined' ? event:
Bug#761103: debsources: highlight #Lxxx lines by default
Hello again, Jason.
To JavaScript hackers lurking here: your comments on this patch are most
welcome!
On Sat, Nov 01, 2014 at 11:37:56AM -0400, Jason Pleau wrote:
I have attached a patch that solves this bug.
Thanks a lot for this patch. It works like a charm on my local
deployment. As I'm no JavaScript guru, I could use extra eyeballs to
review it, and I do have some questions for you (see below), but I see
no reason why this couldn't be integrated.
-a id=L{{ i }} href=#L{{ i }}{{ i }}/abr /
+a id=L{{ i }} href=#L{{ i }} class=linenumber data-line={{ i
}}{{ i }}/abr /
Is it really needed to add class=linenumber to all a elements?
It seems that the JavaScript only uses that class as a selector. So
can't one instead rely on the id=sourcelinenumbers that is already on
the parent pre element, and consider all its a children? That would
avoid bloat in the generated HTML, reduce load time, etc.
+script type=text/javascript
[...]
+/script
I'd like to move this JavaScript snippet to a separate .js file, rather
than re-shipping it every time a file is rendered. Can you change your
patch to do so?
I suggest to name it something like
DEBSOURCES_ROOT/debsources/app/static/javascript/debsources.js (so that
we can use it in the future for other JavaScript-related needs). It will
then be accessible from the HTML as /static/javascript/debsources.js
(note: *not* as /javascript/debsources.js).
Cheers.
--
Stefano Zacchiroli . . . . . . . z...@upsilon.cc . . . . o . . . o . o
Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o
Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
signature.asc
Description: Digital signature
Bug#761103: debsources: highlight #Lxxx lines by default
Hello Stefano,
The attached patch implements your previous suggestions :
On 06/11/14 02:22 PM, Stefano Zacchiroli wrote:
Hello again, Jason.
To JavaScript hackers lurking here: your comments on this patch are most
welcome!
Thanks a lot for this patch. It works like a charm on my local
deployment. As I'm no JavaScript guru, I could use extra eyeballs to
review it, and I do have some questions for you (see below), but I see
no reason why this couldn't be integrated.
-a id=L{{ i }} href=#L{{ i }}{{ i }}/abr /
+a id=L{{ i }} href=#L{{ i }} class=linenumber data-line={{
i }}{{ i }}/abr /
Is it really needed to add class=linenumber to all a elements?
It seems that the JavaScript only uses that class as a selector. So
can't one instead rely on the id=sourcelinenumbers that is already on
the parent pre element, and consider all its a children? That would
avoid bloat in the generated HTML, reduce load time, etc.
Indeed, it was not needed to add a new class. Using the pre element's
#id is a better choice.
+script type=text/javascript
[...]
+/script
I'd like to move this JavaScript snippet to a separate .js file, rather
than re-shipping it every time a file is rendered. Can you change your
patch to do so?
I suggest to name it something like
DEBSOURCES_ROOT/debsources/app/static/javascript/debsources.js (so that
we can use it in the future for other JavaScript-related needs). It will
then be accessible from the HTML as /static/javascript/debsources.js
(note: *not* as /javascript/debsources.js).
I have added a new file:
DEBSOURCES_ROOT/debsources/app/static/javascript/debsources.js
It is only included in the source_file.html template, there's no need
(yet) to force the download of this javascript file in other templates.
The code is now wrapped in an an anonymous function, to not pollute the
global namespace if this javascript file is ever loaded in other pages.
Thanks
--
Jason Pleau
From 8132506a6d178db416f852161e279d367529d8f7 Mon Sep 17 00:00:00 2001
From: Jason Pleau ja...@jpleau.ca
Date: Sat, 1 Nov 2014 10:44:24 -0400
Subject: [PATCH] add automatic line numbers from #Lxx location.hash
It supports #L50 and #L50-L150 (The latter will highlight from line 50
to line 150).
It also changes the hash if we click on a line, and if we click on a
second line holding the SHIFT key, it will highlight the whole range,
and update the hash as well.
---
debsources/app/static/javascript/debsources.js | 116 +
debsources/app/templates/source_file.html | 1 +
debsources/app/templates/source_file_code.inc.html | 7 +-
3 files changed, 119 insertions(+), 5 deletions(-)
create mode 100644 debsources/app/static/javascript/debsources.js
diff --git a/debsources/app/static/javascript/debsources.js b/debsources/app/static/javascript/debsources.js
new file mode 100644
index 000..20b81d8
--- /dev/null
+++ b/debsources/app/static/javascript/debsources.js
@@ -0,0 +1,116 @@
+/* Highlight line numbers according to data received in the anchor
+ * Example: file.cpp#L50-L275 will highlight lines 50 to 275.
+ *
+ * There's also support to select one line or a range of lines (by clicking on
+ * a line or shift-clicking on a range of lines). The URL will be updated
+ * with the selection.
+ */
+
+(function() {
+function highlight_lines(start, end) {
+// First, remove the highlight class from elements that might already have it
+var elements = document.querySelectorAll(span.highlight);
+for (i = 0; i elements.length; ++i) {
+var element = elements[i];
+element.className = element.className.replace(/\bhighlight\b/, '');
+}
+
+// Then, add the highlight class to elements that contain the lines we want to highlight
+for (i = start; i = end; ++i) {
+var element = document.getElementById(line + i);
+element.className = element.className + highlight ;
+}
+}
+
+var hash_changed = function(event, scroll) {
+
+event = typeof event !== 'undefined' ? event: null;
+scroll = typeof scroll !== 'undefined' ? scroll: false;
+
+// Will match strings like #L15 and #L15-L20
+var regex = /#L(\d+)(-L(\d+))*$/;
+
+var match = regex.exec(window.location.hash);
+if (match != null) {
+var first_line = second_line = null;
+first_line = parseInt(match[1]);
+
+if (typeof match[3] !== 'undefined' match[3].length 0) {
+second_line = parseInt(match[3]);
+} else {
+second_line = first_line;
+}
+
+// If we get something like #L20-L15, just swap the two line numbers so the loop will work
+if (second_line first_line) {
+var tmp = first_line;
+first_line = second_line;
+second_line = tmp;
+}
+
+
Bug#761103: debsources: highlight #Lxxx lines by default
Hello !
I have attached a patch that solves this bug.
From the commit message:
It supports two formats: #L50 and #L50-L150 (The latter will highlight
from line 50 to line 150).
It also changes the hash if we click on a line, and if we click on a
second line holding the SHIFT key, it will highlight the whole range,
and update the hash as well, allowing one to share the URL.
I have tested in Chromium 38, Iceweasel 31.2, and IE8 (I assume it
should work for later versions of IE)
Cheers
--
Jason Pleau
From 8bde6c68437ae6634297cdff321b495ed6e9a052 Mon Sep 17 00:00:00 2001
From: Jason Pleau ja...@jpleau.ca
Date: Sat, 1 Nov 2014 10:44:24 -0400
Subject: [PATCH] add automatic line numbers from #Lxx location.hash
It supports #L50 and #L50-L150 (The latter will highlight from line 50
to line 150).
It also changes the hash if we click on a line, and if we click on a
second line holding the SHIFT key, it will highlight the whole range,
and update the hash as well.
---
debsources/app/templates/source_file_code.inc.html | 117 -
1 file changed, 112 insertions(+), 5 deletions(-)
diff --git a/debsources/app/templates/source_file_code.inc.html b/debsources/app/templates/source_file_code.inc.html
index 9ea5a88..17bd3d0 100644
--- a/debsources/app/templates/source_file_code.inc.html
+++ b/debsources/app/templates/source_file_code.inc.html
@@ -19,17 +19,14 @@
tr
td
pre id=sourceslinenumbers{% for i in range(1, nlines+1) -%}
-a id=L{{ i }} href=#L{{ i }}{{ i }}/abr /
+a id=L{{ i }} href=#L{{ i }} class=linenumber data-line={{ i }}{{ i }}/abr /
{%- endfor %}/pre
/td
td
precode id=sourcecode class={% if file_language -%}
{{ file_language }}{% else %}no-highlight
{%- endif %}{% for (line, highlight) in code -%}
- {% if highlight -%}
- span class=highlight{{ line }}/span{% else -%}
- {{ line }}
- {%- endif %}
+span id=line{{ loop.index }} class=codeline {% if highlight -%} highlight {%- endif %}{{ line }}/span
{%- endfor %}/code/pre
/td
{% if msg -%}
@@ -46,3 +43,113 @@
{%- endif %}
/tr
/table
+
+script type=text/javascript
+function highlight_lines(start, end) {
+// First, remove the highlight class from elements that already have it
+var elements = document.querySelectorAll(span.highlight);
+for (i = 0; i elements.length; ++i) {
+var element = elements[i];
+element.className = element.className.replace(/\bhighlight\b/, '');
+}
+
+// Then, add the highlight class to elements that contain the lines we want to highlight
+for (i = start; i = end; ++i) {
+var element = document.getElementById(line + i);
+element.className = element.className + highlight ;
+}
+}
+
+var hash_changed = function(event, scroll) {
+
+event = typeof event !== 'undefined' ? event: null;
+scroll = typeof scroll !== 'undefined' ? scroll: false;
+
+// Will match strings like #L15 and #L15-20
+var regex = /#L(\d+)(-L(\d+))*$/;
+
+var match = regex.exec(window.location.hash);
+if (match != null) {
+var first_line = second_line = null;
+first_line = parseInt(match[1]);
+
+if (typeof match[3] !== 'undefined' match[3].length 0) {
+second_line = parseInt(match[3]);
+} else {
+second_line = first_line;
+}
+
+// If we get something like #L20-L15, just swap the two line numbers so the loop will work
+if (second_line first_line) {
+var tmp = first_line;
+first_line = second_line;
+second_line = tmp;
+}
+
+highlight_lines(first_line, second_line);
+
+if (scroll) {
+window.scroll(0, document.getElementById(L+first_line).offsetTop);
+}
+}
+}
+
+
+function change_hash_without_scroll(element, hash) {
+// This is necessary because when changing window.location.hash, the window will
+// scroll to the element's id if it matches the hash
+var id = element.id;
+element.id = id+'-tmpNoScroll';
+window.location.hash = hash;
+element.id = id;
+}
+
+var last_clicked;
+var line_click_handler = function(event) {
+if (event.preventDefault) {
+event.preventDefault();
+} else {
+event.returnValue = false;
+}
+
+var callerElement = event.target || event.srcElement;
+
+if (!event.shiftKey || !last_clicked) {
+last_clicked = callerElement;
+change_hash_without_scroll(callerElement, L + callerElement.getAttribute('data-line'));
+} else {
+var first_line = parseInt(last_clicked.getAttribute('data-line'));
+var second_line = parseInt(callerElement.getAttribute('data-line'));
+
+if (second_line first_line) {
+var tmp = first_line;
+first_line = second_line;
+second_line = tmp;
+}
+
+
Bug#761103: debsources: highlight #Lxxx lines by default
Package: qa.debian.org Severity: wishlist [ bug originally reported by Joachim Breitner ] URLs like http://sources.debian.net/src/cairo/1.12.14-4/boilerplate/cairo-boilerplate-xcb.c#L395 should highlight the line mentioned in the anchor by default, without requiring the user to pass ?hl. Ideally, the line should be put in the middle of the screen, instead of the top. (Implementing this likely requires some JavaScript fiddling.) Cheers. -- Stefano Zacchiroli . . . . . . . z...@upsilon.cc . . . . o . . . o . o Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o . « the first rule of tautology club is the first rule of tautology club » signature.asc Description: Digital signature
