Last Comment Bug 73330 - too much space at top of bugmail
: too much space at top of bugmail
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: 2.13
: All All
: P3 normal with 1 vote (vote)
: Bugzilla 3.4
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
: 198919 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-03-24 14:58 PST by Jesse Ruderman
Modified: 2010-02-28 10:53 PST (History)
12 users (show)
LpSolit: approval+
LpSolit: approval3.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix based on 2.18rc2 (1.99 KB, patch)
2004-10-11 19:22 PDT, Todd Stansell
no flags Details | Diff | Review
fix based on tip (2.20 KB, patch)
2004-10-11 21:32 PDT, Todd Stansell
no flags Details | Diff | Review
v3 - updated due to bitrot (1.99 KB, patch)
2005-03-05 06:08 PST, Todd Stansell
goobix: review-
Details | Diff | Review
v4 (1.33 KB, patch)
2008-12-02 00:26 PST, Max Kanat-Alexander
wicked: review-
Details | Diff | Review
v5 (802 bytes, patch)
2009-06-25 16:47 PDT, Max Kanat-Alexander
LpSolit: review-
Details | Diff | Review
v6 (1.50 KB, patch)
2009-08-18 13:02 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review

Description Jesse Ruderman 2001-03-24 14:58:40 PST
If someone adds comments to a bug but doesn't change any other fields, the 
bugmail I get has five blank lines between the bug URL and the "additional 
comments" line.  That forces me to scroll down just to see who made the 
change.  IMO, there should just be one blank line.
Comment 1 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 18:59:21 PDT
Mass moving to new product Bugzilla...
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-17 18:24:26 PST
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Comment 3 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-03-31 19:02:43 PST
Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed (preed@sigkill.com).  Jake will be offline for a few months.
Comment 4 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-03-23 23:55:38 PST
*** Bug 198919 has been marked as a duplicate of this bug. ***
Comment 5 Joel Peshkin 2004-03-18 16:14:01 PST
Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.
Comment 6 Todd Stansell 2004-10-11 19:22:25 PDT
Created attachment 161816 [details] [diff] [review]
fix based on 2.18rc2

Attaching our solution based on 2.18rc2.  This also changes slightly the look
of the changes for dependency bugs (prepends a |) so that it stands out a
little better.	I figured I'd submit this for anyone else who wanted a quick
fix.  This will need to get reviewed, etc. if it's going to get checked in
(which I can't do).

Todd
Comment 7 Todd Stansell 2004-10-11 21:32:41 PDT
Created attachment 161827 [details] [diff] [review]
fix based on tip

Updated based on the CVS tip and cleaned up a couple of minor whitespace issues
in the code.

Todd
Comment 8 Todd Stansell 2004-10-14 00:38:29 PDT
I just noticed that this patch undoes what bug 107580 created.  Here and I
thought it was an accident.  So, I'll need a new patch to not undo bug 107580...

Todd
Comment 9 Marc Schumann [:Wurblzap] 2004-11-19 02:30:57 PST
Comment on attachment 161816 [details] [diff] [review]
fix based on 2.18rc2

Removing r? from obsolete attachment.
Comment 10 Max Kanat-Alexander 2005-03-05 04:17:35 PST
As preed is no longer actively working on Bugzilla (from what I know), I somehow
doubt that this bug will make it in for 2.20. :-) If I'm wrong, feel free to
take it back and re-target it. :-)
Comment 11 Todd Stansell 2005-03-05 06:08:51 PST
Created attachment 176362 [details] [diff] [review]
v3 - updated due to bitrot

Bitrot has set in.  Here's a new patch that should apply cleanly to the tip. 
It includes the change to remove the extra space in $substs{"neworchanged"} now
that bug 31314 has landed.  No need to mess up the subject lines if real
threading support is there.

Todd
Comment 12 Vlad Dascalu 2005-06-07 14:41:28 PDT
Comment on attachment 176362 [details] [diff] [review]
v3 - updated due to bitrot

Hi Todd,

Sorry for the delay.

This patch includes 3 unrelated changes. We try to fill separate bugs for each
different change.

In this case, splitting this patch in 3 parts would make the review process
easier. There are too many unrelated changes in order to get to this fast
enough.

Regarding your first change:

-> you're adding a new char; would this be enough to change the 76 limit to 75?
Can multiple | chars be appended at the beginning of a line, in such a way that
75 would need to be lower than that? (I guess no but asking to be sure).


Regarding the second change:

-> this would change the sort-order in mail clients for those that sort by
Subject. It would split "new" mails into 2 groups: at the beginning of the
inbox, for those created before this modification goes in, and somewhere in the
middle, for those created after this modification goes in. Not saying that it's
something due to which review- should be granted, just saying this for
consideration.

Regarding your third change, I only have some questions right now:

-> should the regexps be multilined (the /m option at the end of the line, next
to the g option?)
-> depending on the answer to the first question, is the second regexp enough
in order to cover the \s* stuff in the 3rd one?


