Need to implement Mark|by Date

RESOLVED FIXED in Future

Status

MailNews Core
Backend
P3
normal
RESOLVED FIXED
18 years ago
9 years ago

People

(Reporter: fenella, Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

19.82 KB, image/jpeg
Details
5.55 KB, image/gif
Details
9.25 KB, image/jpeg
Details
40.18 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
39.56 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
[FEATURE]Need to implement Mark Message Read by Date
(Reporter)

Updated

18 years ago
QA Contact: lchiang → fenella

Updated

18 years ago
Blocks: 10791

Updated

18 years ago
Assignee: phil → putterman

Updated

18 years ago
Target Milestone: M15

Comment 1

18 years ago
M15

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: M15 → M20

Comment 2

18 years ago
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.
Keywords: nsbeta3
Target Milestone: M20 → M18

Comment 3

18 years ago
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. 

Comment 4

18 years ago
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.
Keywords: nsbeta3
Target Milestone: M18 → Future

Comment 5

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

Comment 6

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

Comment 7

17 years ago
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.

Comment 8

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

Comment 9

17 years ago
reassigning to sspitzer
Assignee: putterman → sspitzer
Status: ASSIGNED → NEW
Keywords: 4xp

Comment 10

17 years ago
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).
Summary: [FEATURE]Need to implement Mark|by Date → Need to implement Mark|by Date

Updated

17 years ago
Keywords: mozilla1.0

Updated

17 years ago
QA Contact: fenella → laurel

Comment 11

16 years ago
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?

Comment 12

15 years ago
Created attachment 100278 [details]
prototype

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

Comment 13

15 years ago
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.

Comment 14

15 years ago
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 ....

Comment 15

15 years ago
Created attachment 113836 [details]
suggested look

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

Comment 16

15 years ago
Created attachment 113837 [details]
suggested menu entry

Comment 17

15 years ago
Created attachment 113838 [details] [diff] [review]
updated patch

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 :)
Attachment #100278 - Attachment is obsolete: true

Comment 18

15 years ago
Created attachment 115176 [details] [diff] [review]
keep patch up-to-date

updated patch against 1.3b, minor improvements (supporting KEY_ESCAPE, initial
TO date)
Attachment #113838 - Attachment is obsolete: true

Comment 19

15 years ago
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 :)
Attachment #115176 - Flags: review?(mscott)

Comment 20

15 years ago
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...

Comment 21

15 years ago
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)

Comment 22

15 years ago
> 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 :)

Comment 23

15 years ago
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.?

Comment 24

15 years ago
Created attachment 118592 [details]
4.x

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 25

15 years ago
Comment on attachment 115176 [details] [diff] [review]
keep patch up-to-date

removing review request for the moment
Attachment #115176 - Flags: review?(mscott)

Comment 26

15 years ago
Created attachment 120613 [details] [diff] [review]
update patch to jglicks comments and 1.4a branch

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.
Attachment #115176 - Attachment is obsolete: true

Comment 27

15 years ago
Created attachment 120614 [details]
revised screen shot
Attachment #113836 - Attachment is obsolete: true

Comment 28

15 years ago
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? :)
Attachment #120613 - Flags: review?(neil)

Comment 29

15 years ago
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)
Attachment #120613 - Flags: review?(neil) → review-

Comment 30

15 years ago
Created attachment 120760 [details] [diff] [review]
updated patch

Nick, thanks for your useful comments! attached is a new version of the patch
complying to your requests, and updated to HEAD.
Attachment #120613 - Attachment is obsolete: true

Comment 31

15 years ago
Comment on attachment 120760 [details] [diff] [review]
updated patch

next try :)
Attachment #120760 - Flags: review?(neil)

Comment 32

15 years ago
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!
Attachment #120760 - Flags: review?(neil) → review-

Comment 33

15 years ago
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 ... :)

Comment 34

15 years ago
Created attachment 124165 [details] [diff] [review]
patch v1.2

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
Attachment #120760 - Attachment is obsolete: true

Comment 35

15 years ago
Created attachment 128591 [details] [diff] [review]
patch updated to head
Attachment #124165 - Attachment is obsolete: true

Comment 36

15 years ago
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.
Attachment #128591 - Flags: review?(neil.parkwaycc.co.uk)

Comment 37

15 years ago
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.
Attachment #128591 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 38

15 years ago
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 39

15 years ago
Comment on attachment 128591 [details] [diff] [review]
patch updated to head

obsoleting, since it was diffed with the completely wrong reference source ...
Attachment #128591 - Attachment is obsolete: true

Comment 40

15 years ago
Created attachment 128645 [details] [diff] [review]
patch v1.3

Updated

15 years ago
Attachment #128645 - Flags: review?(neil.parkwaycc.co.uk)

Comment 41

15 years ago
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 42

15 years ago
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!
Attachment #128645 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 43

15 years ago
Created attachment 128741 [details] [diff] [review]
patch v1.4

new version - addressed all of the issues raised by review
Attachment #128645 - Attachment is obsolete: true

Updated

15 years ago
Attachment #128741 - Flags: review?(neil.parkwaycc.co.uk)

Comment 44

15 years ago
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...
Attachment #128741 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 45

15 years ago
Created attachment 128813 [details] [diff] [review]
patch v1.5

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 ... :)
Attachment #128741 - Attachment is obsolete: true

Comment 46

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

Updated

15 years ago
Attachment #128813 - Flags: review?(neil.parkwaycc.co.uk)

Comment 47

15 years ago
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.
Attachment #128813 - Flags: superreview?(bienvenu)
Attachment #128813 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #128813 - Flags: review+

Comment 48

15 years ago
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.

Comment 49

15 years ago
> 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?

Comment 50

15 years ago
Created attachment 128876 [details] [diff] [review]
patch v1.5.1 - using nsIMsgFolder::MarkMessagesRead

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

Comment 51

15 years ago
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.
Attachment #128876 - Flags: review?(neil.parkwaycc.co.uk)

Comment 52

15 years ago
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?
Attachment #128876 - Flags: superreview?(bienvenu)
Attachment #128876 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #128876 - Flags: review+

Comment 53

15 years ago
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;
Attachment #128876 - Flags: superreview?(bienvenu) → superreview+

Updated

15 years ago
Attachment #128813 - Flags: superreview?(bienvenu)

Comment 54

15 years ago
> 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).

Comment 55

15 years ago
Fix checked in with some JS ?: cleanup; if it's wrong I'll fix it tomorrow.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 56

15 years ago
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
Blocks: 245063
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.