Closed Bug 83058 Opened 23 years ago Closed 23 years ago

need a way to hide resolved bugs in dependency tree view

Categories

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

2.13
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: afranke, Assigned: kiko)

References

Details

(Whiteboard: [relations:depend])

Attachments

(22 files, 1 obsolete file)

1.02 KB, patch
Details | Diff | Splinter Review
18.00 KB, text/html
Details
21.62 KB, text/html
Details
1.58 KB, patch
Details | Diff | Splinter Review
5.29 KB, patch
Details | Diff | Splinter Review
9.69 KB, patch
Details | Diff | Splinter Review
9.69 KB, patch
Details | Diff | Splinter Review
9.45 KB, patch
Details | Diff | Splinter Review
9.29 KB, patch
Details | Diff | Splinter Review
9.50 KB, patch
Details | Diff | Splinter Review
9.63 KB, patch
Details | Diff | Splinter Review
9.67 KB, patch
Details | Diff | Splinter Review
12.12 KB, patch
Details | Diff | Splinter Review
12.11 KB, patch
Details | Diff | Splinter Review
12.14 KB, patch
Details | Diff | Splinter Review
12.42 KB, patch
Details | Diff | Splinter Review
14.43 KB, patch
Details | Diff | Splinter Review
14.44 KB, patch
Details | Diff | Splinter Review
14.42 KB, patch
Details | Diff | Splinter Review
14.34 KB, patch
Details | Diff | Splinter Review
14.33 KB, patch
afranke
: review+
Details | Diff | Splinter Review
13.49 KB, patch
afranke
: review+
gerv
: review+
Details | Diff | Splinter Review
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.
Target Milestone: --- → Future
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.
Keywords: patch, review
Target Milestone: Future → ---
ooh, I like.  r= justdave when the tree is open again...
Assignee: tara → afranke
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
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.
Priority: P2 → P1
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.10
correcting version field lost in product move
Version: 2.10 → 2.13
Attachment #48269 - Attachment is obsolete: true
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.
Whiteboard: [relations:depend]
Keywords: ui
*** Bug 99210 has been marked as a duplicate of this bug. ***
Reassigning to default owner for review and checkin.
Assignee: afranke → myk
Blocks: 99207
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>.
> 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.
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
Status: NEW → ASSIGNED
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.
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?
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
r=doron on the UI on his behalf as QA.
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).
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=? 
Assignee: myk → kiko
Status: ASSIGNED → NEW
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.
> 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? :-)

Status: NEW → ASSIGNED
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.
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
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 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.
Attachment #52797 - Flags: review+
Comment on attachment 53120 [details] [diff] [review]
kiko_v17: removed whitespace from ( ). Please don't do this to me anymore. :~(

r=afranke
Attachment #53120 - Flags: review+
(spam) CC: Gerv for a 2r= attempt
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
Attachment #53120 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.9; previous revision: 1.8

Gerv
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?
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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: