126792 - Templatise showdependencytree.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Gervase Markham [:gerv]]

What it says on the tin.

Gerv

Comment 1

[Tobias Burnus]

I'm trying to templatise it, but how should one templatise the "DumpKids" function?

It presently adds everything to $html. Since it is a tree and not a list, I
cannot use a simple [% FOREACH i = bugs %].

I could of cause say that one doesn't want to templatise this output and use
simply the $html. Opinions?

Comment 2

[Tobias Burnus]

Attachment: Templatized everything except <ul> output routine

So if anyone wants to modify the <ul> part of the html output (such as the
background colour of the resolved bugs) then you need to come up with a nice
way to do so. ;-)

Comment 3

[Gervase Markham [:gerv]]

Comment on attachment 73245 [details] [diff] [review]
Templatized everything except <ul> output routine

Tobias - thanks for doing this patch. It's a bit hard to review because it 
doesn't follow the established conventions for template formatting. There's no 
reason you should know what these are, as we haven't published the document 
detailing them yet :-) But check out any checked-in template to get the idea.

>+[%# Define strings that will serve as the title and header of this page %]
>+[% buglink = BLOCK %]<a href="show_bug.cgi?id=[% bugid %]">[% bugid %]</a>[% END %]
>+[% title   = BLOCK %]Dependency tree for Bug [% bugid %][% END %]
>+[%    h1   = BLOCK %]Dependency tree for Bug [% buglink %][% END %]
>+
>+[% INCLUDE global/header 
>+   title = title
>+   h1    = h1
>+%]

This is better done as:

>+[% INCLUDE global/header 
>+   title = "Dependency tree for bug $bugid"
>+   h1    = "Dependency tree for bug <a href='show_bug.cgi?id=$bugid'>$bugid</a>"
>+%]

>+
>+[% PROCESS depthControllToolbar %]

One "l" in Control :-)

>+[% h3   =  BLOCK   %]Bugs that bug [% buglink %] depends on[% END %]
>+[% PROCESS makeTreeHTML type = "depend" %
>+
>+
>+[% h3 = BLOCK %]Bugs that bug [% buglink %] blocks[% END %]
>+[% PROCESS makeTreeHTML type = "block" %]

You should pass the "depends on" or "blocks" string as another parameter and 
construct H3 inside the makeTreeHTML block.

>+[%# Provide feedback for omitted bugs %]
>+[% note_text = BLOCK %][% IF maxdepth || hide_resolved %]
>+<p><small><b>(Only [% hide_resolved ? "open" : "" %] bugs
>+[% maxdepth ? "whose depth is less than $maxdepth " : "" %]
>+will be shown)</b></small></p>
>+[% END %][% END %]

I can't read this :-) standard template formatting style would make it
something like:

<p>
  <small>
    <b>
      (Only [% hide_resolved ? "open" : "" %] bugs
      [% maxdepth ? "whose depth is less than $maxdepth " : "" %] 
      will be shown)
   </b>
  </small>
</p>


>+   <!-- Hide/show resolved button 
>+         Swaps text depending on the state of hide_resolved -->

Please use template comments ( [%# %] ) rather than HTML, to avoid bloating the
output.

As a general point, the idea of templates is to separate presentation and
style. 
You appear to have basically rewritten the Perl in template toolkit language,
which 
doesn't really achieve that.

The template needs to be easily hackable on - having lots of conditionals in it
doesn't aid that. Try putting all the logic in the Perl script, setting
booleans in
$vars, and just checking them with [% IF canedit %] or similar.

The best way of conditionally including text is:

[% "whose depth is less than " IF maxdepth %]

Gerv

Comment 4

[Gervase Markham [:gerv]]

Also, you've attached this patch to the wrong bug :-) Your patch is to
showdependency_tree_, and this bug is for showdependency_graph_ :-)

I think the best way to fix this is swap the bugs over.

Gerv

Comment 5

[Tobias Burnus]

Attachment: v2: New version

Remove $linked_id from .cgi (no longer needed), s/caneditbug/editbug/.

>+[%# Define strings that will serve as the title and header of this page %]
I took this from attachment/{edit,enter}.atml, but I changed it now.

In order to reduce the if's used in makeTreeHTML I split it into two blocks.
For the depthControlToolbar I do not yet completely know the best way get rid
of the if's, but I did minor improvements.

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 74301 [details] [diff] [review]
v2: New version

You need to indent your html/template stuff, for clearer reading, ie:

>+
>+[%###########################################################################%]
>+[%# Block for html tree building: Depends on                                #%]
>+[%###########################################################################%]
>+
>+[% BLOCK makeDependTree %]
>+<h3>Bugs that bug <a href='show_bug.cgi?id=$bugid'>$bugid</a> depends on
>+[% IF depend_html != "" %]
>+(<a href="buglist.cgi?bug_id=[% depend_keys %]">view as bug list</a>
>+[% IF canedit %] | <a href="buglist.cgi?bug_id=[% depend_keys %]&tweak=1">change several</a>[% END %])
>+[% END %]</h3>

should be:

[% BLOCK makeDependTree %]
  <h3>
    Bugs that bug <a href='show_bug.cgi?id=$bugid'>$bugid</a> depends on
    [% IF depend_html != "" %]
      (<a href="buglist.cgi?bug_id=[% depend_keys %]">view as bug list</a>
      [% IF canedit %]
	| <a href="buglist.cgi?bug_id=[% depend_keys %]&tweak=1">change
several</a>
      [% END %]
      )
    [% END %]
   </h3>

and so on. the canedit one could be on one line, except that it is then > 80
columns.

Comment 7

[Tobias Burnus]

Attachment: v3: Change Layout in .tmpl file and rename to .html.tmpl

> You need to indent your html/template stuff, for clearer reading
Done. (I think all lines are still/now <= 80 characters)

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 74518 [details] [diff] [review]
v3: Change Layout in .tmpl file and rename to .html.tmpl

This really isn't ready - it doesn't work (the template name is wrong and the
template doesn't compile), and when you fix that, there are loads of graphical
glitches (such as it printing rows of $ signs.)

More testing required, I'm afraid :-|

Gerv

Comment 9

[Tobias Burnus]

> This really isn't ready - it doesn't work
Here it works:
New version
http://bugzilla.physik.fu-berlin.de:8080/bugzilla/showdependencytree-new.cgi?id=98
vs. old version
http://bugzilla.physik.fu-berlin.de:8080/bugzilla/showdependencytree.cgi?id=98

> (the template name is wrong and the
In what sense? "dependencytree.html.tmpl" seems to be in the line with
template/default/buglist/

> template doesn't compile), and when you fix that,
> there are loads of graphical glitches (such as it printing rows of $ signs.)
With NS 4.79 and Opera 5 they look identically -- except for the cell after 'Max
Depth' (due to  ). -- And validator.w3.org says the HTML is ok.
(Mozilla is building I can test with it)

I believe that I created a working patch. The version I use on the server can be
fetch from: http://bugzilla.physik.fu-berlin.de:8080/bugzilla-patch/ (symlinks
to the running bugzilla).

Comment 10

[Gervase Markham [:gerv]]

> > (the template name is wrong and the
> In what sense? "dependencytree.html.tmpl" seems to be in the line with
> template/default/buglist/

In the sense that the CGI still says dependencytree.tmpl and so it can't find
the template! :-)

Are you sure you attached the right patch?

Gerv

Comment 11

[Tobias Burnus]

Attachment: v4: A really working patch (hopefully)

> Are you sure you attached the right patch?

I seemingly haven't. (I have definitly too many bugszilla directories lying
around!)

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 74564 [details] [diff] [review]
v4: A really working patch (hopefully)

>+       (Only [% "open" IF hide_resolved %] bugs
>+             [% "whose depth is less than $maxdepth" IF maxdepth %] 

You need a space before "whose", otherwise you get "bugswhose depth".

Fix that, and r=gerv.

Gerv

Comment 13

[Ville Skyttä]

Attachment: v5: fix ampersands in URLs

This one adds Gerv's review fix in as well as a couple of
& -> & in URL fixes.

Comment 14

[Matthew Tuck [:CodeMachine]]

Comment on attachment 74611 [details] [diff] [review]
v5: fix ampersands in URLs

> my $depend_html = makeTreeHTML($id, "dependson");
> $vars->{'depend_keys'} = join(',', keys %printed);

I don't really like this implicit parameter foo especially given neither return
value is more important than the other, wouldn't using a sub be better here, or
a multi-valued return?

> # Select maximum depth found for use in the toolbar
> $realdepth = $realdepth < $tmpdepth ? $tmpdepth : $realdepth;

Use max($realdepth, $tmpdepth)

> $vars->{'depend_keys'} = join(',', keys %printed);

Should be passed as a list, and comma separation should occur in the template.

I don't like the idea of continuing to generate HTML in the CGI.  This
prohibits XML output and so on.  I think the appropriate way to do this is to
get a tree and pass it to the template, and then let the template do a tree
walk using recursively called blocks, which I think TT can do.

Even if that's not possible, it would be better to pass a list of nodes and
indentation levels.

There will probably be more issues I have with this after these are fixed too.

Comment 15

[Matthew Tuck [:CodeMachine]]

   h1    = "Dependency tree for bug <a href='show_bug.cgi?id=$bugid'>$bugid</a>"

should probably be:

   h1    = "Dependency tree for bug <a
href=\"show_bug.cgi?id=$bugid\">$bugid</a>"

Even if we choose not to have full XHTML compatibility, we should be as close as
possible without failing to HTML-Validate.

Comment 16

[Ville Skyttä]

>  href='show_bug.cgi?id=$bugid'>$bugid</a>"

I don't think there is anything syntactically wrong in using single
quotes to with attribute values in XML, XHTML or older HTML versions
down to 2.0, it's has more to do with coding style preferences.
Of course, consistent style is Good. Personally, I prefer double quotes
over single ones.

See
http://www.w3.org/TR/REC-xml#NT-AttValue
http://www.w3.org/MarkUp/html-spec/html-spec_3.html#SEC3.2.4

> without failing to HTML-Validate.

I'd extend this a bit; without failing to HTML-validate and without
causing unnecessary compatibility problems.
Meaning that things like />'s and checked="checked" should be avoided
even though they may validate.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Folks, what's the status on this?  Tobias, can you post an updated patch taking
Matty's review comments into account?

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Taking.  I'm working on an updated patch.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: review fixes; all HTML has been templatized

>I don't really like this implicit parameter foo especially given neither
return
>value is more important than the other, wouldn't using a sub be better here,
or
>a multi-valued return?

Fixed.

>Use max($realdepth, $tmpdepth)

Fixed.

>Should be passed as a list, and comma separation should occur in the template.


Fixed.

>I don't like the idea of continuing to generate HTML in the CGI.  This
>prohibits XML output and so on.  I think the appropriate way to do this is to
>get a tree and pass it to the template, and then let the template do a tree
>walk using recursively called blocks, which I think TT can do.

Done.  

>   h1	  = "Dependency tree for bug <a
href='show_bug.cgi?id=$bugid'>$bugid</a>"
>
>should probably be:
>
>   h1	  = "Dependency tree for bug <a
>href=\"show_bug.cgi?id=$bugid\">$bugid</a>"

Quotations changed.  I'm a fan of double-quoting HTML attributes too, although
single quotes are in fact legal in both HTML and XHTML.

>I'd extend this a bit; without failing to HTML-validate and without
>causing unnecessary compatibility problems.

This is another bug, and not a 2.16 blocker, so it shouldn't block this fix.

Note that I had to re-write the tree-generating code to generate a data 
structure rather than HTML, which meant changing most of the code 
in the script, so you will probably find it easier to review the patched
script than the patch.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 78811 [details] [diff] [review]
patch v6: review fixes; all HTML has been templatized

A masterpiece of readability and clarity :-)

> if ($hide_resolved !~ /^\d+$/ || $hide_resolved != 1) { $hide_resolved = 0 };

if $hide_resolved !~ /^[0|1]$/ { $hide_resolved = 0 } ?

>+print "Content-Type: text/html\n\n";
>+$template->process("show/dependency-tree.html.tmpl", $vars)
>+  || DisplayError("Template process failed: " . $template->error())
>+  && exit;

ThrowTemplateError() :-)

>+    # If this bug has already been added to the tree someplace, don't add it again.
>+    return if $seen{$root_id};
>+    $seen{$root_id} = 1;

The "this bug appears elsewhere in the tree" thing no longer works? It seems
that bugs just don't appear the second time. Is this deliberate? Do you have
a better plan for this situation?

>+[% BLOCK depthControlToolbar %]
>+ <table cellpadding="3" border="0" cellspacing="0" bgcolor="#d0d0d0">
>+ <tr>
>+   [%# Hide/show resolved button
>+       Swaps text depending on the state of hide_resolved %]

This comment is about six lines too high in the file.

Currently, if the depth is "infinite", the "Infinite" button is clickable. 
It should probably be disabled.

Gerv

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: review fixes

>if $hide_resolved !~ /^[0|1]$/ { $hide_resolved = 0 } ?

Fixed.

>ThrowTemplateError() :-)

There's a bug (or should be) on converting these throughout the code, and this
one should be fixed along with the rest.

>The "this bug appears elsewhere in the tree" thing no longer works? It seems
>that bugs just don't appear the second time. Is this deliberate? Do you have
>a better plan for this situation?

Fixed.	Fixing this bug made me rewrite most of the code, but fortunately for
the better. :-)

>This comment is about six lines too high in the file.

Not really, as it refers to the whole table cell/form block used to display the
button.

>Currently, if the depth is "infinite", the "Infinite" button is clickable. 
>It should probably be disabled.

Yeah, and the depth text field should be blanked out, but this is another bug.

Comment 22

[Gervase Markham [:gerv]]

Comment on attachment 78970 [details] [diff] [review]
patch v7: review fixes

>+my $hide_resolved = $::FORM{'hide_resolved'};
>+$hide_resolved = $hide_resolved ? 1 : 0;

Nit:

+my $hide_resolved = $::FORM{'hide_resolved'} ? 1 : 0;

Same idea; simpler.

>+my $maxdepth = $::FORM{'maxdepth'} || 0;
>+if ($maxdepth !~ /^\d+$/) { $maxdepth = 0 };

You could also do:
$::FORM{'maxdepth'} =~ /^(\d+)$/ ? $1 : 0;
but now I think I'm getting silly :-)

>+[% BLOCK display_tree %]
>+<ul>
>+  [% FOREACH dep_id = tree.$bug_id.dependencies %]
>+    [% dep = tree.$dep_id %]
>+    <li>
>+      [% "<strike>" IF !dep.open %]
>+      <a href="show_bug.cgi?id=[% dep_id %]">[% dep_id %] 
>+        [[% dep.milestone FILTER html %], 

A single space and the above comma appear even if there are no milestones.
So you get: 
   9 [, [e-mail]] - [bleep] censors won't let me [bleep] install
new government

>+        [% dep.assignee_email FILTER html %]] - 
>+        [% IF dep.seen %]
>+          <i>&lt;This bug appears elsewhere in this tree.&gt;</i></a>

No full stop after tree - it looks odd.

I think it would be very cool to be able to enhance showdependencytree
to link "this bug appears elsewhere in this tree" to where it appears.
This wouldn't be hard; but it would cause confusion if, before, the entire
line was a link to a bug and then part of it became a page internal link.

My suggestion is to make it so that none of the titles are linked any more
- i.e. the links stop after the assignee. Then, later, we can link the
words "elsewhere in the tree" as an internal anchor, and there will be
no confusion.

Does that make sense?

Gerv

Comment 23

[Andreas Franke (gone)]

Attachment: patch to apply after v7: linkify "elsewhere in this tree"

I had the same idea as Gerv. This lets you jump to the first occurrence of the
bug by clicking on "elsewhere in the tree". Apply this patch to the version of
dependency-tree.html.tmpl that is generated by patch v7.

Comment 24

[Andreas Franke (gone)]

Comment on attachment 78970 [details] [diff] [review]
patch v7: review fixes

You need to add 'scriptname' to $vars, otherwise it won't work (at least on
4.x).

I have it installed together with the attachment above, and I think it's ready
to go, it's just nits.

I fear that with this patch we will recurse into multiple occurrences of
identical subtrees multiple times, once for each occurrence. (Not sure if it's
been that way before, but I don't think so, since there was the check for
"seen" during recursion.) The following may avoid it, but I'm not really sure
how it affects the correctness of $realdepth.
 showdependencytree.cgi.origpatch    Mon Apr 15 02:56:20 2002
+++ showdependencytree.cgi	Mon Apr 15 03:30:45 2002
@@ -88,6 +88,8 @@
 $vars->{'hide_resolved'}  = $hide_resolved;
 $vars->{'canedit'}	   = UserInGroup("editbugs");

+$vars->{'scriptname'}	   = "showdependencytree.cgi";
+
 print "Content-Type: text/html\n\n";
 $template->process("show/dependency-tree.html.tmpl", $vars)
   || DisplayError("Template process failed: " . $template->error())
@@ -101,6 +103,9 @@
     # Generates a dependency tree for a given bug.

     my ($bug_id, $relationship, $depth, $bugs) = @_;
+
+    # Short-circuit the tree generation if we've been here before
+    next if $bugs->{$bug_id};

     # Get the bug record from the database.
     my $bug = GetBug($bug_id);

Anyway, please carry forward my r=afranke to future versions of this patch. I
think this is very good work.

Comment 25

[Andreas Franke (gone)]

Comment on attachment 78970 [details] [diff] [review]
patch v7: review fixes

Sorry, ignore my previous comment about multiple recursions over identical
subtrees. I missed the "unless" clause in the recursion of GenerateTree.

Just an observation: If you click on "Show resolved", you generate a URL with
an "empty" parameter: "hide_resolved=" . In other cases, you get
"hide_resolved=0".
If I had the choice, I'd prefer either omitting the parameter completely, or
being consistent.

Comment 26

[Andreas Franke (gone)]

Maybe you could add a comment why the "next" in GenerateTree will always find a
containing loop (in particular, why the condition cannot be true if $bug_id=$id,
even if $bug{'exists'} is false). I think this is because in the case where
$bug_id=$id and $bug{'exists'} is false, then you can't get to this code because
it is catched in ValidateBugID($::FORM{'id'}). [It's just because I got a
runtime error when I tried what happens if I remove the $bug_id != $id part of
the condition.]

There is one strange thing with this patch, though. See
http://bugzilla.mathweb.org:8000/showdependencytree.cgi?id=61&hide_resolved=1
I think bug 75 shouldn't be shown there, since it is resolved.

Comment 27

[Andreas Franke (gone)]

Comment on attachment 78970 [details] [diff] [review]
patch v7: review fixes

Also note that all bug information is missing for the bugs that shouldn't be
displayed at all. Same problem here:
http://bugzilla.mathweb.org:8000/showdependencytree.cgi?id=75&hide_resolved=1

I think I should unmark the first-review until this is fixed. It doesn't seem
to be caused by my additional attachment since backing it out does not fix it.
The current CVS version of showdependencytree.cgi does not have this problem.

Comment 28

[Gervase Markham [:gerv]]

afranke: I like your patch (maybe Myk could roll it in) but I think that if we
are going to do that, we should unlinkify the bug summaries. This will make
what's going on much clearer, I think.

Gerv

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v8: review fixes

>+my $hide_resolved = $::FORM{'hide_resolved'} ? 1 : 0;

Fixed.

>A single space and the above comma appear even if there are no milestones.

Fixed.

>No full stop after tree - it looks odd.

The period is correct, since the sentence is complete; it's the angle
brackets that make it look odd (and they're also redundant with the italic).
Reverting nevertheless so this patch works like the old version.  I'll
fix this in a subsequent "grammar/formatting" clean-up patch.

>I think it would be very cool to be able to enhance showdependencytree
>to link "this bug appears elsewhere in this tree" to where it appears.

Good idea, but new bug.

>You need to add 'scriptname' to $vars, otherwise it won't work

Fixed by hard-coding the script name in the template, since no other script
uses this template.

>Just an observation: If you click on "Show resolved", you generate a URL with
>an "empty" parameter: "hide_resolved=" . In other cases, you get
>"hide_resolved=0".

Fixed.

>Maybe you could add a comment why the "next" in GenerateTree will always find
a
>containing loop.

Not sure what you mean, but I think this issue no longer exists.

>I think bug 75 shouldn't be shown there, since it is resolved.

Fixed.

Comment 30

[Gervase Markham [:gerv]]

Comment on attachment 79329 [details] [diff] [review]
patch v8: review fixes

> >A single space and the above comma appear even if there are no milestones.
> 
> Fixed.

It's not, you know :-).

>+[% BLOCK display_tree %]
>+<ul>
>+  [% FOREACH dep_id = tree.$bug_id.dependencies %]
>+    [% dep = tree.$dep_id %]
>+    <li>
>+      [% "<strike>" IF !dep.open %]
>+      <a href="show_bug.cgi?id=[% dep_id %]">[% dep_id %] 
>+        [[% dep.milestone FILTER html %], 

[% IF dep.milestone %]
  [% dep.milestone FILTER html %], 
[% END %]

Fix this for definite, and r=gerv.

Gerv

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v9: fixes dep.milestone

Oops, here's a patch with dep.milestone actually fixed.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 79363 [details] [diff] [review]
patch v9: fixes dep.milestone

r=gerv on this attachment per comment 30

Comment 33

[Andreas Franke (gone)]

Comment on attachment 79363 [details] [diff] [review]
patch v9: fixes dep.milestone

> >I think bug 75 shouldn't be shown there, since it is resolved.
>
> Fixed.

If I go to the URL now with patch v9 installed:
http://bugzilla.mathweb.org:8000/showdependencytree.cgi?id=61&hide_resolved=1
then indeed bug 75 is not displayed any more. But if I click on the "view as
bug list" link, then it's there again. 

Even worse on the second test url:
http://bugzilla.mathweb.org:8000/showdependencytree.cgi?id=75&hide_resolved=1
If I click on the "view as bug list" link there, I get all bugs, because it
refers to branch_id instead of bug_id. (Occurs twice in each of the patches.)

Comment 34

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v10: fixes Andreas' problem

Comment 35

[Andreas Franke (gone)]

Comment on attachment 79503 [details] [diff] [review]
patch v10: fixes Andreas' problem

Nice :-) I couldn't break it, and it's even more readable.

r=afranke

Comment 36

[Gervase Markham [:gerv]]

I'll give this second review after I've tried out interdiff on it and v.9 :-)

Gerv

Comment 37

[Gervase Markham [:gerv]]

Comment on attachment 79503 [details] [diff] [review]
patch v10: fixes Andreas' problem

r=gerv.

Gerv

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.16; previous revision: 1.15
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/dependency-tree.html.tmpl,v
done
Checking in template/default/show/dependency-tree.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/dependency-tree.html.tmpl,v
 <--  dependency-tree.html.tmpl
initial revision: 1.1
done

Comment 39

[Bradley Baetz (:bbaetz)]

Who forgot to run tests and add a version string....

Added under the 'fixes tree bustage if we had a tinderbox' rule, r=./runtests.sh
--verbose x2

--- dependency-tree.html.tmpl	17 Apr 2002 21:58:30 -0000	1.1
+++ dependency-tree.html.tmpl	17 Apr 2002 23:59:19 -0000
@@ -1,3 +1,4 @@
+<!-- [e-mail] -->
 [%# The contents of this file are subject to the Mozilla Public
   # License Version 1.1 (the "License"); you may not use this file
   # except in compliance with the License. You may obtain a copy of