The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in seamonkey2.0a1

Status

SeaMonkey
MailNews: Message Display
--
enhancement
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.0a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
Per bug 401365 comment 21.

Updated

9 years ago
Blocks: 360488
(Assignee)

Comment 1

9 years ago
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=|...
Assignee: mail → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #302415 - Flags: superreview?(mnyromyr)
Attachment #302415 - Flags: review?(mnyromyr)
(Assignee)

Updated

9 years ago
Attachment #302415 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 2

9 years ago
Any comment/review on my patch ?
(Assignee)

Comment 3

9 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 4

9 years ago
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

9 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+

Comment 6

9 years ago
Marking as blocked by bug 413781.
Depends on: 413781
(Assignee)

Comment 7

9 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

9 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

9 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

9 years ago
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+
(Assignee)

Comment 12

9 years ago
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.
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.
(Assignee)

Comment 14

9 years ago
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).
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)
(Assignee)

Comment 16

9 years ago
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>)
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)
(Assignee)

Comment 18

9 years ago
Created attachment 313145 [details] [diff] [review]
(Bv1c-SM) <messenger.dtd> reordering + 1 removal
[Checkin: Comment 19]

Bv1b-SM, with comment 17 suggestion(s).
Attachment #312744 - Attachment is obsolete: true
Attachment #313145 - Flags: review?(neil)

Updated

9 years ago
Attachment #313145 - Flags: review?(neil) → review+
(Assignee)

Updated

9 years ago
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]
(Assignee)

Updated

9 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

9 years ago
Created attachment 313665 [details] [diff] [review]
(Av1a) <mailWindowOverlay.xul> ++
[Checkin: Comment 21]

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

Keeping r+/sr+.
Attachment #302415 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
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
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: Av1a]
(Assignee)

Updated

9 years ago
Attachment #313665 - Attachment description: (Av1a) <mailWindowOverlay.xul> ++ → (Av1a) <mailWindowOverlay.xul> ++ [Checkin: Comment 21]
(Assignee)

Comment 22

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

Updated

9 years ago
No longer blocks: 360488
(Assignee)

Comment 23

9 years ago
Created attachment 314566 [details] [diff] [review]
(Cv1-TB) Removes |junkMailCmd| remnants
[Checkin: See comment 26]

See comment 16.
Attachment #314566 - Flags: review?(philringnalda)
(Assignee)

Comment 24

9 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

9 years ago
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+
(Assignee)

Updated

9 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.