Last Comment Bug 416669 - Port "Bug 350661 - Add Recent Folder Target [...]" to SeaMonkey
: Port "Bug 350661 - Add Recent Folder Target [...]" to SeaMonkey
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.0a1
Assigned To: Serge Gautherie (:sgautherie)
:
:
Mentors:
Depends on: 350661
Blocks: 401365
  Show dependency treegraph
 
Reported: 2008-02-10 06:41 PST by Serge Gautherie (:sgautherie)
Modified: 2008-04-18 23:09 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) <mailWindowOverlay.xul> ++ (12.76 KB, patch)
2008-02-10 08:00 PST, Serge Gautherie (:sgautherie)
mnyromyr: review+
iann_bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
(Bv1-SM) <messenger.dtd> reordering (5.34 KB, patch)
2008-03-27 18:00 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv1a-SM) <messenger.dtd> reordering (4.37 KB, patch)
2008-03-29 07:23 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv1b-SM) <messenger.dtd> reordering + 1 removal (8.63 KB, patch)
2008-03-31 09:37 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv1c-SM) <messenger.dtd> reordering + 1 removal [Checkin: Comment 19] (5.37 KB, patch)
2008-04-02 12:35 PDT, Serge Gautherie (:sgautherie)
neil: review+
Details | Diff | Splinter Review
(Av1a) <mailWindowOverlay.xul> ++ [Checkin: Comment 21] (18.28 KB, patch)
2008-04-04 11:36 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Cv1-TB) Removes |junkMailCmd| remnants [Checkin: See comment 26] (2.85 KB, patch)
2008-04-09 05:03 PDT, Serge Gautherie (:sgautherie)
philringnalda: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2008-02-10 06:41:54 PST
Per bug 401365 comment 21.
Comment 1 Serge Gautherie (:sgautherie) 2008-02-10 08:00:12 PST
Created attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008020901 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

What I did:
1- Have a look at the 4 initial patches from bug 350661.
2- Sync SM file(s) from current TB file.
3- Add my bug 350661 attachment 302411 [details] [diff] [review] "nit and fix".

What I did not:
*"Fully" sync the code indentation. (Leave it to bug 401365.)
*Try and look very long at the "little" diff in the TB code since the initial patches were checked in: like new |id=|...
Comment 2 Serge Gautherie (:sgautherie) 2008-02-17 06:12:13 PST
Any comment/review on my patch ?
Comment 3 Serge Gautherie (:sgautherie) 2008-02-19 05:21:04 PST
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

Karsten:
Not yet, I'm kind swamped at the moment. I hope to/may get to it before
FOSDEM, but I will definitely review it this month.

Me:
No hurry, yet this patch is blocking me for other (related) patch and TB is moving forward even further.

Ian, Neal:
In case you could r+sr this patch before Karsten...
Comment 4 Ian Neal 2008-02-19 16:20:40 PST
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

>Index: mailWindowOverlay.xul
>===================================================================
>diff -u -r1.349 mailWindowOverlay.xul
Please try and give 8 lines of context (-u8)

>+                <menuitem uri="..." class="folderMenuItem menuitem-iconic"
Nit: line up subsequent lines, after menuitem, with uri=, I know it is completely inconsistent throughout this file but for any new ones going in see if we can start getting some consistency.

Seems to do everything it says on the tin. r=me
Comment 5 Karsten Düsterloh 2008-02-29 07:19:35 PST
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

