83058 - need a way to hide resolved bugs in dependency tree view

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Andreas Franke (gone)]

In showdependencytree.cgi, resolved bugs are shown with a strikethough style.
This makes sense if the dependency tree is reasonably small, but it is a big
problem for huge dependency trees where almose all bugs are resolved.

There needs to be a way to view the dependency tree without all the subtrees
that consist only of resolved bugs.

Comment 1

[Andreas Franke (gone)]

Attachment: 2-liner to change the background color for resolved bugs

Comment 2

[Andreas Franke (gone)]

This can't be so hard, can it? If nobody has time to implement this, then please
check in something like the above hack to bugzilla.mozilla.org so that one can
actually use the dependencytree feature on large tracking bugs. Otherwise please
tell me how to get a list of open bugs on this page
	http://bugzilla.mozilla.org/showdependencytree.cgi?id=79119
without saving the resulting html and doing a query-replace-regexp manually...

Adding patch/review, clearing Future milestone.

Comment 3

[Andreas Franke (gone)]

Attachment: dependency tree of bug 79119 _before_ the above patch

Comment 4

[Andreas Franke (gone)]

Attachment: dependency tree of bug 79119 _after_ the above patch

Comment 5

[Dave Miller [:justdave]]

ooh, I like.  r= justdave when the tree is open again...

Comment 6

[Andreas Franke (gone)]

Attachment: better fix for this bug, includes previous patch

Comment 7

[Andreas Franke (gone)]

This new patch adds support for an (optional) parameter 'prune_resolved'. If set
to "on", all resolved bugs are skipped, together with their complete subtrees
(i.e. no matter whether there are any unresolved bugs buried in the subtree).
If the parameter prune_resolved is not passed to the cgi, then an additional
link is generated to activate this feature.

It would be nice if this patch could make it in immediately after the request
tracking stuff, so that it is "in" for that b.m.o upgrade. Hopefully it will
improve usability for tracking bugs immensely.

Feel free to clean up the wording of the link if you have a better suggestion.

Comment 8

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 9

[Andreas Franke (gone)]

Attachment: work in progress, comments welcome

Comment 10

[Andreas Franke (gone)]

Attachment: latest patch for review

Comment 11

[Andreas Franke (gone)]

The latest patch 
- adds a new depth parameter to DumpKids
- has support for a 'maxdepth' CGI parameter
- uses IsOpenedState instead of comparison with NEW/ASSIGNED/REOPENED
- has a form at the bottom of the page to alter the parameters
- has some links at the top for frequently used parameter modifications 
- prints "None" if no bugs are found

Please review and checkin ;-). If you like, modify it before doing so. I'm
sorry, but I can't spend any more time on this. Feel free to change the UI
anyway you like; what I really need from b.m.o is the two links, preferably at
the top, to
-> hide resolved bugs, and 
-> limit recursion depth dramatically.
The first one is even more important to me, so, if you are conservative, you may
consider attachment 46125 [details] [diff] [review] first. Please _do_ check in _something_, so that
dependency views for tracking bugs at bugzilla.mozilla.org become usable again
soon. Thanks.

Comment 12

[Andreas Franke (gone)]

*** Bug 99210 has been marked as a duplicate of this bug. ***

Comment 13

[Andreas Franke (gone)]

Reassigning to default owner for review and checkin.

Comment 14

[Matthew Tuck [:CodeMachine]]

My comments here are:

Why do we need the links?  They are redundant given the submit section, and I
think they get in the way.

Do we still need the grey background on the submit paraphenalia when you added
the <HR>?  It's rather out of place with the rest of Bugzilla.

Would also be nice for a blank line before the <HR>.

Comment 15

[Andreas Franke (gone)]

> Why do we need the links?  They are redundant given the submit section,
> and I think they get in the way.

On every bug page, we have several redundant links:
"Bug List: [...] First Last Prev Next   Show list   Query page   Enter new bug"
are both at the top and at the bottom of every page, but this is for a reason:
It improves usability. You wouldn't want to have to scroll down the entire page
to be able to click on "Next", or any other of these links.

The links on the dependency tree view are there for the same reason: When you
get a very, very long dependency tree page, you don't want to have to scroll to
the bottom just to get this page down to a reasonable size. In 99% of these
cases, you will want to perform exactly one of the two operations offered by
these links: Either hide all resolved bugs, or limit the depth to 1.
And for these operations, the links provide a better usability than the form:
It takes only one click to perform an operation, whereas in the form you either
need two clicks (in the "hide resolved bugs" case), or even more: a double click
to select-highlight the number in the depth text input field, hitting the key
"1", and another click on submit. 

That's why the links are there, and I don't think they get in the way. If you
still don't agree, then what about the following solution:
Remove the links from the dependency tree view page, and instead put them
directly on the bug view page, like this:

	...			_Show dependency tree_ _(Open bugs)_ _(Depth 1)_

or something similar. This may even be better, since it makes you need only one
click to get your desired shortened view, instead of two before.

> Do we still need the grey background on the submit paraphenalia when you added
> the <HR>?  It's rather out of place with the rest of Bugzilla.

It's not necessary, but I like it because it makes it easy to spot the form on
the page. If you don't like it, simply remove the "bgcolor=#d0d0d0" from the
patch.

> Would also be nice for a blank line before the <HR>.

On my Netscape 4.7x, it looks fine as-is, see
http://bugzilla.mathweb.org:8000/showdependencytree.cgi?id=76
However, feel free to change the layout any way you like, 
e.g. by inserting a <P> before the <HR>.

I have made the UI the way I like it, but I don't mind if whoever finally makes
the decision modifies it, as long as the functionality and the easy
accessibility is preserved.

Comment 16

[Christian Reis]

Okay, this looks good, though I want to try and get mpt to comment on the UI if
he has the time.

The code looks quite clean and legible. One thing is that the default of 1000
for depth seems pretty deep, and doesn't fit in the INPUT box you provide for
the UI.

Good url for understanding the problem:
http://bugzilla.mozilla.org/showdependencytree.cgi?id=7954

Comment 17

[Matthew Tuck [:CodeMachine]]

I suggest that you should display the level the user set in the field as is
rather than trying to reduce it to the actual max level - it's too confusing.

Also, I think you should display unlimited as NONE in the (otherwise) numeric
box specifying the max level.

Comment 18

[Christian Reis]

Okay, I've changed the UI part of this patch to implement what I think is a nice
way to navigate through dependencies. 

