Closed Bug 416669 Opened 17 years ago Closed 17 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0a1

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Blocks: TB2SM
Attached patch (Av1) <mailWindowOverlay.xul> ++ (obsolete) (deleted) — Splinter Review
[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=|...
Assignee: mail → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #302415 - Flags: superreview?(mnyromyr)
Attachment #302415 - Flags: review?(mnyromyr)
Attachment #302415 - Flags: review?(iann_bugzilla)
Any comment/review on my patch ?
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...
Attachment #302415 - Flags: review?(neil)
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
Attachment #302415 - Flags: review?(iann_bugzilla) → review+
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. ;-)
Attachment #302415 - Flags: superreview?(neil)
Attachment #302415 - Flags: superreview?(mnyromyr)
Attachment #302415 - Flags: review?(neil)
Attachment #302415 - Flags: review?(mnyromyr)
Attachment #302415 - Flags: review+
Marking as blocked by bug 413781.
Depends on: 413781
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.
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.)
(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 on attachment 302415 [details] [diff] [review] (Av1) <mailWindowOverlay.xul> ++ Neil, ping ?
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.
Attachment #302415 - Flags: superreview?(neil) → superreview+
Attached patch (Bv1-SM) <messenger.dtd> reordering (obsolete) (deleted) — Splinter Review
(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.
Attachment #312171 - Flags: superreview?(neil)
Attachment #312171 - Flags: review?(neil)
(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.
Attached patch (Bv1a-SM) <messenger.dtd> reordering (obsolete) (deleted) — Splinter Review
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).
Attachment #312171 - Attachment is obsolete: true
Attachment #312469 - Flags: superreview?(neil)
Attachment #312469 - Flags: review?(neil)
Attachment #312171 - Flags: superreview?(neil)
Attachment #312171 - Flags: review?(neil)
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.
Attachment #312469 - Flags: superreview?(neil)
Attachment #312469 - Flags: review?(neil)
(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>)
Attachment #312469 - Attachment is obsolete: true
Attachment #312744 - Flags: superreview?(neil)
Attachment #312744 - Flags: review?(neil)
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.
Attachment #312744 - Flags: superreview?(neil)
Attachment #312744 - Flags: review?(neil)
Bv1b-SM, with comment 17 suggestion(s).
Attachment #312744 - Attachment is obsolete: true
Attachment #313145 - Flags: review?(neil)
Attachment #313145 - Flags: review?(neil) → review+
Keywords: checkin-needed
Whiteboard: [c-n: Bv1c-SM // Leave opened]
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
Keywords: checkin-needed
Whiteboard: [c-n: Bv1c-SM // Leave opened]
Attachment #313145 - Attachment description: (Bv1c-SM) <messenger.dtd> reordering + 1 removal → (Bv1c-SM) <messenger.dtd> reordering + 1 removal [Checkin: Comment 19]
Av1, with comment 4 and comment 11 suggestion(s). Keeping r+/sr+.
Attachment #302415 - Attachment is obsolete: true
No longer depends on: 413781
Keywords: checkin-needed
Whiteboard: [c-n: Av1a]
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: Av1a]
Attachment #313665 - Attachment description: (Av1a) <mailWindowOverlay.xul> ++ → (Av1a) <mailWindowOverlay.xul> ++ [Checkin: Comment 21]
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) V.Fixed
Status: RESOLVED → VERIFIED
No longer blocks: TB2SM
(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.*|
Phil, ping ?
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.
Attachment #314566 - Attachment description: (Cv1-TB) Removes |junkMailCmd| remnants → (Cv1-TB) Removes |junkMailCmd| remnants [checked in]
Attachment #314566 - Flags: review?(philringnalda) → review+
Attachment #314566 - Attachment description: (Cv1-TB) Removes |junkMailCmd| remnants [checked in] → (Cv1-TB) Removes |junkMailCmd| remnants [Checkin: See comment 26]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: