[FEATURE]Need to implement Mark Message Read by Date

M15

This is m20. If we are not going to implement (which I assume we aren't), we need to remove the menu item. I'll nominate to nsbeta3 for removing the menu item. Correcting target milestone to be in line with our triaging guidelines.

actually, the proper way for me to do this would have been to open a new bug to remove the menu item and keep this bug helpwanted.

http://bugzilla.mozilla.org/show_bug.cgi?id=41866 filed to remove the menu item. When this feature gets implemented in the future, we'll need to put the menu item back.

*** Bug 72215 has been marked as a duplicate of this bug. ***

I believe that bug #64091 is a duplicate of this bug.

Gath Wallace and I gave the reasons why this bug should be a dup of bug 64091 on that bug's page. In short this one has wrong decription (feature, not 4xp), wrong component (I believe) and therefore is assigned not to the person working on this field. If I'm wrong, let someone correct me.

*** Bug 64091 has been marked as a duplicate of this bug. ***

reassigning to sspitzer

Please remove "FEATURE" from the summary. According to http://www.mozilla.org/quality/bugzilla-code-definitions.html "FEATURE" would imply this is not a bug, but a RFE, requiring "Enhancement" severity level, while this guy *is* a bug as it is a Product Parity (4xp).

For somebody who has never really seen Netscape 4.x: How would such a feature look like (in the user interface)? IOW: What is the specification for this RFE? :) And those who know: On a scale from 0 to 9 (where 0 is tampering a little bit with a JS file, and 9 is rewriting the complete mail/news-UI :), how difficult would it be to implement this?

The attachement implements a "mark read by date" functionality in pure XUL/JS. It's more a proof of concept than a patch, because * The UI needs some work. It was just quick shot, and besides the design to be discussed, there are some quirks like ESC/ENTER not working in the dialog currently. * The performance is bad up to horrible in large folders - I sue JS for this at the moment :). I would like to use the nsIMsgDatabase::MarkReadByDate function (actually, I took the implementation and re-wrote it in JS), but this method is not scriptable because of a native parameter being used. But at least it works. Have to ask some questions about interfaces (like "how frozen is nsIMsgDatabase?" :) in the proper newsgroups to see how to proceed to reach the C++ implementation (new method without the native parameter, remove the parameter, new interface ...). But at least it works :)

Mmmh, a "Mark by date" patch has been provided in this bug for roughly 4 months now. Maybe sombody could have a look at it and give some feedback, so that it can be incorporated and improved. I'd rather accept a slow implementation of this first, than having none at all.

Sebastian, I suppose would be my resposibility to care for this (and seek for comments), sorry for not doing so. I'll try to do so (unfortunately next week, at the earliest). I plan to attach a screen shot of the prototype, so the UI experts can flame me, and to provide a slightly improved patch ....

suggestion how this could look like. Added a "from" (not only "up to"), not sure if this is really needed.

update the patch to some more recent code (1.3a, actually) additionally, the new version supports localized date input - took this code from the search-for-messages functionality (searchTermOverlay.js), where it had been introduced in the meantime. For this, I moved the respective code to a shared place. The two screenshots (attachement 113836 and attachement 113837) is what this patch implements. Would be interested in any feedback :)

updated patch against 1.3b, minor improvements (supporting KEY_ESCAPE, initial TO date)

Comment on attachment 115176 [details] [diff] [review] keep patch up-to-date really no idea who I'd have to ask for reviewing this ... mscott, if I was wrong with picking you (: sorry, and would you please gimme a hint? thanks :)

I can't tell codewise, but I applied the patch and built and everything seems to work just fine. Although the date is always shown in mm/dd/yy format (that's not my local preference), maybe someone could provide some feedback there. Other than that I love the functionality of this...

Frank, I've just seen that you are rolling your own time/date parser. Have a look at PR_ParseTimeString etc..., for the existing functions (Defined as a function in: nsprpub/pr/src/misc/prtime.c, line 956 Defined as a function prototype in: nsprpub/pr/include/prtime.h, line 264) (I'm not familiar with this code, just pointing to it after some IRCing with others)

> Although the date is always shown in mm/dd/yy format (that's not my local > preference) Hmm. Do you have a preference called "mailnews.search_date_format" set tosomething other than 0? When you search for messages in mailnews, what is the date format there (preferably to be checked in a version without the patch, as with the patch, both the search dialog and the mark-read-by-date dialog share the date formatting code)? > Frank, I've just seen that you are rolling your own time/date parser. Well, I've, ehm, borrowed the code from the search message dialog. Since it came in only recently (between 1.2 and 1.3a, if I'm correct), and before this, the dialog always bothered with the non-localized format, I suppose there is no other _easy_ way. > Have a look at PR_ParseTimeString etc..., for the existing functions > (Defined as a function in: nsprpub/pr/src/misc/prtime.c, line 956 Actually, this patch is JavaScript only, so we cannot use the C-functions directly. Admittedly, I am not sure if there is a wrapper or component which would make them accessible to JS. But with the reasoning above, I doubt that there is, because else nobody would have taken the time to newly implement the date localization in JS for the search dialog. > ... after some IRCing with others Perhaps I really should start using this medium for such things, too :)

jglick, I'd be interested in your feedback from a user-experience point of view (if I was wrong in cc'ing you for this, please undo :) - do you mind having a look at the suggested screen shots/shortcuts etc.?

Mnemonic looks fine, "D" for Date. For access key, I believe "C" is available, but you'll want to verify that. (check out the accessbility pages on mozilla). As for the dialog, attached is a 4.x example. I'd recommend reducing the wording and removing the group box. The default date that 4.x uses is useful to users as an example of format also. Dialog Title: "Mark Messages as Read by Date" Text: "Mark messages as read:" Field Labels: "From:" "To:"

Comment on attachment 115176 [details] [diff] [review] keep patch up-to-date removing review request for the moment

jglick, thanks for your feedback (and sorry for taking so long to respond to it). I updated the dialog according to your suggestions (will attach a new screenshot), and also checked the keyboard shortcuts (they don't conflict). Defaulting to today's date is also present now. What I can't do similar to the 4.x way is the spin controls, respectively the edit field consisting of 3 sub edits: I did not find anything like this in Moz so far (so I can't steal it :), and I am not deep enough in XUL to create this from scratch. So I'd like to keep it with simple edit fields for the moment - at least this is consistent with the only other place I found where dates are entered - the search message dialog.

Comment on attachment 120613 [details] [diff] [review] update patch to jglicks comments and 1.4a branch Neil, you've been suggested to me for reviewing this. Could you please have a look at the patch, if you find a free minute? :)