- There is a top and bottom-repeated toolbar (MattyT: for very large bugs this
is important and it's fairly standard)
- As Andreas has requested, it is a one-click operation to unlimit the depth and
unhide resolved bugs.
- 0 and bogus values passed for maxdepth are checked for and cleaned up. 0 is
special and means "unlimited".
- bogus values passed for hide_resolved are checked and set to 0.

This design has been guided somewhat by mpt, but he would rather we use a
radiobutton to select between unlimited and bound. I'm not sure about this and
it does add mouse clicks to the operation, and since this is Andreas' bug, I
ddn't do it.

Boris and Jake have reviewed the UI and found it acceptable.

Of course, the ideal would be for each stack of bugs to collapsable via some
sort of twisty, but I make it clear that this is completely out of the scope of
this bug. Please r= my perl as it's usually broken, though this time I have
taint turned on and no problems seem to have occured.
One question that bears to zach's scripts and taint in general. I have added a
-T to the perl exec line and now the test fails for it with a:

Too late for "-T" option at showdependencytree.cgi line 1.
not ok 50 - showdependencytree.cgi--ERROR
#     Failed test (t/001compile.t at line 69)

Should I leave taint on or not?

Comment 19

[Christian Reis]

Attachment: kiko_v1: patch implements full navigation, hideable resolution, taintmode on

Comment 20

[Christian Reis]

This patch is applied to
http://bugs.async.com.br/showdependencytree-83058.cgi?id=172 and can be viewed
there for any public bug. Unfortunately there aren't many public bugs with
dependencies, but I have tested on a private bug with lots and it's really nice.

Andreas, I would like your feedback, and i'm now waiting for r=/sr= and checkin

Comment 21

[Christian Reis]

Attachment: kiko_v2: fix dain bramage in parameter. this works.

Comment 22

[Christian Reis]

Attachment: kiko_v3: perl changes and cleanups per Jake's review

Comment 23

[Christian Reis]

Attachment: kiko_v4: removed hash length function for perl keys

Comment 24

[Christian Reis]

Attachment: kiko_v5: makes this all valid HTML

Comment 25

[Christian Reis]

Attachment: kiko_v6: added scriptname to ACTION fixes it for ns4

Comment 26

[Christian Reis]

Attachment: Fixes problems with NS4. 0 errors on w3c validator, tested cross browser.

Comment 27

[Christian Reis]

r=doron on the UI on his behalf as QA.

Comment 28

[Andreas Franke (gone)]

Comment on attachment 52461 [details] [diff] [review]
Fixes problems with NS4.  0 errors on w3c validator, tested cross browser.

>+    my $bgcolor = "#d9d9d9";

You have changed the bgcolor from #b0b0b0 to #d9d9d9. I think this is less 
usable if you are interested in the open bugs only, but since there is now 
a button to hide all resolved bugs entirely, I don't care about this any more.

Note that having resolved bugs shown is useful to find resolved bugs depending 
on open ones. (This should never happen, but in this case theory and reality 
don't match.) IMO these situations can be more easily detected if the contrast 
is high, but this is a minor issue, so you can leave it as-is if you like.

>+    my $hide_me = 0;

>+        if ( $maxdepth && $depth > $maxdepth) {
>+            # if we specify a maximum depth, we hide the output when
>+            # that depth has occured, but continue recursing so we know
>+            # the actual depth of the tree.
>+            $hide_me = 1;
>+        }

I don't understand how this can work: I can't find the corresponding place 
where you set $hide_me back to 0 again. You possibly lose a lot of entries, 
don't you?
What about an 
          } else { $hide_me = 0};
?
Or even
          $hide_me = ($maxdepth && $depth > $maxdepth);
?

>+        my $list_started = 0;

>+                if ( ! $list_started ) { $html .= "<ul>"; $list_started = 1 }

Is there a reason to avoid empty <ul></ul> pairs? 

>+            if ( ! $hide_me ) { 

I don't like these countless "if (!$hide_me)" very much. Is there a better way?
Maybe you can do with only a single one if you bracket the whole chunk of code
with this?

>+            $realdepth = $realdepth < $depth ? $depth : $realdepth;

For code readability, doesn't perl have a "max" function?

>+    return $truncmsg;

Does $truncmsg really have to be a separate thing? I don't think it's natural
for the DumpKidsTree routine to return this string. Either it should be part
of the $html (I don't care whether at the beginning or at the end), or it 
should be just a boolean or an integer, and the string should be created from
it later.

>+    my $scriptname = $::ENV{'SCRIPT_NAME'}; # showdependencytree.cgi

Since the $scriptname doesn't change, it can be assigned outside the
drawDepForm, can't it?

Maybe drawDepForm can make use of a template? Ok, that's probably out of the
scope of this bug... :)

>+    if ( $hide_resolved ) {
>+        $prune_input = '<input type="hidden" name="hide_resolved" value="0">';
>+        $prune_msg = "Show Resolved";
>+    } else {
>+        $prune_msg = "Hide Resolved";
>+        $prune_input = '<input type="hidden" name="hide_resolved" value="1">';
>+    }

Why do $prune_msg and $prune_input occur in a different order in the else case?
Since you have renamed the parameter from "prune_resolved" to "hide_resolved",
it seems a bit odd to me to name the variables "prune_xxx".

The drawDepForm looks a bit long and verbose to me, but that may be necessary.
Btw, using NS4/Linux, all buttons seem to be enabled all the time. Maybe I was
wrong about the "disabled" attribute?

>+# check to see if realdepth for depended bugs is larger and update
>+$realdepth = $realdepth < $tmp ? $tmp : $realdepth;

What happens if you don't clear $realdepth and simply drop the $tmp stuff? 
Wouldn't this give the same result? But anyway, your way may be cleaner code.

>+drawDepForm($maxdepth);

Why do you pass $maxdepth as a parameter, but not $hide_resolved? 
I'd say either both or none. In the long run, we should add a comment
at the beginning of each function that mentions all the variables from
outside that are used (other than the explicit parameters).

Comment 29

[Christian Reis]

Attachment: kiko_v8: code reorganization, comments and cleanups as per Andreas' comments

Comment 30

[Christian Reis]

Attachment: kiko_v9: fix minor issue with depth in last patch

Comment 31

[Christian Reis]

Andreas, thanks a lot for the comments. Let me try and get an r=afranke here:

> You have changed the bgcolor from #b0b0b0 to #d9d9d9. I think this is less
> usable if you are interested in the open bugs only, but since there is now
> a button to hide all resolved bugs entirely, I don't care about this any more.

I did do on request from the some mozilla QA people (doron and bz IIRC)
that said the contrast with the blue and purple link colors were bad, but   
I singled it out as a variable to be easily changeable if you would like
to do so on your installation.

> >+    my $hide_me = 0;
>
> >+        if ( $maxdepth && $depth > $maxdepth) {
> >+            # if we specify a maximum depth, we hide the output when
> >+            # that depth has occured, but continue recursing so we know
> >+            # the actual depth of the tree.
> >+            $hide_me = 1;
> >+        }
>           
> I don't understand how this can work: I can't find the corresponding place
> where you set $hide_me back to 0 again. You possibly lose a lot of entries,
> don't you?
 
No, $hide_me is set outside the loop and doesn't need an else clause
since it is evaluated only once. Basically, this means that we won't
show information for any bug's dependents which are deeper than maxdepth.

> >+        my $list_started = 0;
> 
> >+                if ( ! $list_started ) { $html .= "<ul>"; $list_started = 1$
> 
> Is there a reason to avoid empty <ul></ul> pairs?
  
Yes. It's against the HTML4 transitional specification, and considered good
practice.

> 
> >+            if ( ! $hide_me ) {
> 
> I don't like these countless "if (!$hide_me)" very much. Is there a better wa$
> Maybe you can do with only a single one if you bracket the whole chunk of code
> with this?

Much better, done.

> >+            $realdepth = $realdepth < $depth ? $depth : $realdepth;
> 
> For code readability, doesn't perl have a "max" function?

Unfortunately, no.
> >+    return $truncmsg;
> 
> Does $truncmsg really have to be a separate thing? I don't think it's natural
> for the DumpKidsTree routine to return this string. Either it should be part
> of the $html (I don't care whether at the beginning or at the end), or it
> should be just a boolean or an integer, and the string should be created from
> it later.

Well, it had to be, since we had to insert it before the html. But I've changed
that section a lot and I think you'll like how it turned out. Basically,
DumpKidsTree became makeTreeHTML and it now generates _all_ HTML for that tree.

> >+    my $scriptname = $::ENV{'SCRIPT_NAME'}; # showdependencytree.cgi
> 
> Since the $scriptname doesn't change, it can be assigned outside the
> drawDepForm, can't it?

Yeah, right. Duh.

> Why do $prune_msg and $prune_input occur in a different order in the 
> else case? Since you have renamed the parameter from "prune_resolved"
> to "hide_resolved", it seems a bit odd to me to name the variables
> "prune_xxx".

Duh. Fixed.

> The drawDepForm looks a bit long and verbose to me, but that may be 
> necessary. Btw, using NS4/Linux, all buttons seem to be enabled all
> the time. Maybe I was wrong about the "disabled" attribute?

It's long and verbose, but that's needed for some of the forms intricacies and
usability, IMO. And Mozilla does honour disabled, even if NS4 doesn't, so I left
it in. The form still works normally on NS4.

> >+# check to see if realdepth for depended bugs is larger and update
> >+$realdepth = $realdepth < $tmp ? $tmp : $realdepth;
> 
> What happens if you don't clear $realdepth and simply drop the $tmp stuff?   
> Wouldn't this give the same result? But anyway, your way may be cleaner code.

The problem is that realdepth can be larger in either one of the sections, so I
really need to check which one (blockers or dependents) is deeper.

> >+drawDepForm($maxdepth);
> 
> Why do you pass $maxdepth as a parameter, but not $hide_resolved?
> I'd say either both or none. In the long run, we should add a comment
> at the beginning of each function that mentions all the variables from
> outside that are used (other than the explicit parameters).

Ok, removed parameter. I've documented all functions to state what they do, what
locals and what globals they modify. So I guess this is almost a holy rewrite.

Oh, I've changed the parameters to dumpKids() - I removed the confusing second
parameter and switch it internally, since it was ultimately bound to the first.
Am I ready for r=? 

Comment 32

[Christian Reis]

Attachment: kiko_v10: cosmetic fixes (thanks caillon)

Comment 33

[Christian Reis]

Attachment: kiko_v11: add style comments to make it behave even when using strict dtd viewing mode

Comment 34

[Andreas Franke (gone)]

Re: $hide_me

Ok, I can see now how this works. You compute $hide_me for every node in the
tree, and then you have a if (! $hide_me) block. So I'd say you don't need that
variable at all. Move the comments from the $hide_me = 1 to the beginning of
this if block, and replace (! $hide_me) by its definition. 
Btw, you forgot to remove one of these ifs:
   if ( ! $hide_me ) { $html .= qq|<strike><span style="color: $fgcolor; ...

>DumpKidsTree became makeTreeHTML and it now generates _all_ HTML for that tree.

Nice.

+# drawDepForm()
+#
+# params
+#   none
+# globals modified
+#   none

:-) I think "globals used (but not modified)" would be interesting as well here.

>+# get dependents
>+my $block_html = makeTreeHTML( $id, $linked_id, "blocked" );

>+# get blockers
>+my $depend_html = makeTreeHTML( $id, $linked_id, "dependson" );

> I've changed the parameters to dumpKids() - I removed the confusing second
> parameter and switch it internally

Are you sure that the comments and the calls match? Later on, you print
$depend_html before $block_html. Another problem with this is that now there are
two places were "blocked" and "dependson" are hardcoded.

>+#       will generate HTML for the 

This is incomplete.

>+# makeTreeHTML calls dumpkids 

should be DumpKids :-)

>+# DumpKids( id, target, 1 )

could be depth instead of 1

I do not like that you use $id both as a local variable and as a global
variable; especially since there are cases where they have different values.
But this is just programming style, so you may have a different opinion.

Anyway, r=afranke on all future versions of this patch, if you remove the
redundant if (! $hide_me) occurrence.

Unfortunately, I think this needs a second review.

Comment 35

[Christian Reis]

Attachment: kiko_v12: comment fixes thanks to timeless, changed feedback message, fixed to afranke's comment

Comment 36

[Christian Reis]

Attachment: kiko_v13: fixed minor nits

Comment 37

[Christian Reis]

> Ok, I can see now how this works. You compute $hide_me for every node in the
> tree, and then you have a if (! $hide_me) block. So I'd say you don't need 
> that variable at all. Move the comments from the $hide_me = 1 to the 
> beginning of this if block, and replace (! $hide_me) by its definition.

Fixed.

> Btw, you forgot to remove one of these ifs:
>   if ( ! $hide_me ) { $html .= qq|<strike><span style="color: $fgcolor; ...
Fixed (duh).

> :-) I think "globals used (but not modified)" would be interesting as well 
> here.

Added.

> Are you sure that the comments and the calls match? Later on, you print
> $depend_html before $block_html.

Comments were unclear, order swapped. Fixed.

> Another problem with this is that now there are
> two places were "blocked" and "dependson" are hardcoded.

Well, considering the fact that they _are_ hardcoded in multiple places
(including the function calls!) I don't think this overweighs the _much_ cleaner
API to the function. Won't fix. :-)

> >+# DumpKids( id, target, 1 )
> could be depth instead of 1

Made depth be by default 1 if not passed in, removed horrible 1 in function
call. Fixed.

> I do not like that you use $id both as a local variable and as a global
> variable; especially since there are cases where they have different values.
> But this is just programming style, so you may have a different opinion.

I don't. This was stupid. It is now fixed.

Timeless did a _very_ thorough hack of my comments and they are now reasonably
cleaned up. If you pick something up let me know. Andreas, if you will r= this
(thanks) please mark so on the attachment, granted?

One last thing. I have changed the way feedback is provided for hidden bugs and
and the phrase printed out is either:

(Bugs marked resolved will be omitted.)

(Bugs with depth > 1 will be omitted.)

or

(Bugs marked resolved or with depth > 1 will be omitted.)

I know this is a subtle change but IMHO either we provide proper feedback on
what is hidden, or none at all. I think this change has improved usability, but
I know it's different from what your original intention was.

How many patches will I have to send, I wonder, before this is done? :-)

Comment 38

[Christian Reis]

Attachment: kiko_v14: one more for luck. removed unused variable. it's perfect now (sure).

Comment 39

[Christian Reis]

Attachment: kiko_v15: rolled back one fix, changed feedback message

Comment 40

[Christian Reis]

Attachment: kiko_v16: stop being stupid

Comment 41

[Christian Reis]

Okay, I had foobared the fix for $hide_me that was suggested. That is fixed, and
the feedback message for cleared bugs is now cleaner.

Desperately seeking r=/sr= on this. I hope we don't go to three digit versions
on this patch.

Comment 42

[Gervase Markham [:gerv]]

kiko: can bug 30480 be rolled into this? If you can limit the number of bugs
listed, then being able to turn that limited list into a buglist would be
extremely useful.

Gerv

Comment 43

[Christian Reis]

Gerv, this patch is big enough as it is, IMHO. I would be very pleased to work
on bug 30480 and have this implemented (it's not very hard) as soon as this gets
checked in; managing my open bugs for review is a lot of work (updating,
un-bitrot-ing, etc).

Comment 44

[Andreas Franke (gone)]

Comment on attachment 52797 [details] [diff] [review]
kiko_v16: stop being stupid

rs=afranke

I had reviewed v.11, and I don't have the time to re-review the complete patch again.I tried to find out the diffs between v.11 and this one, but due to style changes nearly everything    seemed to have changed.
So I'll rubber-stamp this one. Assuming you have tested the patch, and are willing to fix any minor issues that should arise as a fall-out from this, I'd say let's check it in asap.

(I should note however that I don't like the new brackets style: I think (x, y, z) is at least as good as, if not more readable than ( x, y, z ).
But that's a style question, so if the decision has been made, that's ok.

If someone is willing to check this in with a second rubber-stamp review, you have my promise that I'll help fix any fallout issues.

Let's do it.

Comment 45

[Christian Reis]

Attachment: kiko_v17: removed whitespace from ( ). Please don't do this to me anymore. :~(

Comment 46

[Andreas Franke (gone)]

Comment on attachment 53120 [details] [diff] [review]
kiko_v17: removed whitespace from ( ). Please don't do this to me anymore. :~(

r=afranke

Comment 47

[Christian Reis]

(spam) CC: Gerv for a 2r= attempt

Comment 48

[Gervase Markham [:gerv]]

Comment on attachment 53120 [details] [diff] [review]
kiko_v17: removed whitespace from ( ). Please don't do this to me anymore. :~(

r=gerv. Let's get it in and see what happens :-)

Gerv

Comment 49

[Gervase Markham [:gerv]]

/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.9; previous revision: 1.8

Gerv

Comment 50

[Paul Baker]

let me get this straight...if you choose to hide resolved bugs, a resolved bug
is hidden even if it has children that are still open? isn't that kinda bad?
granted bugs should not be marked resolved if a child bug isn't, but since
bugzilla allows that shouldn't we make it a point to...point that out?

Comment 51

[Christian Reis]

Paul, you are accurate in pointing it out. However, in this patch
I don't wander down the list of children for open bugs that could
block us. Right now, I don't think it's very important; it could
be argued that it should be impossible to resolve a bug with open
children (there's bug 24496 right there) that would simplify the 
matter.

Right now I am reluctant to fix this locally in showdependencytree
where the rest of the Bugzilla treats dependent bugs rather independently.

At any rate, if you really think this should be fix, open a new bug
and CC me so we can hash it out.