Open Bug 778533 Opened 12 years ago Updated 2 years ago

Add reply management feature to Lightning for Thunderbird.

Categories

(Calendar :: General, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: hanlin.dev, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713224749
Attached patch work in progress (obsolete) — Splinter Review
Attachment #646962 - Flags: ui-review?(bwinton)
Attachment #646962 - Flags: review?(mconley)
Attachment #646962 - Attachment description: This patch contains a fairly complete set of features I planned for this summer. → work in progress
Hi folks, 

Han Lin is my student for this Summer of Code, and he's implemented a fairly complete reply reminder. After discussing that with Philipp and Jb, we agreed that it would be nice to land this as part of Calendar. Therefore, we're asking for a preliminary review from both of you so that we can get early feedback and polish it :).

- Han Lin, could you please list the current issues with the patch? (I'm thinking about the Gloda stuff).
- Could you please give a high-level overview of the patch? The big pieces, the overall logic, as well as the different areas that your patch touches.
- Could you also please give instructions as to how to build a Thunderbird with your patch included? I myself am unsure how to do.

Thanks,

jonathan
Status: UNCONFIRMED → NEW
Ever confirmed: true
The weekly status reports can be found at <http://code.google.com/p/reply-manager/wiki/WeeklyStatusReport?repo=wiki&path=/WeeklyStatusReport.wiki>; I think there are some videos too.
Known issues:

1. A message is marked expect-reply by set the "ExpectReply" string property to "true". I added a Gloda attribute to query for such messages. However, modification to the property on the message header will not be actively reflected to the Gloda attribute. The workaround is to toggle the "flagged" property on the message header twice to force the Gloda attribute to update.

2. The js date object returned by the Lightning datepicker in the message compose window ignores timezone offset while calling the toISOString method so that method returns a wrong local date. Other methods such as getDate and getMonth are fine so I use them to get a YYYY-MM-DD formated date string. This is not very elegant but it works.

3. Some style information is not actively applied when the lightning minimonth is displayed outside of the main message window. The strange thing is that after I inspect the CSS rules of that element from the DOM inspector, the correct look will appear. This one has no workaround yet.
Features:

1. The user can mark an email as expecting replies while one is being composed or from the message window. If enabled, the addon will create a calendar event with suitable title and link that event to the message in the following ways:
  a. if replies are received the addresses will be removed from the event title.
  b. if the message is deleted, the event will also be removed.
  c. if the reply deadline is modified, the date of the event will also be changed.
It creates a separate calendar so the user's own calendars are not messed up.

2. Additional information is added to the message header pane to make it clear that a message is expecting replies and how many people have replied.

Logic:

To check for replies the addon makes use of the Gloda to query for messages in the same conversation as the marked message so it can only find properly threaded messages at this moment. Then it uses a mail listener to watch for the msgAdded and msgsDeleted events to make appropriate changes to the calendar events when replies are received.

Adding the "ac_add_options --enable-calendar" to the .mozconfig will build a Thunderbird with the patched Lightning installed.
This patch contains all features I want to include.
Attachment #646962 - Attachment is obsolete: true
Attachment #646962 - Flags: ui-review?(bwinton)
Attachment #646962 - Flags: review?(mconley)
Attachment #647989 - Flags: ui-review?(bwinton)
Attachment #647989 - Flags: review?(mconley)
Attachment #647989 - Attachment description: Complete patch → Complete patch(add --enable-calendar to build with the patch added)
Hi,

Could anyone with commit access submit this patch to try with a custom mozconfig that calls --enable-calendar? I'm currently on holidays and I don't have access to my work machine so unfortunately there's not much I can do, on an EDGE connection to boot :).

Thanks,

jonathan
Unless I did something wrong,

https://build.mozilla.org/buildapi/self-serve/try-comm-central/rev/8c5f1e2bdfa591d306829f81bdf393b710d287b9

I didn't enable tests, since I believe only the Thunderbird tests will be run anyway and they are not relevant to these changes.
Try run for 8c5f1e2bdfa5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8c5f1e2bdfa5
Results (out of 18 total builds):
    success: 9
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@kewis.ch-8c5f1e2bdfa5
Hi Philipp,

It seems that I don't have access to the link you provided. Could you please describe what happened?

Thanks,

Han Lin
Improve some UI elements. The Gloda bug and minimonth bug are also fixed.
Attachment #647989 - Attachment is obsolete: true
Attachment #647989 - Flags: ui-review?(bwinton)
Attachment #647989 - Flags: review?(mconley)
Attachment #650614 - Flags: ui-review?(bwinton)
Attachment #650614 - Flags: review?(mconley)
The releng bot is just not aware of Thunderbird. Here is the link to the builds:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mozilla@kewis.ch-8c5f1e2bdfa5/

The self-serve link is irrelevant, but this will give you build info: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8c5f1e2bdfa5
From a quick view I think this could be a separate extension as well that just uses and extends the installed Lightning extension.

I applied the patch to Lightning 1.9 and did some short testing using Thunderbird 17.0a1 and SeaMonkey 2.14a1.

1) Selecting an email in Thunderbird after installation throw:
> Error: TypeError: aMsgHdr.getStringProperty is not a function
> Source file: resource://calendar/modules/replyManagerUtils.jsm 

2) The message pane shows 'This message has been replied to.' although I just send the mail myself and no response was made. 

3) The message pane shows 'The deadline for replies has passed.' although the deadline date is in the future. The date is not shown in correct format.

4) There are invisible buttons added to the message pane that doesn't contain text but only a tooltip like 'Click to read the replies' or 'click to see who have not replied'. Clicking them throws
> Error: Couldn't find a collection for msg: [xpconnect wrapped nsIMsgDBHdr]
> Source file: chrome://messenger/content/msgHdrViewOverlay.js Line: 2726

5) Disabling 'Replay Expected' for an email throw 
> Error: '[JavaScript Error: "aItem is null" {file: "file:///.../calStorageCalendar.js" line: 586}]' when calling method: [calICalendar::deleteItem] = NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS
> Source file: resource://calendar/modules/replyManagerCalendar.jsm Line: 126

Possible cause: initially the preference 'create event' was disabled and therefore no event was created. Later I enabled the preference and therefore you try to delete non existing event.

6) It is possible to rename the calendar "replyManagerCalendar" and therefore a new one will be created every time.

7) Fatal error is thrown if the calendar "replyManagerCalendar" is set to read-only.

8) Please ensure that your patch doesn't break Lightning if installed in SeaMonkey. 

Currently it will already throw an error during SeaMonkey startup.
> Error: TypeError: this.allRepliedBox is null
> Source File: chrome://lightning/content/replyManagerMsgHdrViewOverlay.js Line: 158

Or when openeing compose window:
> Error: TypeError: replymanagerhbox is null
> Source File: chrome://lightning/content/replyManagerComposeOverlay.js Line: 27

> Error: TypeError: gMsgCompose is null
> Source File: chrome://lightning/content/replyManagerComposeOverlay.js Line: 33

The new replay manager preference tab is not shown in SeaMonkey.

9) Should the extension check if Gloda is available and enabled?

That's all for now :)
Comment on attachment 650614 [details] [diff] [review]
Complete Patch (build with the "--enable-calendar" option)

I'm getting:

++DOMWINDOW == 29 (0x10308ee80) [serial = 52] [outer = 0x7f903e18a120]
JavaScript error: chrome://lightning/content/replyManagerMsgHdrViewOverlay.js, line 245: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]

when I try to view a sent message.


I also think it might be nice to actually send that info in the email header, so that if I'm using Thunderbird on the remote side, I'll be able to see when people expect a reply by…  :)


But I'm going to have to say ui-r- until I'm able to see the sent header pane, I'm afraid.

Thanks,
Blake.
Attachment #650614 - Flags: ui-review?(bwinton) → ui-review-
Hi Blake,