Comment on attachment 120613 [details] [diff] [review] update patch to jglicks comments and 1.4a branch I assume you will update the patch for any recent bitrot. >--- mozilla.bak/mailnews/base/resources/content/mailWindowOverlay.js 2003-03-23 23:12:36.000000000 +0100 >+++ mozilla/mailnews/base/resources/content/mailWindowOverlay.js 2003-04-15 22:38:40.000000000 +0200 >@@ -1230,6 +1230,18 @@ > MarkSelectedMessagesFlagged(markFlagged); > } > >+function MsgMarkReadByDate( folder ) >+{ >+ if ( folder ) >+ { >+ var arg = { msgFolder: folder }; >+ >+ window.openDialog( 'markByDate.xul','markreadbydate', >+ 'modal,titlebar,chrome,resizable,status,centerscreen', >+ arg ); >+ } >+} >+ Should be window.openDialog("chrome://messenger/content/markByDate.xul", "", "chrome,modal,titlebar,centerscreen", GetLoadedMsgFolder()); I don't think the dialog needs to be resizable, or have a status bar. Also, GetLoadedMsgFolder() should always work here. >--- mozilla.bak/mailnews/base/resources/content/markByDate.js 1970-01-01 01:00:00.000000000 +0100 >+++ mozilla/mailnews/base/resources/content/markByDate.js 2003-04-15 22:38:42.000000000 +0200 >@@ -0,0 +1,137 @@ >+/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ But you correctly used 2 :-) >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1 This is a new file that you have written, so it gets MPL, also write yourself in as the initial developer and contributor. >+ var now = new Date( ); >+ var initialDate = new Date( now.getTime() - 24 * 60 * 60 * 1000 ); Should have a constant for this. Also, use Date.now() instead of new Date().getTime() >+ upperDateBox.value = convertDateToString( initialDate ); >+ upperDateBox.setSelectionRange( 0, upperDateBox.textLength ); If you need this, then use .select() >+ // extract the database >+ if ( window.arguments && window.arguments[0] ) >+ { >+ messageFolder = window.arguments[0].msgFolder; >+ if ( messageFolder ) >+ messageDatabase = messageFolder.getMsgDatabase( null ); >+ } You can do this later, if you need to. >+ // some handlers >+ doSetOKCancel( onOk, 0 ); [Don't need this - see later] >+function onOk() >+{ >+ // get the times as entered by the user >+ // the fallback for the lower bound, if not entered, is the beginning of the >+ // counting (1970-01-01), for the upper bound it's "today". >+ var prLower = getPRTime( document.getElementById("lowerDate"), new Date( 0 ) ); >+ var prUpper = getPRTime( document.getElementById("upperDate"), new Date( ) ); >+ >+ // for the upper date, we have to do a correction: >+ // if the user enters a date, then she means (because we told her it is meant this >+ // way :) that all messages sent at this day should be marked, too, but the PRTime >+ // calculated from this would point to the beginning of the day. So we need to >+ // increment it by (number of micro seconds per days - 1) >+ prUpper = prUpper + 24 * 60 * 60 * 1000 * 1000 - 1; This doesn't work for the default date, because it could have any amount of time. >+ if ( header ) >+ header = header.QueryInterface( Components.interfaces.nsIMsgDBHdr ); >+ >+ if ( header ) This doesn't work, what you need is if (header instanceof Components.interfaces.nsIMsgDBHdr) >+ if ( ( lower < messageDate ) && ( messageDate <= upper ) ) You need lower <= here! Also, messageDate < upper would be neater, then you could add a whole day to the upper date. >+ var commitType = Components.interfaces.nsMsgDBCommitType.kSmallCommit; >+ messageDatabase.Commit( commitType ); const or inline, please. >--- mozilla.bak/mailnews/base/resources/content/markByDate.xul 1970-01-01 01:00:00.000000000 +0100 >+++ mozilla/mailnews/base/resources/content/markByDate.xul 2003-04-15 22:38:42.000000000 +0200 >@@ -0,0 +1,64 @@ >+<?xml version="1.0"?> >+ >+<!-- >+ The contents of this file are subject to the Netscape Public >+ License Version 1.1 (the "License"); you may not use this file >+ except in compliance with the License. You may obtain a copy of >+ the License at http://www.mozilla.org/NPL/ MPL again. >+<!DOCTYPE dialog SYSTEM "chrome://messenger/locale/markByDate.dtd"> >+ >+<page You got the DOCTYPE right, but you're using the wrong type of root element, please learn how and use a <dialog>. >+ <keyset id="mailKeys"> >+ <key id="key_cancel" keycode="VK_ESCAPE" oncommand="window.close();"/> >+ <key id="key_return" keycode="VK_RETURN" oncommand="onOk();"/> >+ </keyset> Stuff that doesn't apply to a <dialog> >+ <vbox id="okCancelButtons" flex="0"/> Stuff that doesn't apply to a <dialog> (flex="0" is wrong anyway)

Nick, thanks for your useful comments! attached is a new version of the patch complying to your requests, and updated to HEAD.

Comment on attachment 120760 [details] [diff] [review] updated patch next try :)

Comment on attachment 120760 [details] [diff] [review] updated patch Well, I tried this out, and I opened a folder with 281 unread, and used this function to mark 277 read, and left 4 unread. But unfortunately when I left the folder and came back it went back to 281 unread. This is probably because I was using IMAP so it simply refreshed the unread count from the server, ignoring your local database changes. So you need to figure out how to push the changes to the server, whether you are online or offline (in which case they are saved somewhere...) Here are some other nits, and a possible bug: >+ switch (arrayOfStrings[0]) >+ { >+ case "1999": >+ if (arrayOfStrings[1] == "12" && >+ arrayOfStrings[2] == "31") >+ gSearchDateFormat = 1; >+ else if (arrayOfStrings[1] == "31" && >+ arrayOfStrings[2] == "12") >+ gSearchDateFormat = 2; >+ break; >+ case "12": >+ if (arrayOfStrings[1] == "31" && >+ arrayOfStrings[2] == "1999") >+ gSearchDateFormat = 3; >+ else if (arrayOfStrings[1] == "1999" && >+ arrayOfStrings[2] == "31") >+ gSearchDateFormat = 4; >+ break; >+ case "31": >+ if (arrayOfStrings[1] == "12" && >+ arrayOfStrings[2] == "1999") >+ gSearchDateFormat = 5; >+ else if (arrayOfStrings[1] == "1999" && >+ arrayOfStrings[2] == "12") >+ gSearchDateFormat = 6; >+ break; >+ } Something went wrong here, because my short date was "31/12/99" This code looks like it's overkill, would you mind rewriting it so that it only looks for the "12" and "31" and assumes that the other one is "99" or "1999"? >+ MsgMarkReadByDate( ); No space between ( and ) please (every time). >+ <command id="cmd_markReadByDate" oncommand="goDoCommand('cmd_markReadByDate'); event.preventBubble()" disabled="true"/> Don't bother fixing the existing code, but you can avoid the event.preventBubble()s by using command="cmd_markReadByDate" instead of observes="cmd_markReadByDate" below. >+var messageFolder; >+var messageDatabase; You don't use these here. >+const c_milliSecondsPerDay = 24 * 60 * 60 * 1000; >+const c_microSecondsPerDay = c_milliSecondsPerDay * 1000; I think the preferred name for consts is actually kMilliSecondsPerDay etc. >+function getPRTime( element, fallbackJDate ) I think a fallbackPRTime might be more useful. Actually, now that I look, you passed an int for today, bug! >+ if ( "" == element.value ) Should have the "" on the right. >+function onCancel() >+{ >+ return true; // allow closing >+} Don't need this, it's the default. >+ prUpper = prUpper + c_microSecondsPerDay; += >+ if ( header >+ && ( header instanceof Components.interfaces.nsIMsgDBHdr ) >+ ) Don't need header && because (null instanceof ...) is always false. >+ if ( ( lower <= messageDate ) && ( messageDate < upper ) ) >+ { >+ messageDatabase.MarkHdrRead( header, true, null ); >+ bFound = true; >+ } Thought: What if the header is already read? >+ ondialogaccept="return onOk();" Normally call this one onAccept() for some reason ;-) >+ ondialogcancel="return onCancel();"> Don't need this (it's the default) >+<!ENTITY messageMarkByDate.label "Mark Messages as Read by Date"> >+<!ENTITY markByDateDesc.label "Mark messages as read:"> >+<!ENTITY markByDateLower.label "From:"> >+<!ENTITY markByDateUpper.label "To:"> This can have an MPL too!

thanks again, Neil - will ask the technical issues raised by your comments by mail, to not spam people in case this is going to be some more questions ... :)

revised patch, complying to most Neil's request, and now also working for IMAP. The only missing thing is the rewriting of the code which I moved from searchTermOverlay.js into a new file. changes over the previous version: - calling commit on the database only if necessary (it really seems that the implementation does not detect if something needs to be committed) - in case of IMAP: propagate the change to the server - fixing the default-value bug - requested cosmetics

Comment on attachment 128591 [details] [diff] [review] patch updated to head Neil, I'd appreciate if you'd find some time to review this. I think the most interesting part (compared to older versions) is the small change at nsIMsgImapMailFolder::storeImapFlags, to allow calling it from JScript. A more detailed justification went out by mail :). Thanks.

Comment on attachment 128591 [details] [diff] [review] patch updated to head Looks like you diffed too much here, try again :-( I like call to store IMAP flags, especially fixing the broken .idl definition (what was mscott? thinking when he wrote that). The preferred name for arbitrary constants turns out to be MILLISECONDS_PER_DAY etc. kXXX are used for enumerations. Just out of interest, an alternative approach would have been to simulate a view unread messages dated before <whenever> and marked them all read, which would have the advantage of not freezing the program while it enumerates the unread messages, otherwise I feel this could get quite slow on large groups.

from comment 32 > I think the preferred name for consts is actually kMilliSecondsPerDay etc. from comment 37 > The preferred name for arbitrary constants turns out to be > MILLISECONDS_PER_DAY etc. kXXX are used for enumerations. Ehm - which one should I use now? :)

Comment on attachment 128591 [details] [diff] [review] patch updated to head obsoleting, since it was diffed with the completely wrong reference source ...

Comment on attachment 128645 [details] [diff] [review] patch v1.3 Note: not tested yet. >+ var time = new Date(); >+ time.setSeconds(0); >+ time.setMinutes(0); >+ time.setHours(0); >+ time.setYear(year); >+ time.setMonth(month); >+ time.setDate(date); There should of course be a time.setMilliseconds(0); here - can you fix this? >+ // Javascript time is in seconds, PRTime is in microseconds Also please fix this comment - JS time is ms! >+ // so multiply by 1000 when converting >+ return (time.getTime() * 1000); >+const MILLI_SECONDS_PER_DAY = 24 * 60 * 60 * 1000; >+const MICRO_SECONDS_PER_MILLI_SECOND = 1000; Don't use this, use a (correct) comment to explain why as above. >+const MICRO_SECONDS_PER_DAY = MILLI_SECONDS_PER_DAY * MICRO_SECONDS_PER_MILLI_SECOND; milliseconds and microseconds are single words in English, please remove the _ before SECONDS. >+ // and give it an initial date - "yesterday" >+ var initialDate = new Date( Date.now() - MILLI_SECONDS_PER_DAY ); >+ // note that this is sufficient - though it is in the midth of the previous day >+ // (actually: it's "now" - 24 hours), we convert it to a date string, and there >+ // the time vanishes This doesn't work between midnight and 1am on the day after summer time started... but if you set the hours to 0 and subtract 1 hour it should work. >+function getPRTimeFromJTime( jTime ) >+{ >+ return jTime.getTime() * MICRO_SECONDS_PER_MILLI_SECOND; >+} Inline this, it's only used once. >+function getPRTime( element, fallbackPRTime ) >+{ >+ if ( element.value == "" ) >+ return fallbackPRTime; >+ else >+ return convertStringToPRTime( element.value ); >+} To save you retrieving the .value twice, perhaps you could pass element.value as a string argument instead? Or perhaps just fix convertStringToPRTime to accept a fallback? >+ // the fallback for the lower bound, if not entered, is the beginning of the >+ // counting (1970-01-01) beginning of time works best for me ;-) >+ var beginningOfTime = getPRTimeFromJTime( new Date( 0 ) ); If you do your sums this works out as 0 :-) >+ var prLower = getPRTime( document.getElementById( "lowerDate" ), beginningOfTime ); >+ >+ // for the upper bound it's "today". >+ var dateThisMorning = getPRTimeFromJTime( >+ new Date( Math.floor( Date.now() / MILLI_SECONDS_PER_DAY ) * MILLI_SECONDS_PER_DAY ) >+ ); This doesn't work during summer time. You'll have to set the hours, minutes, seconds and milliseconds to zero instead. >+/** marks all headers in the database, which's time is between the two s/, which's/ whose/ >+ dump( "markByDate::markInDatabase: there /is/ no database to operate on!" ); Missing \n >+ var bIsIMAPFolder = false; >+ var messageKeys = new Array; >+ if ( messageFolder instanceof Components.interfaces.nsIMsgImapMailFolder ) >+ bIsIMAPFolder = true; Should be: var bIsIMAPFolder = messageFolder instanceof Components.interfaces.nsIMsgImapMailFolder; var messageKeys = []; >+ if ( header instanceof Components.interfaces.nsIMsgDBHdr ) >+ { >+ messageDate = header.date; >+ if ( ( lower <= messageDate ) && ( messageDate < upper ) ) >+ { >+ if ( !header.isRead ) // don't do anything until really necessary You can && this to the first if (with extra ()s). >+ const commitType = Components.interfaces.nsMsgDBCommitType.kSmallCommit; >+ messageDatabase.Commit( commitType ); I don't think it's worth it but if you want to use the const then you might as well name it kSmallCommit.

Comment on attachment 128645 [details] [diff] [review] patch v1.3 Testing revealed no further issues, but r- based on previous comments. And I would really like to see a better getLocaleShortDateFormat!

new version - addressed all of the issues raised by review

Comment on attachment 128741 [details] [diff] [review] patch v1.4 Almost there! >+ var possibleSeparators = [ "/", "-", "." ]; You can actually use a string for this, it works just like a character array: const possibleSeparators = "/-."; >+ var arrayOfStrings; >+ var i = 0; >+ for ( i=0; i<possibleSeparators.length; ++i ) Spaces around the = and < operators. Also put the var inside the for i.e. for ( var i = 0; >+ if ( i == possibleSeparators.length ) I think checking for arrayOfStrings.length == 3 (or != 3) would be a more "obvious" check. >+ } >+ else >+ { >+ if ( arrayOfStrings[1] == "31" ) Use else if i.e. } else if ( arrayOfStrings[1] == "31" ) { >+function getPRTime( stringValue, fallbackPRTime ) >+{ >+ if ( stringValue == "" ) >+ return fallbackPRTime; >+ else >+ return convertStringToPRTime( stringValue ); >+} You don't use this function any more, please remove it. >- void storeImapFlags(in long flags, in boolean addFlags, out nsMsgKey keysToFlag, in long numKeys); >+ void storeImapFlags(in long flags, in boolean addFlags, [array, size_is (numKeys)] in nsMsgKey keysToFlag, in PRUint32 numKeys); >-NS_IMETHODIMP nsImapMailFolder::StoreImapFlags(PRInt32 flags, PRBool addFlags, nsMsgKey *keys, PRInt32 numKeys) >+NS_IMETHODIMP nsImapMailFolder::StoreImapFlags(PRInt32 flags, PRBool addFlags, nsMsgKey *keys, PRUint32 numKeys) I've looked at this function and uses AllocateUidStringFromKeys which thinks that numKeys is a PRInt32; although you're not the only caller trying to pass a PRUint32 I suggest you leave it as a PRInt32 just in case, and the author of AllocateUidStringFromKeys can fix it when he fixes the bug that I think it's got...

addressed everything. but: > I suggest you leave it as a PRInt32 just in case, and the author of > AllocateUidStringFromKeys can fix it when he fixes the bug that I think it's > got... Well, the problem is that when leaving it signed, then the xpidl complains that the size_is argument has the wrong type, and should be unsigned long or RUint32. Okay, I looked where AllocateUidStringFromKeys is used, and it seems that it's called from - either ReplayOfflineMoveCopy, StoreImapFlags, StoreCustomKeywords, where a proper PRInt32 is passed, which originates from a method parameter - or from somewhere else, where a PRUint32 is passed into the PRUint32 slot. Looking at ReplayOfflineMoveCopy and StoreCustomKeywords, both suffer from the same problem as StoreImapFlags - they're not scriptable, because the message keys are not passed as IDL array. Correcting this, and changing their respective array-size parameter to unsigned long instead of long, allowed me to change the AllocateUidStringFromKeys parameter from PRInt32 to PRUint32, too. This way, we have PRUint32 in all places, and 3 more methods which are scriptable. So I decided to correct all these 3 methods while I was there ... (I probably overlooked something, again, because of this ... :)

> - or from somewhere else, where a PRUint32 is passed into the PRUint32 slot. ^ should be PRInt32, of course ---------------------------------------+

Comment on attachment 128813 [details] [diff] [review] patch v1.5 Looks good to me. Asking bienvenu for sr so that he can double-check the PRInt32/PRUint32 fix.

the PRUint32 stuff looks ok to me. However, you should consider using nsIMsgFolder::MarkMessagesRead(in nsISupportsArray messages, boolean markRead) that handles the imap case automatically, since imap folders override that method and mark the messages read on the server. Also, you get the advantages of batching.

> However, you should consider using nsIMsgFolder::MarkMessagesRead(in > nsISupportsArray messages, boolean markRead) I remember that some time ago, I read something about nsISupportsArray being deprecated, and kind of superseeded by ns(I)Array, which was newly created by Alec Flett to have a freezable array interface. Should I use nsISupportsArray, anyway?

using MarkMessagesRead now - seems a little bit faster for larger numbers of messages :)

Comment on attachment 128876 [details] [diff] [review] patch v1.5.1 - using nsIMsgFolder::MarkMessagesRead Neil, could you please have another look at this? Not much changed from the previous version (only markByDate.js) ... Not sure if I should exclude the nsIImapMailFolder changes now - speaking strictly, they're not necessary anymore. However, I think since we have them, and they do not hurt, we should include them. If you disagree, I'll remove them from the patch and submit a new issue, just to ensure that they're not lost.

Comment on attachment 128876 [details] [diff] [review] patch v1.5.1 - using nsIMsgFolder::MarkMessagesRead Just to be sure, the default date to catch up to is yesterday, but if you delete that then it will catch up to today, correct?

Comment on attachment 128876 [details] [diff] [review] patch v1.5.1 - using nsIMsgFolder::MarkMessagesRead sr=bienvenu, this will be nice. One nit - js has the ? operator so this kind of code could be more succinct: + if ( arrayOfStrings[1] == "12" ) // 31.12.1999 + gSearchDateFormat = 5; + else // 31.1999.12 + gSearchDateFormat = 6; e.g. gSearchDataFormat = (arrayOfStrings[1] == "12" ? 5 : 6;

> Just to be sure, the default date to catch up to is yesterday, but if you > delete that then it will catch up to today, correct? yep ... I think this isn't really bad: "yesterday" is only a suggestion for the upper limit (which is as reasonable is anything else), and "today" is a good handling (IMO) for a user which just doesn't want to be bothered with entering a date (it's probably pretty seldom to leave the upper limit empty, anyway).

Fix checked in with some JS ?: cleanup; if it's wrong I'll fix it tomorrow.

thanks - I was just compiling a fixed js-?-version on the very latest sources, but I also appreciate it this way :). Thanks a lot!

neil, I just tried this and it worked great. the UI looked a little weird though. I've logged a bug on that, and assigned to you. please see bug #245063