The patch as looks okay (modulo IanN's nits) and does work as in TB - but TB will change "soon" by courtesy of bug 413781, so we should wait for that.
Especially, since the current notion of "recent folder" on Linux doesn't adhere to any human-understandable algorithm. ;-)
Comment 6 Karsten Düsterloh 2008-02-29 07:20:20 PST
Marking as blocked by bug 413781.
Comment 7 Serge Gautherie (:sgautherie) 2008-02-29 07:59:32 PST
What I was now looking for was a sr actually, not a second r,
but that's good to get anyway.

My plan was *not* to wait (= not go straight) for bug 413781:
while that bug is a very good target by itself,
I would very much prefer to do the synchronization step by step:
here, then port bug 350543, then fix bug 68174, ...

It seems easier to track regressions (if any), (and for me to find out which TB bugs to sync from !)
moreover, doing it this way in bug 68174 already allowed me to find/fix/enhance "underlying" current code behaviors.
Comment 8 Karsten Düsterloh 2008-02-29 08:13:33 PST
That's why I still set the sr - if Neil agrees, we can still go on with this here, especially since 413781 still seems to need some work. 
(I am not an sr, btw.)
Comment 9 Serge Gautherie (:sgautherie) 2008-02-29 08:38:08 PST
(In reply to comment #8)
> That's why I still set the sr - if Neil agrees, we can still go on with this
> here, especially since 413781 still seems to need some work. 

Fine with me :-)

> (I am not an sr, btw.)

Ah !
I was in doubt because you are module owner on <http://www.seamonkey-project.org/dev/project-areas>,
but <http://www.seamonkey-project.org/dev/review-and-flags> says only Neil and Peter are super-reviewers.
I'll try and remember that.
Comment 10 Serge Gautherie (:sgautherie) 2008-03-16 18:19:41 PDT
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

Neil, ping ?
Comment 11 neil@parkwaycc.co.uk 2008-03-24 13:58:17 PDT
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

>-      <menupopup id="menu_MovePopup"/>
>-        <template>
>+        <menupopup id="menu_MovePopup">
...
>+        </menupopup>
>+      <template>
...
>+      <menupopup id="menu_copyPopup">
...
>       <menupopup/>
>       <template>
...
>+      </template>
>+    </menu>
>+    <menuseparator id="copyMsgRecentSeparator"/>
>+    </menupopup>
I know the indentation around here is already messed up, but you're making things even worse. The <template> that you outdented did in fact correctly have 8 spaces of indent. The new menupopups that you're adding should also have 8 spaces of indent. The menu and menuseparator should thus have 10 spaces of indent, etc. (Don't bother changing the indentation of any other old lines even though they're wrong.)

> <!ENTITY moveMsgMenu.label "Move">
> <!ENTITY moveMsgMenu.accesskey "M">
>+<!ENTITY moveCopyMsgRecentMenu.label "Recent">
>+<!ENTITY moveCopyMsgRecentMenu.accesskey "R">
> <!ENTITY copyMessageLocation.label "Copy Message Location">
> <!ENTITY copyMessageLocation.accesskey "M">
> <!ENTITY copyMsgMenu.label "Copy">
> <!ENTITY copyMsgMenu.accesskey "C">
Well, I don't know what copyMessageLocation is doing there, but I'd prefer Recent to be after Copy.

>+<!ENTITY contextMoveCopyMsgRecentMenu.label "Recent">
>+<!ENTITY contextMoveCopyMsgRecentMenu.accesskey "R">
And this is totally in the wrong place, it belongs after contextCopy.

sr=me with these fixed.
Comment 12 Serge Gautherie (:sgautherie) 2008-03-27 18:00:13 PDT
Created attachment 312171 [details] [diff] [review]
(Bv1-SM) <messenger.dtd> reordering

(In reply to comment #11)

Before doing what you asked, here's a (SM) patch that moves some lines around: nothing more. (I prefer to do it separately.)

> (From update of attachment 302415 [details] [diff] [review])
> Well, I don't know what copyMessageLocation is doing there, but I'd prefer
> Recent to be after Copy.

While the cvs diff moves |moveMsgMenu.*| around,
the point is actually to move |copyMessageLocation.*| before it, as in the XUL file.

> And this is totally in the wrong place, it belongs after contextCopy.

Oh, I see ... Actually, I suggest to synchronize/move the whole |<!-- Thread Pane Context Menu -->| block after Thunderbird file.
Comment 13 neil@parkwaycc.co.uk 2008-03-28 04:21:38 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 302415 [details] [diff] [review])
> > Well, I don't know what copyMessageLocation is doing there, but I'd prefer
> > Recent to be after Copy.
> the point is actually to move |copyMessageLocation.*| before it, as in the XUL
> file.
Actually in the XUL file it's in the context menus, but this is the main block.
Comment 14 Serge Gautherie (:sgautherie) 2008-03-29 07:23:10 PDT
Created attachment 312469 [details] [diff] [review]
(Bv1a-SM) <messenger.dtd> reordering

Bv1-SM, with comment 13 suggestion.

(In reply to comment #13)
> Actually in the XUL file it's in the context menus, but this is the main block.

Oh, right ... I'll move and rename it to |contextCopyMessageLocation.*| (in SM and TB) in a followup patch(es).
Comment 15 neil@parkwaycc.co.uk 2008-03-31 08:16:01 PDT
Comment on attachment 312469 [details] [diff] [review]
(Bv1a-SM) <messenger.dtd> reordering

So, I don't see what this achieves. I had a quick look at messenger.dtd and the only entities that looked out of place were for the Tools menu.
Comment 16 Serge Gautherie (:sgautherie) 2008-03-31 09:37:31 PDT
Created attachment 312744 [details] [diff] [review]
(Bv1b-SM) <messenger.dtd> reordering + 1 removal

(In reply to comment #15)
> (From update of attachment 312469 [details] [diff] [review])
> So, I don't see what this achieves. I had a quick look at messenger.dtd and the
> only entities that looked out of place were for the Tools menu.

*Moves the "Thread Pane Context Menu" around:
 only to synchronize (this part) with the TB file.
 (So Av1 patch can be in sync' too.)
*Gather the "Tools Menu" items together, per comment 15 suggestion,
 where the TB one are.
*Removes unused |junkMailCmd.*|.
 (See <http://mxr.mozilla.org/seamonkey/search?string=junkMailCmd&case=on&tree=seamonkey>)
Comment 17 neil@parkwaycc.co.uk 2008-04-02 08:49:35 PDT
Comment on attachment 312744 [details] [diff] [review]
(Bv1b-SM) <messenger.dtd> reordering + 1 removal

This still includes the spurious context menu entity move? Also, the tools menu probably belongs before the folder pane.
Comment 18 Serge Gautherie (:sgautherie) 2008-04-02 12:35:31 PDT
Created attachment 313145 [details] [diff] [review]
(Bv1c-SM) <messenger.dtd> reordering + 1 removal
[Checkin: Comment 19]

Bv1b-SM, with comment 17 suggestion(s).
Comment 19 Reed Loden [:reed] (use needinfo?) 2008-04-04 02:11:02 PDT
Checking in suite/locales/en-US/chrome/mailnews/messenger.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/messenger.dtd,v  <--  messenger.dtd
new revision: 1.224; previous revision: 1.223
done
Comment 20 Serge Gautherie (:sgautherie) 2008-04-04 11:36:53 PDT
Created attachment 313665 [details] [diff] [review]
(Av1a) <mailWindowOverlay.xul> ++
[Checkin: Comment 21]

Av1, with comment 4 and comment 11 suggestion(s).

Keeping r+/sr+.
Comment 21 Reed Loden [:reed] (use needinfo?) 2008-04-08 11:35:27 PDT
Checking in mailnews/base/resources/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.351; previous revision: 1.350
done
Checking in suite/locales/en-US/chrome/mailnews/messenger.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/messenger.dtd,v  <--  messenger.dtd
new revision: 1.225; previous revision: 1.224
done
Comment 22 Serge Gautherie (:sgautherie) 2008-04-09 04:30:39 PDT
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed
Comment 23 Serge Gautherie (:sgautherie) 2008-04-09 05:03:49 PDT
Created attachment 314566 [details] [diff] [review]
(Cv1-TB) Removes |junkMailCmd| remnants
[Checkin: See comment 26]

See comment 16.
Comment 24 Serge Gautherie (:sgautherie) 2008-04-09 05:14:12 PDT
(In reply to comment #14)
> Oh, right ... I'll move and rename it to |contextCopyMessageLocation.*| (in SM
> and TB) in a followup patch(es).

I filed:
Bug 427967 – Rename |copyMessageLocation.*| to |contextCopyMessageLocation.*|
Comment 25 Serge Gautherie (:sgautherie) 2008-04-18 17:36:48 PDT
Phil, ping ?
Comment 26 Phil Ringnalda (:philor) 2008-04-18 23:09:37 PDT
Comment on attachment 314566 [details] [diff] [review]
(Cv1-TB) Removes |junkMailCmd| remnants
[Checkin: See comment 26]

r=philringnalda

mail/base/content/hiddenWindow.js 1.9
mail/locales/en-US/chrome/messenger/messenger.dtd 1.74

If you're in a hurry for review, you'll find that attaching the patch to an open Thunderbird bug that actually says what it's about will get quicker results than attaching it to the closed SeaMonkey bug about something else where you happened to notice and coincidentally fix the corresponding SeaMonkey bug. Bug numbers are cheap, we can afford to use lots of them.

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