Thank you very much for the review. The comments are very helpful.

I think wrong elements are showing in the message header pane because the changes are not indexed by Gloda. I'm not sure about the reason but for a new account, Gloda does not index messages in the Sent folder until restart. Maybe I should add code to hide the elements if the message is not indexed. I'll also add check on Gloda.

As for Seamonkey, my initial goal is to add this feature to Thunderbird so can I just include the files in Thunderbird build of Lightning?

Thanks!

Han Lin
There is only one Lightning package that can be installed in both Thunderbird and SeaMonkey. There exists no different Lightning package for SeaMonkey.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #16)
> I also think it might be nice to actually send that info in the email
> header, so that if I'm using Thunderbird on the remote side, I'll be able to
> see when people expect a reply by…  :)
There are certain security implications here. I wouldn't want everyone to know by when I expect a reply, as it could put up false pressure if the timeframe is made to short only to serve as a reminder.


What we could do to get around the Seamonkey issues would be to add code that finds out if installed in Tb or Seamonkey, but unless its an unreasonable amount of work I think we should try to achieve Seamonkey compatibility.
So just for the record, Gloda may just be disabled, and you can never assume that a message has been indexed. However, the workaround is to use MsgHdrToMimeMessage. That function will stream a message header and execute the callback you provide with the MimeMessage representation, which includes all the headers. This is exactly what you're passed in the gloda plugin that you wrote. That way, you can have two code-paths: the fast one, that works when gloda has indexed the message, and the slow one, that kicks in when gloda failed to index the message. You should able to factor out the shared code.

However, since in your case it's a simple string property on a message header, you may just get away with just using the nsIMsgDBHdr, not the full representation :).
(In reply to Philipp Kewisch [:Fallen] from comment #19)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #16)
> > I also think it might be nice to actually send that info in the email
> > header, so that if I'm using Thunderbird on the remote side, I'll be able to
> > see when people expect a reply by…  :)
> There are certain security implications here. I wouldn't want everyone to
> know by when I expect a reply, as it could put up false pressure if the
> timeframe is made to short only to serve as a reminder.
> 
> 
> What we could do to get around the Seamonkey issues would be to add code
> that finds out if installed in Tb or Seamonkey, but unless its an
> unreasonable amount of work I think we should try to achieve Seamonkey
> compatibility.

Given that the pencil down date for Summer of Code is coming, there will not be enough time for Seamonkey compatibility. But after this I will definitely extend this feature to Seamonkey.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #16)
> Comment on attachment 650614 [details] [diff] [review]
> Complete Patch (build with the "--enable-calendar" option)
> 
> I'm getting:
> 
> ++DOMWINDOW == 29 (0x10308ee80) [serial = 52] [outer = 0x7f903e18a120]
> JavaScript error:
> chrome://lightning/content/replyManagerMsgHdrViewOverlay.js, line 245:
> NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
> 
> when I try to view a sent message.
> 
> 
> I also think it might be nice to actually send that info in the email
> header, so that if I'm using Thunderbird on the remote side, I'll be able to
> see when people expect a reply by…  :)
> 
> 
> But I'm going to have to say ui-r- until I'm able to see the sent header
> pane, I'm afraid.
> 
> Thanks,
> Blake.

Hi Blake,

Does this always happen? I haven't seen this before so I'm not sure what caused it. Do you have a set of steps to reproduce this error? At first glance, there is a call to Services.prefs.getBoolPref at line 245 which may be caused by the presence of the "calendar.replymanager.enabled" preference.

Thanks,

Han Lin
The get*Pref functions throw when the preference doesn't exist or is of a different type. You could use Lightning's cal.getPrefSafe() function, but its probably better to find out whats being passed, likely the pref doesn't exist.
Fixed a few things:
1. Add Gloda check. If Gloda is disabled or the message is not indexed the elements will be hidden.

2. The calendar is identified by its ID now instead of its name.

3. Add application restriction in Lightning's jar manifest so the overlays won't be applied if installed in Seamonkey.

4. The date is now shown in locale format.

5. Before any modification to the ReplyManagerCalendar, it will be made editable.

6. If we observe that our calendar is being deleted, a new one will be created.

7. Ensure consistency between calendar events and message header properties:
  a) If the event date is changed through the calendar UI, the "ExpectReplyDate" header property will also be modified to reflect this change.
  b) If the event is removed through the calendar UI, the associated message will be unmarked.
Attachment #650614 - Attachment is obsolete: true
Attachment #650614 - Flags: review?(mconley)
Attachment #651182 - Flags: ui-review?(bwinton)
Attachment #651182 - Flags: review?(mconley)
I just found out that observers must contain all member functions to avoid errors. So I added those methods.
Attachment #651182 - Attachment is obsolete: true
Attachment #651182 - Flags: ui-review?(bwinton)
Attachment #651182 - Flags: review?(mconley)
Attachment #651518 - Flags: ui-review?(bwinton)
Attachment #651518 - Flags: review?(mconley)
Add Fallen to the reviewers, which I should have done at the beginning.
Attachment #651518 - Attachment is obsolete: true
Attachment #651518 - Flags: ui-review?(bwinton)
Attachment #651518 - Flags: review?(mconley)
Attachment #651786 - Flags: ui-review?(bwinton)
Attachment #651786 - Flags: review?(philipp)
Attachment #651786 - Flags: review?(mconley)
Blocks: 782680
Comment on attachment 651786 [details] [diff] [review]
Complete Patch (build with the "--enable-calendar" option)

Review of attachment 651786 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Han-Lin,

Epic patch! :)

So I've got a bunch of notes for you. Style-wise, I think you'd benefit from reading the Javascript specific stuff in the Mozilla style guide: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

I'm certainly guilty of violating the style guide from time to time, but a refresher can't hurt.

Anyhow, see my notes below. If any of my comments seem general, like making sure to put a space after //, please apply them to the rest of your patch. There are some things I kept mentioning as I saw them, but there are things I mentioned only once despite repeated occurrences.

Also, I haven't tried building this patch yet, so my review says nothing about functionality - more about architecture and style.

Finally, I'm not a calendar coder, so hopefully Fallen can point out any calendar abusage. :)

Let me know if you have any questions,

-Mike

::: calendar/base/modules/replyManagerCalendar.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["replyManagerCalendar"];

I'm not sure how Fallen likes things in the calendar codebase, but in mail/, we prefer let over var (even at global scope), for more sensical scoping reasons.

So, I'd prefer "let" over any place you've used "var".

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["replyManagerCalendar"];
> +
> +const Cu = Components.utils;

We usually also alias Components.interfaces to Ci, that way you can use Ci.calICalendarManager down below.