The actual reason for r- is the 3-changes-in-one-patch thing. As you can see
there are several issues to address (we want to make sure everything is ok
before commiting) and having separate bugs for each issue would help focus
discussion better.

If you could open bugs and create patches for the first 2 changes and keep this
bug for the 3rd change, it would be great.

I'd be happy to review the patch for the 3rd change, together with the answers
to those questions :-)
Comment 13 Max Kanat-Alexander 2008-12-02 00:26:20 PST
Created attachment 350938 [details] [diff] [review]
v4

Wow, I am amazed that this has never been fixed. In any case, it was really simple, here's the patch. I tested this with the following configurations:

* Adding an attachment during bug creation and requesting review, with a comment.
* Changing a bug with just a comment
* Changing a bug with just field changes
* Changing a bug with both field changes and a comment

And all of them look fine to me, now.
Comment 14 Max Kanat-Alexander 2008-12-02 00:27:27 PST
We could theoretically put this into 3.2, but it touches the email template, which a lot of people customize, and it's also one of the most-commonly-seen things in Bugzilla, and people tend to be touchy about things like that, so for now I'm targeting it to the trunk.
Comment 15 Reed Loden [:reed] (use needinfo?) 2008-12-02 00:32:58 PST
I want this for bmo, too.
Comment 16 Max Kanat-Alexander 2008-12-02 00:45:20 PST
Well, with at least that one voice, there are probably others who feel the same, particularly considering how old this bug is. How about we just fix it for 3.2.1, since it's a high gain for a low risk.
Comment 17 Wayne Mery (:wsmwk, NI for questions) 2008-12-05 05:10:54 PST
(In reply to comment #16)
> Well, with at least that one voice, there are probably others who feel the
> same

YaY!  You bet. Thanks for picking this up Max.

I thought I had plugged for this a long time ago - a serious annoyance, especially now that thunderbird is losing some vertical space. Now looking for an extension to remove extra blank lines from message display for the bugmail archive. If anyone finds such an extension or function please post in the bug.
Comment 18 Teemu Mannermaa (:wicked) 2008-12-27 13:34:11 PST
Comment on attachment 350938 [details] [diff] [review]
v4

Looks nice for all three bug change cases (comment only, diffs and comment and diffs only) but seems to produce funky output for new bugs:

---! quote start !---
https://landfill.bugzilla.org/w32/show_bug.cgi?id=5540

Summary: New bug filed!
    Classification: Unclassified
...
   Estimated Hours: 42.0
Testing new bug filing.

-- 
---! quote end !---

Like you can see, first field indentation is wrong and comment touches the last field.

>Index: template/en/default/email/newchangedmail.txt.tmpl
>===================================================================
>-[%+ diffs %]
>+[%+ FILTER trim %][% diffs %][% END %]

Is there a reason for this complex FILTER syntax? Is this trimming even necessary or would chomping of newlines suffice?
Comment 19 Wayne Mery (:wsmwk, NI for questions) 2009-06-25 11:28:09 PDT
:( missed v3.2.1 and 3.2.3.

With compact header now gone from native thunderbird, plus extensions like bugmail taking screen height, getting rid of bugmail's blanks lines will be an even greater blessing. so sev=minor>normal
Comment 20 Max Kanat-Alexander 2009-06-25 16:47:23 PDT
Created attachment 385273 [details] [diff] [review]
v5

Okay, this should fix it.
Comment 21 Frédéric Buclin 2009-07-16 19:00:36 PDT
Comment on attachment 385273 [details] [diff] [review]
v5

This still doesn't fix the problem reported in comment 0. If there is a comment only (and no field change), your patch makes no difference.

Note that parts of the bugmail look very close to each other when you change a field and comment. Not sure that's nicer. The bugmail looks very compact.
Comment 22 Wayne Mery (:wsmwk, NI for questions) 2009-08-10 00:17:28 PDT
Can we take the patch as is and leave this open for a follow up patch?  The major issue IMO is *not* when there is only a comment.
Comment 23 Max Kanat-Alexander 2009-08-18 13:02:57 PDT
Created attachment 395126 [details] [diff] [review]
v6

Okay, here we go! Tested in every configuration, seems to work fine and handle the problem everywhere.
Comment 24 Frédéric Buclin 2009-08-18 16:05:45 PDT
Comment on attachment 395126 [details] [diff] [review]
v6

Yes, works fine. r=LpSolit
Comment 25 Frédéric Buclin 2009-08-18 16:06:12 PDT
3.2 is restricted to security bugs.
Comment 26 Max Kanat-Alexander 2009-08-18 21:46:59 PDT
tip:

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.126; previous revision: 1.125
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.14; previous revision: 1.13
done

3.4:

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.124.2.2; previous revision: 1.124.2.1
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.12.2.2; previous revision: 1.12.2.1
done

Note You need to log in before you can comment on or make changes to this bug.