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)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.0a1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•17 years ago
|
||
[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)
Assignee | ||
Updated•17 years ago
|
Attachment #302415 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 2•17 years ago
|
||
Any comment/review on my patch ?
Assignee | ||
Comment 3•17 years ago
|
||
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 5•17 years ago
|
||
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+
Assignee | ||
Comment 7•17 years ago
|
||
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•17 years ago
|
||
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.)
Assignee | ||
Comment 9•17 years ago
|
||
(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.
Assignee | ||
Comment 10•17 years ago
|
||
Comment on attachment 302415 [details] [diff] [review]
(Av1) <mailWindowOverlay.xul> ++
Neil, ping ?
Comment 11•17 years ago
|
||
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+
Assignee | ||
Comment 12•17 years ago
|
||
(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)
Comment 13•17 years ago
|
||
(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.
Assignee | ||
Comment 14•17 years ago
|
||
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 15•17 years ago
|
||
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)
Assignee | ||
Comment 16•17 years ago
|
||
(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 17•17 years ago
|
||
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)
Assignee | ||
Comment 18•17 years ago
|
||
Bv1b-SM, with comment 17 suggestion(s).
Attachment #312744 -
Attachment is obsolete: true
Attachment #313145 -
Flags: review?(neil)
Updated•17 years ago
|
Attachment #313145 -
Flags: review?(neil) → review+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: Bv1c-SM // Leave opened]
Comment 19•17 years ago
|
||
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]
Assignee | ||
Updated•17 years ago
|
Attachment #313145 -
Attachment description: (Bv1c-SM) <messenger.dtd> reordering + 1 removal → (Bv1c-SM) <messenger.dtd> reordering + 1 removal
[Checkin: Comment 19]
Assignee | ||
Comment 20•17 years ago
|
||
Av1, with comment 4 and comment 11 suggestion(s).
Keeping r+/sr+.
Attachment #302415 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Comment 21•17 years ago
|
||
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]
Assignee | ||
Updated•17 years ago
|
Attachment #313665 -
Attachment description: (Av1a) <mailWindowOverlay.xul> ++ → (Av1a) <mailWindowOverlay.xul> ++
[Checkin: Comment 21]
Assignee | ||
Comment 22•17 years ago
|
||
[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
Assignee | ||
Comment 23•17 years ago
|
||
See comment 16.
Attachment #314566 -
Flags: review?(philringnalda)
Assignee | ||
Comment 24•17 years ago
|
||
(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.*|
Assignee | ||
Comment 25•17 years ago
|
||
Phil, ping ?
Comment 26•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
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.
Description
•