416669 - Port "Bug 350661 - Add Recent Folder Target [...]" to SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Serge Gautherie (:sgautherie)]

Per bug 401365 comment 21.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (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)]

Any comment/review on my patch ?

Comment 3

[Serge Gautherie (:sgautherie)]

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]

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]

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]

Marking as blocked by bug 413781.

Comment 7

[Serge Gautherie (:sgautherie)]

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]

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)]

(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)]

Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++

Neil, ping ?

Comment 11

[[e-mail]]

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)]

Attachment: (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

[[e-mail]]

(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)]

Attachment: (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

[[e-mail]]

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)]

Attachment: (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

[[e-mail]]

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)]

Attachment: (Bv1c-SM) <messenger.dtd> reordering + 1 removal [Checkin: Comment 19]

Bv1b-SM, with comment 17 suggestion(s).

Comment 19

[Reed Loden [:reed]]

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)]

Attachment: (Av1a) <mailWindowOverlay.xul> ++ [Checkin: Comment 21]

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

Keeping r+/sr+.

Comment 21

[Reed Loden [:reed]]

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)]

[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)]

Attachment: (Cv1-TB) Removes |junkMailCmd| remnants [Checkin: See comment 26]

See comment 16.

Comment 24

[Serge Gautherie (:sgautherie)]

(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)]

Phil, ping ?

Comment 26

[Phil Ringnalda (:philor)]

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.