@@ +15,5 @@
> +
> +/**
> + * replyManagerCalendar
> + * Handles interaction with Lightning, the calendar application for Thunderbird
> + * To use this module, it is required to have Lightning installed.

Since the module resides within calendar, which is strictly Lightning code, isn't this comment redundant? Can't we assume that if the caller is able to access replyManagerCalendar, that Lightning must be installed?

@@ +17,5 @@
> + * replyManagerCalendar
> + * Handles interaction with Lightning, the calendar application for Thunderbird
> + * To use this module, it is required to have Lightning installed.
> + */
> +var replyManagerCalendar = {

Exports like this are usually capitalized, so I'd make this ReplyManagerCalendar.

@@ +18,5 @@
> + * Handles interaction with Lightning, the calendar application for Thunderbird
> + * To use this module, it is required to have Lightning installed.
> + */
> +var replyManagerCalendar = {
> +  /* This method is called in replyManagerMailWindowOverlay.js

Block comments like this should be in the format

/** 
 * Some comments
 * another line.
 */

Applies to all below as well.

@@ +21,5 @@
> +var replyManagerCalendar = {
> +  /* This method is called in replyManagerMailWindowOverlay.js
> +   * after the window fires the load event. This ensures that 
> +   * replyManagerCalendar exists. */
> +  initCalendar : function ()

No space before : and no space after function

@@ +25,5 @@
> +  initCalendar : function ()
> +  {
> +    let calendarID = Services.prefs.getCharPref("calendar.replymanager.calendarID");
> +    /* Get the calendar with the calendarID. If the ID is an empty string,
> +     * the calendar does not exists. So create one. */

typo: "exists" -> "exist".

@@ +35,5 @@
> +    replyManagerCalendarManagerObserver.init();
> +    replyManagerCalendarObserver.init();
> +  },
> +
> +  retrieveItem: function(id,calendar)

space after ,

Function parameters should start with "a" according to the Mozilla style guide. So this should be aId, and aCalendar. Applies to all functions you define.

@@ +37,5 @@
> +  },
> +
> +  retrieveItem: function(id,calendar)
> +  {
> +    let listener = new replyManagerCalendar.calOpListener();    

You've got some trailing whitespace on this line, and a few more throughout your patch. Please remove it.

@@ +48,5 @@
> +   * @param date is the javascript date object
> +   * @param id is the messageId field of the message header
> +   * @param status is a string that will be the title of the event
> +   */
> +  addEvent : function(dateStr, id, status)

No space before :

@@ +51,5 @@
> +   */
> +  addEvent : function(dateStr, id, status)
> +  {
> +    this.calendar.readOnly = false;
> +    /* First we need to test of an event with the same

We generally use block comments for function / class headers, and use inline comments for the rest.

So these should be:

// First we need to test of an event with the same
// id exists. If so what we need is modification instead
// of addition.

@@ +66,5 @@
> +    let event = cal.createEvent(iCalString);
> +    event.icalString = iCalString;
> +    
> +    // set Title (Summary) 					  		   
> +    event.title = status + ": 1 Email";

This needs to be l10n'able.

@@ +69,5 @@
> +    // set Title (Summary) 					  		   
> +    event.title = status + ": 1 Email";
> +				
> +    // set ID
> +    event.id=id;

Spaces on either side of =

@@ +80,5 @@
> +  /**
> +   * modifyCalendarEvent updates the title of the calendar event
> +   * to the status string
> +   * @param id uniquely identifies the event to be modified it is
> +            nsIMsgDBHdr::messageId

Nit: missing *

@@ +91,5 @@
> +    this.calendar.readOnly = false;
> +    /* First we need to test if such event exists, if not we need
> +     * to create a new event. */
> +    if (!this.retrieveItem(id, this.calendar)) {
> +      //No such event, create one

Nit: space after //

@@ +102,5 @@
> +
> +    let newEvent = cal.createEvent(iCalString);
> +    newEvent.id = id;
> +    newEvent.calendar = this.calendar;
> +    newEvent.title = status + ": 1 Email";

Needs to be l10n'able.

@@ +133,5 @@
> +  mId: null,
> +  mStatus: null,
> +
> +  onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
> +    // stopEventPump();

Why is this commented out? Should it be removed?

@@ +140,5 @@
> +    this.mId = aId;
> +  },
> +
> +  onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
> +    // XXX check success(?); dump un-returned data,

What does this comment mean? Is it necessary? I don't think it adds much...

@@ +147,5 @@
> +}
> +
> +function generateICalString(aDateStr) {
> +  // Strategy is to create iCalString and create Event from that string
> +  let iCalString = "BEGIN:VCALENDAR\n";

Why not something more like this:

let iCalString = "BEGIN:VCALENDAR\n" +
                 "BEGIN:VEVENT\n" +
                 "DTSTART;VALUE=DATE: " + aDateStr + "\n" +
                 "DURATION=PT1D\n" +
                 "BEGIN:VALARM\n" +
                 "ACTION:DISPLAY\n" +
                 "TRIGGER:-PT1M\n" +
                 "END:VALARM\n" +
                 "END:VEVENT\n" +
                 "END:VCALENDAR\n";

@@ +167,5 @@
> +}
> +
> +/* Get the calendar with the given ID, if such a calendar does not exist,
> + * this method will call createNewCalendar. */
> +function getCalendarById(aCalID) {

This function does more than what the header says - not only does it get or create a calendar, it also assigns it to replyManagerCalendar.calendar.

Not only that, but the name of the function, getCalendarById, implies that the function will *return* something (from the name of the header, I'd assume the calendar that was gotten or created).

I think I'd prefer this function, and createNewCalendar, simply return the gotten or created calendar, leaving it up to the caller to assign it to replyManagerCalendar.calendar.

What I'd suggest is adding a new function to replyManagerCalendar called ensureHasCalendar or something, and this function calls getCalendarById, assigning the reuslt to replyManagerCalendar.calendar.

@@ +168,5 @@
> +
> +/* Get the calendar with the given ID, if such a calendar does not exist,
> + * this method will call createNewCalendar. */
> +function getCalendarById(aCalID) {
> +  calendarManager = Cc["@mozilla.org/calendar/manager;1"]

This is being set as a global, which isn't good. Use let. If this is actually supposed to be a global (which I don't think it is), define it with let in global scope near the stop of the file, and call it gCalendarManager.

@@ +169,5 @@
> +/* Get the calendar with the given ID, if such a calendar does not exist,
> + * this method will call createNewCalendar. */
> +function getCalendarById(aCalID) {
> +  calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> +                    .getService(Components.interfaces.calICalendarManager);

The first . should be lined up with the [ in the line above.

@@ +182,5 @@
> +/* Create a new reply manager calendar and assign the id of the calendar
> + * to the "calendar.replymanager.calendarID" preference. */
> +function createNewCalendar() {
> +  let calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> +                    .getService(Components.interfaces.calICalendarManager);

Line up the first . with the first [ in the line above. Replace Components.interfaces with Ci.

@@ +184,5 @@
> +function createNewCalendar() {
> +  let calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> +                    .getService(Components.interfaces.calICalendarManager);
> +  let temp = calendarManager.createCalendar("storage", 
> +              Services.io.newURI("moz-profile-calendar://", null, null));

Try:

let temp = calendarManager.createCalendar("storage",
                                          Services.io.newURI("moz-profile-calendar://",
                                          null,
                                          null));

alternatively:

let temp = calendarManager.createCalendar("storage",
  Services.io.newURI("moz-profile-calendar://", null, null));
^-- two space indent.

@@ +185,5 @@
> +  let calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> +                    .getService(Components.interfaces.calICalendarManager);
> +  let temp = calendarManager.createCalendar("storage", 
> +              Services.io.newURI("moz-profile-calendar://", null, null));
> +  temp.name = "ReplyManagerCalendar";

Does this need to be l10n'able? Will the user ever see this?

@@ +192,5 @@
> +  replyManagerCalendar.calendar = temp;
> +}
> +
> +/* Notify observers of change to the ReplyManagerCalendar */
> +function notifyObservers(aEvent) {

I'm not sure this function saves you much effort. Instead of adding another level of indirection, why not just call Services.obs.notifyObservers where you've been using notifyObservers? You might want to set ReplyManager as a const in the top of the file.

@@ +200,5 @@
> +
> +var replyManagerCalendarManagerObserver = {
> +  init: function() {
> +    let calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> +                    .getService(Components.interfaces.calICalendarManager);

Line up the first . with the [ in the line above. I'll stop mentioning this now.

@@ +215,5 @@
> +  onCalendarDeleting: function(aCalendar) {
> +    if (aCalendar.id == replyManagerCalendar.calendar.id) {
> +      /* The calendar being deleted is our reply manager calendar we need
> +       * to create a new one */
> +      createNewCalendar();

Here, I'd call that function I described earlier - replyManagerCalendar.ensureHasCalendar.

@@ +222,5 @@
> +};
> +
> +var replyManagerCalendarObserver = {
> +  init: function() {
> +    replyManagerCalendar.calendar.addObserver(this);

I'm not wild about how tightly coupled replyManagerCalendarObserver is to replyManagerCalendar here...

@@ +252,5 @@
> +  onLoad: function() {},
> +  
> +  onAddItem: function() {},
> +  
> +  /* The user may change the date of a reminder through the calenar interface,

typo: calenar -> calendar

@@ +259,5 @@
> +  onModifyItem: function(aNewItem, aOldItem) {
> +    /* if the calendar is our ReplyManagerCalendar we know this is the case. */
> +    let calendar = aOldItem.calendar;
> +    if (calendar == replyManagerCalendar.calendar) {
> +      /* Generate a YYYY-MM-DD formated date string */

Can't you use nsIScriptableDateFormat for all of this date formatting instead of rolling your own?

::: calendar/base/modules/replyManagerUtils.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["replyManagerUtils"];

let over var. const might be even better.

@@ +7,5 @@
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +const gPrefBranch = Cc["@mozilla.org/preferences-service;1"]

Just use Services.prefs instead of gPrefBranch.

@@ +16,5 @@
> +Cu.import("resource:///modules/mailServices.js");
> +Cu.import("resource:///modules/Services.jsm");
> +Cu.import("resource:///modules/gloda/index_msg.js");
> +
> +var replyManagerUtils = {

let not var

@@ +20,5 @@
> +var replyManagerUtils = {
> +  /** Get the list of email addresses who have not replied to the message
> +   * @param aGlodaMsg
> +   * @param callback function: receiving four arguments - aGlodaMsg, aCollection, 
> +                               recipients aray and reply status flags array.

Nit - missing *

@@ +29,5 @@
> +      onItemsAdded: function() {},
> +      onItemsModified: function() {},
> +      onItemsRemoved: function() {},
> +      onQueryCompleted: function(aCollection) {
> +        let didReply = new Array(aGlodaMsg.recipients.length);

Instead of this, I think I'd prefer:

let didReply = [];
let recipients = [];

for (let i = 0; i < aGlodaMsg.recipients.length; ++i) {
  let recipient = aGlodaMsg.recipients[i]._value;
  didReply[i] = aCollection.items.some(function(aItem) {
    return aItem.from_value == recipient;
  });
  recipients[i] = recipient;
}

aCallback(aGlodaMsg, aCollection, recipients, didReply);

I didn't test that, but I think the logic is equivalent...or have I missed something?

Also, is _value safe to use? _ indicates that it's an internal value... is there no public accessor?

@@ +71,5 @@
> +      onItemsModified: function() {},
> +      onItemsRemoved: function() {},
> +      onQueryCompleted: function(aCollection) {
> +        //We need to ensure that the message has been indexed
> +        if (aCollection.items.length > 0) 

So what happens if aCollection.items.length == 0? We never call callback?

@@ +86,5 @@
> +  setExpectReplyForHdr: function replyManagerUtils_setExpectReplyForHdr(aMsgHdr, aDateStr) 
> +  {
> +    markHdrExpectReply(aMsgHdr, true, aDateStr);
> +    
> +    if (gPrefBranch.getBoolPref("calendar.replymanager.create_calendar_event_enabled"))

Services.prefs.getBoolPref instead.

@@ +102,5 @@
> +     * before the preference was set to false. */
> +    try {
> +      replyManagerUtils.removeHdrFromCalendar(aMsgHdr);
> +    } catch(e) {
> +      /* There may be no such event so we need this try block. */

Yikes - we don't want to catch all exceptions silently.

At the very least, we should use Components.utils.reportError(e) to report the problem.

@@ +119,5 @@
> +    let callback = function (aGlodaMessage, aCollection, recipientsList, didReply) {
> +      let subject = aGlodaMessage._subject;
> +      let recipients = getNotRepliedRecipients(recipientsList, didReply);
> +      let dateStr = (aDateStr) ? aDateStr : aMsgHdr.getStringProperty("ExpectReplyDate");
> +      let newDate = (aDateStr) ? getDateForICalString(aDateStr) :

Format this one like:

let newDate = (aDateStr) ? getDateForICalString(aDateStr)
                         : null;

@@ +124,5 @@
> +                                 null;
> +      /* When all people have replied to our email, the recipients will be an empty string.
> +       * In that case we need to give the event a more meaningful title.*/
> +      let newStatus = (recipients == "") ? 
> +                      "\"" + subject + "\" : All recipients have replied to this email" :

This needs to be l10n'able.

@@ +132,5 @@
> +    }
> +    if (aDateStr) {
> +      aMsgHdr.setStringProperty("ExpectReplyDate", aDateStr);
> +    }
> +    if (gPrefBranch.getBoolPref("calendar.replymanager.create_calendar_event_enabled"))

Services.prefs.getBoolPref

@@ +141,5 @@
> +   * @param aMsgHdr is an nsIMsgDBHdr object
> +   */
> +  isHdrExpectReply: function replyManagerUtils_isHdrExpectReply(aMsgHdr) {
> +    let isExpectReply = aMsgHdr.getStringProperty("ExpectReply");
> +    if (isExpectReply == "true")

This can be simplified to:

return aMsgHdr.getStringProperty("ExpectReply") == "true";

@@ +153,5 @@
> +   * @param aMsgHdr is the message header associated with this event
> +   */
> +  addHdrToCalendar: function replyManagerUtils_addHdrToCalendar(aMsgHdr) {
> +    let headerParser = MailServices.headerParser;
> +    /* We need to merge the three fields and remove duplicates.

Use // for these. Line 160 is improperly aligned.

@@ +181,5 @@
> +     * than the real value so we need to separete the values.
> +     */
> +    let dateStr = aMsgHdr.getStringProperty("ExpectReplyDate");
> +    let date = getDateForICalString(dateStr);
> +    let status = "\"" + aMsgHdr.subject + "\" is expecting replies from " 

Needs to be l10n'able.

@@ +198,5 @@
> +  openComposeWindow: function replyManagerUtils_openComposeWindow(aGlodaMsg, aCollection, recipientsList, didReply) {
> +    let recipients = getNotRepliedRecipients(recipientsList, didReply);
> +    /* Create the compose window with a mailto url and the recipients and subject will
> +     * be automatically filled in. */
> +    let mailtoURL = "mailto:" + recipients + "?subject=" + aGlodaMsg._subject;

Instead of hand-rolling a mailto: URI, I think you should use nsIMsgCompFields, and call MailServices.compose.OpenComposeWindowWithParams.

Use MXR to find usage of OpenComposeWindowWithParams for clues.

@@ +199,5 @@
> +    let recipients = getNotRepliedRecipients(recipientsList, didReply);
> +    /* Create the compose window with a mailto url and the recipients and subject will
> +     * be automatically filled in. */
> +    let mailtoURL = "mailto:" + recipients + "?subject=" + aGlodaMsg._subject;
> +    let boilerplate = gPrefBranch.getCharPref("calendar.replymanager.boilerplate");

Services.prefs.

@@ +215,5 @@
> + * @param bExpectReply is the boolean value indicating whether
> + *        the message is expecting replies
> + * @param aDate is the expect reply date. It must be provided if
> + *        bExpectReply is true */
> +function markHdrExpectReply(aMsgHdr, bExpectReply, aDate) {

aExpectReply

@@ +228,5 @@
> +  aMsgHdr.setStringProperty("ExpectReply", bExpectReply);
> +  if (bExpectReply)
> +    aMsgHdr.setStringProperty("ExpectReplyDate", aDate);
> +  
> +  //We need to re-index this message to reflect the change to the Gloda attribute

space after //

@@ +243,5 @@
> +  }
> +}
> +
> +function getNotRepliedRecipients(recipientsList, didReply) {
> +  let recipients = [recipient for each ([i, recipient] in Iterator(recipientsList)) if (!didReply[i])].join(",");

> 80 chars

@@ +244,5 @@
> +}
> +
> +function getNotRepliedRecipients(recipientsList, didReply) {
> +  let recipients = [recipient for each ([i, recipient] in Iterator(recipientsList)) if (!didReply[i])].join(",");
> +  return recipients;

might as well drop the alias and return the above.

::: calendar/lightning/content/lightning.js
@@ +125,5 @@
>  pref("calendar.view.useSystemColors", false);
> +
> +//Reply Manager
> +pref("calendar.replymanager.enabled", false);
> +  //Create an associated calendar event when a email is marked expecting reply

These comments need to be aligned properly. Also, space after //

@@ +127,5 @@
> +//Reply Manager
> +pref("calendar.replymanager.enabled", false);
> +  //Create an associated calendar event when a email is marked expecting reply
> +pref("calendar.replymanager.create_calendar_event_enabled", false);
> +  //This is the boilerplateused that will be filled into the content area in the compose window

type? boilerplateused -> boilerplate

::: calendar/lightning/content/replymanager/replyManagerComposeOverlay.js
@@ +6,5 @@
> +
> +function onLoad() {
> +  /* We need both ReplyManager and Gloda indexer enabled to make this feature work. */
> +  let enabled = Services.prefs.getBoolPref("calendar.replymanager.enabled") &
> +                Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled");

Instead of reading the pref, it's safer to check GlodaIndexer.enabled directly.

@@ +10,5 @@
> +                Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled");
> +  let replymanagerhbox = document.getElementById("replymanager-hbox");
> +  if (enabled) {
> +    replymanagerhbox.collapsed = false;
> +    gMsgCompose.addMsgSendListener(replyManagerSendListener);//add reply manager send listener

"add reply manager send listener" I don't think this comment adds much

@@ +16,5 @@
> +     * of the minimonth header and then reset it to the original value can get the color back.*/
> +    //First get the minimonth element and its header
> +    let datepicker = document.getElementById("reminder-date");
> +    let nodeList = document.getAnonymousNodes(datepicker);
> +    for (let i = 0; i < 3; ++i) {

This magic number should be assigned to a const.

@@ +22,5 @@
> +      nodeList = nodeList[0].childNodes;
> +    }
> +    let mmheader = document.getAnonymousElementByAttribute(nodeList[0], "anonid", "minimonth-header");
> +    //Then set the class
> +    mmheader.setAttribute("class", null);

We don't need to set the class to null, if we're overwriting it in the line below.

@@ +53,5 @@
> +
> +
> +/* reply manager compose state listener
> + * In order to communicate with the send listener, instead of using a single
> + * listener object, each time the send listener gets notified, a new compse

typo: compse -> compose

@@ +74,5 @@
> +    let toggle = document.getElementById("other-elements-toggle").checked;
> +    let aDate = document.getElementById("reminder-date").value;
> +    /* the toISOString method is ignoring timezone offsets so we can't take
> +     * the substring from it. We can only manually generate the date string.*/
> +    let intDate = aDate.getDate();

Can't we use nsIScriptableDateFormat for this?

@@ +100,5 @@
> +  let datePicker = document.getElementById("reminder-date");
> +  datePicker.disabled = !toggle.checked;
> +}
> +
> +window.addEventListener("load", onLoad);

I think you should use compose-window-init and compose-window-close here instead, since we recycle the compose window, and that can cause weirdness.

@@ +102,5 @@
> +}
> +
> +window.addEventListener("load", onLoad);
> +window.addEventListener("unload", onUnload);
> +document.getElementById("msgcomposeWindow").addEventListener("compose-window-reopen", onLoad);

can't you just use window.addEventListener for this?

::: calendar/lightning/content/replymanager/replyManagerDateDialog.xul
@@ +6,5 @@
> +<?xml-stylesheet href="chrome://calendar/content/widgets/calendar-widget-bindings.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://calendar/skin/widgets/minimonth.css" type="text/css"?>
> +<!DOCTYPE dialog  SYSTEM "chrome://lightning/locale/replyManager.dtd">
> +
> +<dialog id="replyManagerDateDialog" title="&replyManagerDateDialog.title;" 

Put title on the next line and indent.

@@ +18,5 @@
> +        flex="1">
> +
> +<script type="application/javascript" src="chrome://calendar/content/calUtils.js"/>
> +<script type="application/javascript" src="chrome://calendar/content/calendar-ui-utils.js"/>
> +<script type="text/javascript">

Javascript should be in its own file, and not inlined.

@@ +56,5 @@
> +]]>
> +</script>
> +
> +<dialogheader title="&replyManagerDateDialogHeader.title;"/>
> +<minimonth id="replyManagerDatePicker"/>

These elements should be indented 2 spaces since they're inside <dialog>. Same with the <script>'s above.

::: calendar/lightning/content/replymanager/replyManagerMailWindowOverlay.js
@@ +10,5 @@
> +function onLoad() 
> +{
> +  let replyManagerMenu = document.getElementById("replyManagerMailContextMenu");
> +  replyManagerMenu.hidden =
> +         !Services.prefs.getBoolPref("calendar.replymanager.enabled");

Only indent this line two spaces.

@@ +24,5 @@
> +        replyManagerMenu.hidden = true;
> +      } else {
> +        replyManagerMenu.hidden = false;
> +      }
> +    });

Format this block like this:

document.getElementById("mailContext")
        .addEventListener("popupshowing", function() {
  replyManagerMenu.hidden = (gFolderDisplay.selectedMessage == null);
});

@@ +37,5 @@
> +  prefs: null,
> +
> +  onLoad: function()
> +  {
> +    this.prefs = Components.classes["@mozilla.org/preferences-service;1"]

Use Services.prefs.

@@ +103,5 @@
> +  let msgHdr = gFolderDisplay.selectedMessage;
> +  /* Since we are going to change the property of the email, we
> +   * need to reflect this change to the header view pane. Thus
> +   * hdrViewDeployItems is called in order to make this change.*/
> +  if (checkbox.getAttribute("checked") == "true") {

Just use checkbox.checked - see https://developer.mozilla.org/en-US/docs/XUL/checkbox

@@ +108,5 @@
> +    replyManagerUtils.resetExpectReplyForHdr(msgHdr);
> +    checkbox.setAttribute("checked", "false");
> +    menuitem.setAttribute("disabled", "true");
> +    replyManagerHdrViewWidget.hdrViewDeployItems();
> +  } else if (checkbox.getAttribute("checked") == "false") {

If checkbox.checked isn't true, then it's not checked.  You don't need this other condition check here.

@@ +113,5 @@
> +    let params = {
> +      inMsgHdr: msgHdr,
> +      outDate: null
> +    };
> +    window.openDialog("chrome://lightning/content/replyManagerDateDialog.xul", "replyManagerDateDialog",

> 80 chars

@@ +196,5 @@
> +  msgsDeleted: function(aItems) {
> +    let mailEnumerator = aItems.enumerate();
> +    while (mailEnumerator.hasMoreElements()) {
> +      let msg = mailEnumerator.getNext()
> +                              .QueryInterface(Components.interfaces.nsIMsgDBHdr);

You shouldn't use QueryInterface in Javascript - you should use instanceof instead.

So:

let msg = mailEnumerator.getNext()

if (msg instanceof Components.interfaces.nsIMsgDBHdr &&
    replyManagerUtils.isHdrExpectReply(msg)) {
  replyManagerUtils.resetExpectReplyForHdr(msg);
}

::: calendar/lightning/content/replymanager/replyManagerMailWindowOverlay.xul
@@ +7,5 @@
> +<!ENTITY % replyManagerDTD SYSTEM "chrome://lightning/locale/replyManager.dtd">
> +%replyManagerDTD;
> +]>
> +<overlay id="replyManagerMailWindowOverlay"
> +  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Indent second line so that it appears like:

<overlay id="replyManagerMailWindowOverlay"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

@@ +9,5 @@
> +]>
> +<overlay id="replyManagerMailWindowOverlay"
> +  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +<script type="application/javascript" src="chrome://lightning/content/replyManagerMailWindowOverlay.js"/>

The contents of the overlay tag should be indented to spaces.

::: calendar/lightning/content/replymanager/replyManagerMsgHdrViewOverlay.js
@@ +35,5 @@
> +    let params = {
> +      inMsgHdr: msgHdr,
> +      outDate: null
> +    };
> +    window.openDialog("chrome://lightning/content/replyManagerDateDialog.xul", "replyManagerDatePicker",

> 80 chars

@@ +53,5 @@
> +  let params = {
> +    inMsgHdr: msgHdr,
> +    outDate: null
> +  };
> +  window.openDialog("chrome://lightning/content/replyManagerDateDialog.xul", "replyManagerDateDialog",

> 80 chars

@@ +65,5 @@
> +
> +/* open the dialog showing the list of mail addresses that have not responded to
> + * the displayed message. */
> +function showNotReplied() {
> +  let openDialogFunction = function(aGlodaMsg, aCollection, recipients, didReply) {

Instead of having these arrays, I think it'd be way better to pass objects, so that we could have

let recipient = {
  didReply: true,
  value: recipientValue
}

Possible?

@@ +75,5 @@
> +    let params = {
> +      inAddressList: addressList,
> +      outSendReminder: null,
> +    };
> +    window.openDialog("chrome://lightning/content/replyManagerShowAddressDialog.xul","replyManagerDateDialog",

> 80 chars

@@ +82,5 @@
> +    if (params.outSendReminder) {
> +      replyManagerUtils.startReminderComposeForHdr(aGlodaMsg.folderMessage);
> +    }
> +  };
> +  replyManagerUtils.getNotRepliedForHdr(replyManagerHdrViewListener.displayedMessage, openDialogFunction);

> 80 chars

@@ +237,5 @@
> +  hdrViewDeployItems: function() {
> +    let msgHdr = replyManagerHdrViewListener.displayedMessage;
> +    /* If ReplyManager is disabled we should not show either of the boxes. */
> +    if(!Services.prefs.getBoolPref("calendar.replymanager.enabled") ||
> +       !Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled") ||

Should use GlodaIndexer.enabled

::: calendar/lightning/content/replymanager/replyManagerMsgHdrViewOverlay.xul
@@ +7,5 @@
> +<!ENTITY % replyManagerDTD SYSTEM "chrome://lightning/locale/replyManager.dtd">
> +%replyManagerDTD;
> +]>
> +<overlay id="replyManagerMsgHdrViewOverlay"
> +  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Line up xmlns with id

@@ +9,5 @@
> +]>
> +<overlay id="replyManagerMsgHdrViewOverlay"
> +  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +<script type="application/javascript" src="chrome://lightning/content/replyManagerMsgHdrViewOverlay.js"/>

The contents of the overlay node should be indented 2 spaces.

@@ +47,5 @@
> +          <label id="notAllRepliedLabel" value="&notAllRepliedLabel.value;" role="textbox"
> +                 aria-readonly="true"/>
> +
> +          <label id="ExpectReplyDateLabel" flex="1" role="textbox"
> +	           aria-readonly="true"/>

Line up aria-readonly with id above.

::: calendar/lightning/content/replymanager/replyManagerPreferences.js
@@ +6,5 @@
> + 
> +/**
> + * Global Object to hold methods for the Reply Manager pref pane
> + */
> +var gReplyManagerPane = {

let

@@ +19,5 @@
> +   * to show the categories saved in preferences.
> +   */
> +  init: function() {
> +    this.enableReplyManagerCheckbox = document
> +              .getElementById("enableReplyManagerCheckbox");

If you need to break this up, do so like this:

this.enableReplyManagerCheckbox =
  document.getElementById("enableReplyManagerCheckbox");

@@ +32,5 @@
> +  toggleReplyManagerEnabled: function() {
> +    this.enableElements(this.enableReplyManagerCheckbox.checked);
> +  },
> +  
> +  enableElements: function(bEnabled) {

Function parameter should start with a, so aEnabled.

::: calendar/lightning/content/replymanager/replyManagerPreferences.xul
@@ +10,5 @@
> +
> +<overlay id="ReplyManagerPaneOverlay"
> +         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +<vbox id="calPreferencesBoxReplyManager">

These tags should be indented, since they're inside the overlay tag.

::: calendar/lightning/content/replymanager/replyManagerShowAddressDialog.xul
@@ +5,5 @@
> +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://lightning/skin/replyManager.css"?>
> +<!DOCTYPE dialog  SYSTEM "chrome://lightning/locale/replyManager.dtd">
> +
> +<dialog id="replyManagerShowAddressDialog" title="&replyManagerShowAddressDialog.title;" 

Please format like this:

<dialog id="replyManagerShowAddressDialog"
        title="&replyManagerShowAddressDialog.title;"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        onload="onLoad();"
        buttons="accept,cancel"
        ...etc
        flex="1">

@@ +14,5 @@
> +ondialogaccept="return doOK();"
> +ondialogcancel="return doCancel();"
> +flex="1">
> +
> +<script type="text/javascript">

This script should be in a separate JS file.

@@ +35,5 @@
> +  return true;
> +}
> +]]>
> +</script>
> +<dialogheader title="&replyManagerShowAddressDialogHeader.title;"/>

These two tags should be indented, since they're inside dialog.
Attachment #651786 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #27)

> I'm not sure how Fallen likes things in the calendar codebase, but in mail/,
> we prefer let over var (even at global scope), for more sensical scoping
> reasons.
Sounds fine to me. I'm undecided on global scope, but everywhere else I tend to advocate for using let.

> We usually also alias Components.interfaces to Ci, that way you can use
> Ci.calICalendarManager down below.
calendar hasn't been using Ci/Cu/Cc/Cr very widely, mostly to avoid running into problems when its being defined more than once.




> @@ +80,5 @@
> > +  /**
> > +   * modifyCalendarEvent updates the title of the calendar event
> > +   * to the status string
> > +   * @param id uniquely identifies the event to be modified it is
> > +            nsIMsgDBHdr::messageId
> 
> Nit: missing *

Also, consider adding some whitespaces betweeen "id" and the parameters's description.


> > +function generateICalString(aDateStr) {
> > +  // Strategy is to create iCalString and create Event from that string
> > +  let iCalString = "BEGIN:VCALENDAR\n";
> 
> Why not something more like this:
> 
> let iCalString = "BEGIN:VCALENDAR\n" +
>                  "BEGIN:VEVENT\n" +
>                  "DTSTART;VALUE=DATE: " + aDateStr + "\n" +
>                  "DURATION=PT1D\n" +
>                  "BEGIN:VALARM\n" +
>                  "ACTION:DISPLAY\n" +
>                  "TRIGGER:-PT1M\n" +
>                  "END:VALARM\n" +
>                  "END:VEVENT\n" +
>                  "END:VCALENDAR\n";

I haven't checked the context on this, but you may consider creating the event and modiying its props instead, i.e:

let item = cal.createEvent();
item.startDate = cal.createDateTime(aDateStr);
let alarm = cal.createAlarm();
let offset cal.createDuration();
offset.minutes = 1;
alarm.offset = offset;
alarm.action = "DISPLAY";
item.addAlarm(alarm);

Then you can directly use the item.


> > +function createNewCalendar() {
> > +  let calendarManager = Cc["@mozilla.org/calendar/manager;1"]
> > +                    .getService(Components.interfaces.calICalendarManager);
> > +  let temp = calendarManager.createCalendar("storage", 
> > +              Services.io.newURI("moz-profile-calendar://", null, null));
> 
> Try:
> 
> let temp = calendarManager.createCalendar("storage",
>                                          
> Services.io.newURI("moz-profile-calendar://",
>                                           null,
>                                           null));
> 
> alternatively:
> 
> let temp = calendarManager.createCalendar("storage",
>   Services.io.newURI("moz-profile-calendar://", null, null));
> ^-- two space indent.

Alternatively:

let calUri = Services.io.newURI("moz-profile-calendar://", null, null);
let temp = cal.getCalendarManager().createCalendar("storage", calUri);


Consider using cal.getCalendarManager() wherever you use the calendar manager. This function is in calUtils.jsm.

> > +  temp.name = "ReplyManagerCalendar";
> 
> Does this need to be l10n'able? Will the user ever see this?
Unless its changed later, its likely visible. I'd suggest just naming it "Reply Manager" and making it l10n'able.



(gPrefBranch.getBoolPref("calendar.replymanager.create_calendar_event_enabled"))
> 
> Services.prefs.getBoolPref instead.

cal.getPrefSafe()

> > +  msgsDeleted: function(aItems) {
> > +    let mailEnumerator = aItems.enumerate();
> > +    while (mailEnumerator.hasMoreElements()) {
> > +      let msg = mailEnumerator.getNext()
> > +   
                           .QueryInterface(Components.interfaces.nsIMsgDBHdr);
> 
> You shouldn't use QueryInterface in Javascript - you should use instanceof
> instead.
This looks to me like you should be using TB's iteratorUtils, in which case you don't need to QI anyway, since its done for you.


This is for me also only a first pass with some comments on Mike's review.
Comment on attachment 651786 [details] [diff] [review]
Complete Patch (build with the "--enable-calendar" option)

The UI on this seems pretty minimal, and I think we want to make it a little more obvious.  For instance, I didn't see any menu items in the "Message" menu when viewing a sent message, and there weren't any Reply-Manager-related buttons I could drag into the compose toolbar or the message header toolbar.  It's a great feature, I think it should be more visible!  :)

(Now, part of this could be a mis-compilation on my part.  If you wanted to kick off a try-server build (or ask Protz to kick one off for you, details at https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer ), I would be happy to test that instead.)

Thanks for your work on this, and I look forward to seeing the next iteration of this patch!

Later,
Blake.
Attachment #651786 - Flags: ui-review?(bwinton) → ui-review-
In this patch I addressed the problems from Mike's and Philipp's comments. Most of them are coding style issues. Thank you for such detailed comments.
Attachment #651786 - Attachment is obsolete: true
Attachment #651786 - Flags: review?(philipp)
Attachment #653733 - Flags: review?(philipp)
Attachment #653733 - Flags: review?(mconley)
Hi Mike,

Thank you for the review. I've fixed most of them and for the following ones the reasons are:

> @@ +259,5 @@
> > +  onModifyItem: function(aNewItem, aOldItem) {
> > +    /* if the calendar is our ReplyManagerCalendar we know this is the case. */
> > +    let calendar = aOldItem.calendar;
> > +    if (calendar == replyManagerCalendar.calendar) {
> > +      /* Generate a YYYY-MM-DD formated date string */

> Can't you use nsIScriptableDateFormat for all of this date formatting instead of rolling your own?

This part won't be shown to the user. The date string is stored as a string property of the message header to indicate the deadline for replies. Since YYYY-MM-DD is the simplest format, I prefer this fixed date format. But nsIScriptableDateFormat won't format the date to this format. This applies to the one used in replyManagerComposeOverlay.js.


> @@ +22,5 @@
> > +      nodeList = nodeList[0].childNodes;
> > +    }
> > +    let mmheader = document.getAnonymousElementByAttribute(nodeList[0], "anonid", "minimonth-header");
> > +    //Then set the class
> > +    mmheader.setAttribute("class", null);

> We don't need to set the class to null, if we're overwriting it in the line below.

The class is not overwritten. What we need here is to change it and change it back so that the correct theme is applied.


> @@ +103,5 @@
> > +  let msgHdr = gFolderDisplay.selectedMessage;
> > +  /* Since we are going to change the property of the email, we
> > +   * need to reflect this change to the header view pane. Thus
> > +   * hdrViewDeployItems is called in order to make this change.*/
> > +  if (checkbox.getAttribute("checked") == "true") {

> Just use checkbox.checked - see https://developer.mozilla.org/en-US/docs/XUL/checkbox

Actually the checkbox here is a menuitem with a type of checkbox so it doesn't have a checked property so I can only check its state using the XUL attribute. I should make the identifier more meaningful.:)

Thank you very much!

Han Lin
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #29)
> Comment on attachment 651786 [details] [diff] [review]
> Complete Patch (build with the "--enable-calendar" option)
> 
> The UI on this seems pretty minimal, and I think we want to make it a little
> more obvious.  For instance, I didn't see any menu items in the "Message"
> menu when viewing a sent message, and there weren't any
> Reply-Manager-related buttons I could drag into the compose toolbar or the
> message header toolbar.  It's a great feature, I think it should be more
> visible!  :)
> 
> (Now, part of this could be a mis-compilation on my part.  If you wanted to
> kick off a try-server build (or ask Protz to kick one off for you, details
> at https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer ), I would
> be happy to test that instead.)
> 
> Thanks for your work on this, and I look forward to seeing the next
> iteration of this patch!
> 
> Later,
> Blake.

Hi Blake,

I have been trying to design the interface in a way that involves minimal user interaction so I didn't not add in toolbarbuttons. But I have one in mind - a button to show all marked messages when clicked. I have this function in the mailContext menu but it may be a good idea to add it to the toolbarpalette. I'll try to come up with more.

Thanks!

Han Lin
Comment on attachment 653733 [details] [diff] [review]
Complete Patch (build with the "--enable-calendar" option)

This looks OK to me, but Fallen gets the final word, since this is his problem / code domain (hence the f+, as opposed to the r+).
Attachment #653733 - Flags: review?(mconley) → feedback+
Some buttons are added to the mail toolbar palette. They are "mark/unmark", "change deadline" and "show all marked messages" buttons.
A "Reply Manager" menu is also added to the Message menu in the menu bar.
Attachment #653733 - Attachment is obsolete: true
Attachment #653733 - Flags: review?(philipp)
Attachment #655959 - Flags: ui-review?(bwinton)
Attachment #655959 - Flags: review?(philipp)
(In reply to Mike Conley (:mconley) from comment #33)
> Comment on attachment 653733 [details] [diff] [review]
> Complete Patch (build with the "--enable-calendar" option)
> 
> This looks OK to me, but Fallen gets the final word, since this is his
> problem / code domain (hence the f+, as opposed to the r+).

Thanks! :)
(In reply to hanlin.dev from comment #32)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #29)
> > The UI on this seems pretty minimal, and I think we want to make it a little
> > more obvious.  For instance, I didn't see any menu items in the "Message"
> > menu when viewing a sent message, and there weren't any
> > Reply-Manager-related buttons I could drag into the compose toolbar or the
> > message header toolbar.  It's a great feature, I think it should be more
> > visible!  :)
> 
> I have been trying to design the interface in a way that involves minimal
> user interaction so I didn't not add in toolbarbuttons. But I have one in
> mind - a button to show all marked messages when clicked. I have this
> function in the mailContext menu but it may be a good idea to add it to the
> toolbarpalette. I'll try to come up with more.

Okay, so I'm a little unclear as to how I would do a couple of tasks.

1) I'm writing email from the compose window, and I expect a reply tomorrow.
   I don't see any buttons, or fields so I'm not sure how to get that.
   There also don't seem to be any menu items.

2) I have a previously-written email I would like a reply from.
   I don't see any indication of the current reply-by date, or how to change it.

I also just noticed the error "JavaScript error: chrome://lightning/content/replyManagerMailWindowOverlay.js, line 54: gMessageListeners is not defined", which makes me wonder if something crazy has happened with my build.

Mike, can you build this patch, and if it seems to work for you, steal the UI-Review from me?

Thanks,
Blake.
Comment on attachment 655959 [details] [diff] [review]
Complete Patch (build with the "--enable-calendar" option)

Stealing ui-review.

Han Lin - this is a pretty cool feature! I have a few comments / suggestions about the UI, however.

Here we go:

For the pane in the Preferences dialog:
* I'm not in love with group within a group there - too many boxes.
* The checkbox within a caption isn't a pattern that we use. I'd just avoid groups entirely, unless there are explicit sections to break up.
* Also, this is a cool feature, and I wonder if we should enable it by default...up to Fallen, I guess.

In the main 3pane window:
* The View Expect-Reply Messages button needs an icon, plus language update... "View Expect-Reply Messages" is a little confusing. Maybe "List Messages Expecting Reply".
* Unmark Expect-Reply Messages needs icon, plus language update - maybe "Stop Expecting Reply"
* Change Deadline needs an icon, and maybe it should be "Change Reply Deadline".

In the message reader / preview pane:

* Other Actions > Send Reminder doesn't pre-populate TO field.
* I think we should move the Change Deadline toolbar button in here.
* Same thing with Unmark Expect-Reply toolbar button.

When viewing a message that is expecting a reply:
* If "Nobody" has responded, what is happening when I press that button?
* Datepicker for changing deadline is not-obvious, and gets weirdly tall on multiple lines
* Buttons should be shrunk to size of text, or be right-aligned.


Composer:
* "Expecting replies" checkbox left side should be lined up with address / subject fields
* "Expecting replies" above and below margins do not match
* Distance between "Expecting replies by" and datepicker should be equal to distance between From: and identity dropdown
* Not sure if default to today makes sense. Maybe tomorrow?
* I don't like that if I set it to today, it immediately reminds me / alerts me. Maybe set it to the end of the work day, or if we're after work day, to midnight?

Let me know if you have any questions about these.
Attachment #655959 - Flags: ui-review?(bwinton) → ui-review-
(In reply to Blake Winton (:bwinton - Thunderbird UX)[On vacation until Sept. 11th.] from comment #36)
> (In reply to hanlin.dev from comment #32)
> > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #29)
> > > The UI on this seems pretty minimal, and I think we want to make it a little
> > > more obvious.  For instance, I didn't see any menu items in the "Message"
> > > menu when viewing a sent message, and there weren't any
> > > Reply-Manager-related buttons I could drag into the compose toolbar or the
> > > message header toolbar.  It's a great feature, I think it should be more
> > > visible!  :)
> > 
> > I have been trying to design the interface in a way that involves minimal
> > user interaction so I didn't not add in toolbarbuttons. But I have one in
> > mind - a button to show all marked messages when clicked. I have this
> > function in the mailContext menu but it may be a good idea to add it to the
> > toolbarpalette. I'll try to come up with more.
> 
> Okay, so I'm a little unclear as to how I would do a couple of tasks.
> 
> 1) I'm writing email from the compose window, and I expect a reply tomorrow.
>    I don't see any buttons, or fields so I'm not sure how to get that.
>    There also don't seem to be any menu items.
> 
> 2) I have a previously-written email I would like a reply from.
>    I don't see any indication of the current reply-by date, or how to change
> it.
> 
> I also just noticed the error "JavaScript error:
> chrome://lightning/content/replyManagerMailWindowOverlay.js, line 54:
> gMessageListeners is not defined", which makes me wonder if something crazy
> has happened with my build.
> 
> Mike, can you build this patch, and if it seems to work for you, steal the
> UI-Review from me?
> 
> Thanks,
> Blake.

I set the feature to disabled by default so the elements are not showing. So for the first time we need to enable it in the preference window. Maybe I should set it to enabled by default.
(In reply to Mike Conley (:mconley) from comment #37)
> Comment on attachment 655959 [details] [diff] [review]
> Complete Patch (build with the "--enable-calendar" option)
> 
> Stealing ui-review.
> 
> Han Lin - this is a pretty cool feature! I have a few comments / suggestions
> about the UI, however.
> 
> Here we go:
> 
> For the pane in the Preferences dialog:
> * I'm not in love with group within a group there - too many boxes.
> * The checkbox within a caption isn't a pattern that we use. I'd just avoid
> groups entirely, unless there are explicit sections to break up.
> * Also, this is a cool feature, and I wonder if we should enable it by
> default...up to Fallen, I guess.
I'll change the look of the preference window and maybe I should set it to enabled by default.

> In the main 3pane window:
> * The View Expect-Reply Messages button needs an icon, plus language
> update... "View Expect-Reply Messages" is a little confusing. Maybe "List
> Messages Expecting Reply".
> * Unmark Expect-Reply Messages needs icon, plus language update - maybe
> "Stop Expecting Reply"
> * Change Deadline needs an icon, and maybe it should be "Change Reply
> Deadline".
When I coded these buttons I could not find suitable icons for these buttons. I'll try to look for some later.

> In the message reader / preview pane:
> 
> * Other Actions > Send Reminder doesn't pre-populate TO field.
> * I think we should move the Change Deadline toolbar button in here.
> * Same thing with Unmark Expect-Reply toolbar button.
Hmm, if someone did forget to reply, his address should appear in the TO field. Is there any error message in the error console?
The toolbarbuttons have their menuitem counterparts in the otherActionsPopup.
The "Expect Reply" menuitem (a checkbox typed item) is used for marking/unmarking.
The "Modify Deadline" is for changing the deadline. I added the toolbarbuttons to make the more visible to the user.

> When viewing a message that is expecting a reply:
> * If "Nobody" has responded, what is happening when I press that button?
A conversation tab with only the sent message will appear.
> * Datepicker for changing deadline is not-obvious, and gets weirdly tall on
> multiple lines
The datepicker isn't particular to any line of text so I thought I may make it reside beside the two text boxes and this makes it a bit tall. I considered using a datepicker but it would make the text a bit long. Is this better?
> * Buttons should be shrunk to size of text, or be right-aligned.
> 
> 
> Composer:
> * "Expecting replies" checkbox left side should be lined up with address /
> subject fields
> * "Expecting replies" above and below margins do not match
> * Distance between "Expecting replies by" and datepicker should be equal to
> distance between From: and identity dropdown
> * Not sure if default to today makes sense. Maybe tomorrow?
Good idea.
> * I don't like that if I set it to today, it immediately reminds me / alerts
> me. Maybe set it to the end of the work day, or if we're after work day, to
> midnight?
The events are made whole day event so reminder will also pop up immediately when the date is modified. Is there a way to suppress the alert for once?
> 
> Let me know if you have any questions about these.

Sorry I replied so late I was taken away by schools affairs.
Assignee: nobody → hanlin.dev
Status: NEW → ASSIGNED
Attachment #690080 - Flags: ui-review?(philipp)
Attachment #690080 - Flags: review?(philipp)
Attachment #655959 - Attachment is obsolete: true
Attachment #655959 - Flags: review?(philipp)
Comment on attachment 690080 [details] [diff] [review]
Build with --enable-calendar option

After discussion with HanLin and protz, we will rework this to an extension for now until the remaining issues (i.e Seamonkey support) are fixed. This also allows us to gather early feedback from the addons community.
Attachment #690080 - Flags: ui-review?(philipp)
Attachment #690080 - Flags: review?(philipp)
Assignee: hanlin.dev → nobody
Status: ASSIGNED → NEW
Version: Trunk → unspecified
Type: defect → enhancement
Component: Lightning Only → General
OS: All → Unspecified
Hardware: All → Unspecified
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: