Merge lp:~mwasilew/lava-scheduler/log_linenumbers into lp:lava-scheduler

[Milosz Wasilewski]

The patch line numbers to the full log view. There is also direct URL and line highlight.

[Antonio Terceiro (terceiro)]

Hi Milosz,

This is very nice, thanks!

Follows a few comments to improve your contribution.

 review needs-fixing

I don't have any code-specific comments, since I am going to suggest a
slightlly different approach which will probably change the details of
the code a lot.

Using a table breaks with very long lines (see attachment). The log is
already presented in preformated text, why don't you just prefix each
line with a number in a link?

i.e.

<pre>
  ...
  <a href='#L_X_Y'>$LINENO</a> {ACTUAL LOG LINE GOES HERE}
  ...
</pre>

You just need to make sure $LINENO has always the same numbers of
characters with the numbers aligned to the right, i.e. " 1", " 2", ...
" 10", ... "100". I would just pick a number of characters that can fit
the largest log you can think of, like 6, for a log of up to 10⁶ lines.

Doing this would simplify your code a lot ...

Also, why don't we use a direct counting instead of sectioned counting?
I think it's more natural for users.

I.e.

1
2
3
4

instead of

Section 1.1
Section 1.2
...

(even with the spaces between the sections, I think direct numbering is
still better)

Another problem with this approach is that it doesn't take into account
the case of when you are watching a log while the job is still running:
only the lines that were already available when you loaded the page get
line numbers, the new ones don't (see attachment). Note the poll
function right below #logfile_content.

if I were you I would do the following:

  when rendering the template, just prefix every line with a link whose
  content has a fixed width in number of characters. (you can still wrap
  each line in a div to get the nice :target effect)

  modify the poll() function to do the same on the client side, just
  incrementing a counter left in the HTML by the server-side code.

or maybe even doing everything on the client side already (???)

review: Needs Fixing

[Antonio Terceiro (terceiro)]

of course I forgot the attachment :-)

[Antonio Terceiro (terceiro)]

well, launchpad discards the attachment. Here it is:

http://people.linaro.org/~terceiro/tmp/linenumbers.png

[Milosz Wasilewski (mwasilew)]

Antonio,

Thanks for the comments.

I was aware that the line numbers are messed when the job is still running. As for the other comments:

- tables are used for purpose. It makes possible to select (copy) only the log content, not the line numbers. I hope they can stay :)
- sections are the easiest way of providing links. The problem is, that the whole content is divided into sections in the view (http://bazaar.launchpad.net/~linaro-validation/lava-scheduler/trunk/view/head:/lava_scheduler_app/views.py#L673). That can be still addressed with templatetag working on the set of sections. The original HTML output looks like this:

<a name="entry0"></a>
<pre class="log_log"><LAVA_DISPATCHER>2013-06-14 05:52:32 PM INFO: Attempting to connect to device </pre>
<a name="entry1"></a>
<pre class="log_console">Connected. </pre>
<a name="entry2"></a>
<pre class="log_log"><LAVA_DISPATCHER>2013-06-14 05:52:32 PM INFO: Matched 'Connected\\.\r'...

To sum up, I see 3 ways here:
1. apply this simple change which can be reverted once the complex solution (including this request https://bugs.launchpad.net/lava-scheduler/+bug/1089800) is available. I volunteer to provide this patch :)
2. apply your comments which might not be trivial and end up with complex solution that will need to be changed if the foldable sections are implemented.
3. don't apply this patch and go directly to complex solution. This will take some time though :(

[Antonio Terceiro (terceiro)]

On Fri, Aug 02, 2013 at 12:16:27PM -0000, Milosz Wasilewski wrote:
> To sum up, I see 3 ways here:
> 1. apply this simple change which can be reverted once the complex
> solution (including this request
> https://bugs.launchpad.net/lava-scheduler/+bug/1089800) is available.
> I volunteer to provide this patch :)

let's go baby steps then.

I will merge this so it will end up on staging on the next pulse.

=== modified file 'lava_scheduler_app/static/lava_scheduler_app/css/logfile.css'
--- lava_scheduler_app/static/lava_scheduler_app/css/logfile.css	2011-12-13 04:22:40 +0000
+++ lava_scheduler_app/static/lava_scheduler_app/css/logfile.css	2013-07-30 13:58:30 +0000
@@ -24,3 +24,13 @@
     border-width: medium;
 }
 
+.line 
+{
+    line-height: 18px;
+    white-space: nowrap;
+}
+
+div:target 
+{
+    background-color: rgb(255, 128, 128);
+}

=== modified file 'lava_scheduler_app/templates/lava_scheduler_app/job_log_file.html'
--- lava_scheduler_app/templates/lava_scheduler_app/job_log_file.html	2012-03-19 23:44:40 +0000
+++ lava_scheduler_app/templates/lava_scheduler_app/job_log_file.html	2013-07-30 13:58:30 +0000
@@ -6,6 +6,7 @@
 {% endblock %}
 
 {% block content %}
+{% load linenumbers %}
 <h2>Dispatcher log file - {{ job.id }} </h1>
 <a href="{% url lava.scheduler.job.log_file.plain job.pk %}">Download as text file</a>
 
@@ -15,7 +16,7 @@
 {% if section.0 == 'console' and section.1 > 20 and not forloop.last %}
 <a href="#entry{{ forloop.counter }}">skip {{ section.1 }} lines to next log entry &rarr;</a>
 {% endif %}
-<pre class="log_{{ section.0 }}">{{ section.2 }}</pre>
+{% linenumbers section.2 forloop.counter0 section.0 %}
 {% endfor %}
 {% if job.status == job.RUNNING %}
 <img id="progress" src="{{ STATIC_URL }}lava_scheduler_app/images/ajax-progress.gif"/>

=== added directory 'lava_scheduler_app/templatetags'
=== added file 'lava_scheduler_app/templatetags/__init__.py'
=== added file 'lava_scheduler_app/templatetags/linenumbers.py'
--- lava_scheduler_app/templatetags/linenumbers.py	1970-01-01 00:00:00 +0000
+++ lava_scheduler_app/templatetags/linenumbers.py	2013-07-30 13:58:30 +0000
@@ -0,0 +1,45 @@
+from django import template
+from django.db.models import fields
+from django.utils.html import escape
+from django.utils.safestring import mark_safe
+
+register = template.Library()
+
+class LineNumbers(template.Node):
+    def __init__(self, text, prefix, style):
+        self.text = template.Variable(text)
+        self.prefix = template.Variable(prefix)
+        self.style = template.Variable(style)
+
+    def render(self, context):
+        text = self.text.resolve(context)
+        prefix = self.prefix.resolve(context)
+        style = self.style.resolve(context)
+        ret = "<table><tr><td>"
+        for i in range(0, len(text.splitlines())):
+            name = "L_%s_%s" % (prefix, i)
+            display = "Section %s.%s" % (prefix, i)
+            ret += "<div class=\"line\"><a name=\"%s\"></a> \
+                <a href=\"#%s\">%s</a></div>" % (name, name, display)
+
+        ret += "</td><td class=\"code\"><div class=\"containter\"> \
+            <pre class=\"log_%s\">" % (style)
+
+        for index, line in enumerate(text.splitlines()):
+            ret += "<div id=\"L_%s_%s\" class=\"line\"> &nbsp;%s</div>" % \
+                (prefix,
+                 index,
+                 mark_safe(escape(line).encode('ascii', 'xmlcharrefreplace')))
+
+        ret += "</pre></div></td><tr></table>"
+
+        return ret
+
[e-mail]('linenumbers')
+def do_linenumbers(parser, token):
+    try:
+        tag_name, text, prefix, style = token.split_contents()
+    except ValueError:
+        raise template.TemplateSyntaxError("%r tag requires 3 arguments" % \
+            token.contents.split()[0])
+    return LineNumbers(text, prefix, style)