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

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 (???)

of course I forgot the attachment :-)

well, launchpad discards the attachment. Here it is:

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

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 :(

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.