make the prototype event dialog work for sunbird

VERIFIED FIXED in 0.7

Status

Calendar
Sunbird Only
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Michael Büttner (no reviews TFN), Assigned: Michael Büttner (no reviews TFN))

Tracking

Bug Flags:
blocking-calendar0.7 +

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

11 years ago
since we agreed that the prototype dialog will be enabled by default I need to make sure that it works smoothly in sunbird - this is currently not the case.
(Assignee)

Comment 1

11 years ago
Created attachment 256908 [details]
binary-files v1

this file contains the address-book icon that should be displayed next to the name of each attendee. since taking it directly from the messenger-folder no longer is an option, i need to put it into calendar/resources/skin.
(Assignee)

Comment 2

11 years ago
Created attachment 256911 [details] [diff] [review]
patch v1

besides some packaging issues (missing files in the appropriate jar.mn's) the following bits and pieces have been addresses to make the new dialog available for sunbird.
* preprocess sun-calendar-event-dialog.xul to optionally include the necessary *.js files.
* address book icon is now available from local skin folder instead of pulling it directly from the messenger folder.
* LDAP auto completion is now optionally available in case the appropriate service is installed
* parsing of the attendee names now doesn't rely on the headerparser service any longer. this is necessary in case LDAP auto completion produces strings like 'name <email>'.
* some menu entries (e.g. new address book card) will now be dynamically removed from the menu if we're running under sunbird.
* opening release notes/about dialog now forwards the calls depending on the application we're running in.
Attachment #256911 - Flags: first-review?(mvl)
(Assignee)

Comment 3

11 years ago
lilmatt just reminded me that we want to get rid of m\c\resources and stick the icon in m\c\base\themes. mvl, just assume that this patch does it right for now ;-)
Comment on attachment 256911 [details] [diff] [review]
patch v1

>+++ mozilla/calendar/lightning/jar.mn	2007-03-01 15:32:55.875000000 +0100
> #expand    skin/classic/calendar/prevnextarrow.png  (/calendar/sunbird/themes/__THEME__/sunbird/prevnextarrow.png)
>+#expand    skin/classic/calendar/abcard.png         (/calendar/resources/skin/classic/abcard.png)

That file should be in the same dir as the other files coming from this jar.mn. (and use __THEME__ to make it work)
Comment on attachment 256911 [details] [diff] [review]
patch v1

>+          if(component)
>+            this.mHeaderParser = component.getService(Components.interfaces.nsIMsgHeaderParser);

Nit: The file ssems to be mostly 4 space indent. So use that for new code too. (some for a lot of the other code you add)
Also add a space between the if and the (. (same for all other if statements you add)

r- for now based on the theme issue i mentioned in the previous comment.
Attachment #256911 - Flags: first-review?(mvl) → first-review-

Comment 6

11 years ago
Seeing also for a wcap-case:
http://groups.google.com/group/mozilla.dev.apps.calendar/browse_thread/thread/bce3fdfb29a9d7e5/e2a9168549ddd080
shouldn't this go into 0.5 so bith lightning and sunbird can profit from the new dialog? 
Flags: blocking-calendar0.5?

Comment 7

11 years ago
(In reply to comment #6)
We agreed to switch the dialog ASAP after 0.5 release (and 0.5 needs to go out ASAP).
Clearing the flag per comment#7.
Flags: blocking-calendar0.5?

Updated

11 years ago
Flags: blocking-calendar0.7?

Updated

11 years ago
Status: NEW → ASSIGNED
Flags: blocking-calendar0.7? → blocking-calendar0.7+
(Assignee)

Comment 9

11 years ago
Created attachment 270576 [details]
binary-files v2

according to comment #3 the icons are now living under m\c\base\themes.
Attachment #256908 - Attachment is obsolete: true
(Assignee)

Comment 10

11 years ago
Created attachment 270577 [details] [diff] [review]
patch v2

I incorporated the previous comments and updated the patch so that it applies again.
Attachment #256911 - Attachment is obsolete: true
Attachment #270577 - Flags: review?(philipp)

Updated

11 years ago
Blocks: 386589
Comment on attachment 270577 [details] [diff] [review]
patch v2

In general what I noticed is that the Attendee dialog is of course a bit bloated, since in sunbird we don't have an ldap service (correct me if I am wrong). It would be nice if we could have an attendee-dialog-light that shows if either no ldap service is available or there is no ldap server configured (or the calendar provider doesn't support attendee freebusy, but thats another issue). I'm perfectly happy if this happens in a spin-off bug, I even have the feeling there may already be one.

Additionally, please run the review script I gave you to get rid of some whitespace problems and such.


>+        // this will be called if file->new has been selected from within the dialog
>+        args.onNewEvent = function(calendar) {
>+            createEventWithDialog(calendar, null, null);
>+        }
>+          
There are a few ^M characters here that shouldn't be there.

>-  content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
>+* content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
Before we do the extra preprocessing here, can't we just use nsIStringBundle in sun-calendar-event-dialog.js ? This would also be consistent with other functions in that file, that also use nsIStringBundle.

>+  skin/classic/calendar/abcard.png         (/calendar/base/themes/common/abcard.png)
I think at some point we had the jar.mn files sorted alphabetically. It seems that this has changed. I'd like to restore that order, if possible. As a start could you sort this file in at the top?

>+          var component = Components.classes["@mozilla.org/messenger/headerparser;1"];
>+          if(component)
>+              this.mHeaderParser = component.getService(Components.interfaces.nsIMsgHeaderParser);
Aside from the missing blank, please use {} even for one-line if blocks.

>+                if (this.mHeaderParser) {
>+                    this.mHeaderParser.parseHeadersWithArray(fieldValue,emailAddresses,names,fullNames);
Missing blanks here and in the next couple of chunks. I know this may be a xul file but as long as it stays readable try to keep the line under 80 chars.

>+            if (this.mPrefBranchInternal) {
>+                this.mPrefBranchInternal.addObserver("ldap_2.autoComplete.useDirectory",
>+                                                     this.mDirectoryServerObserver, false);
>+                this.mPrefBranchInternal.addObserver("ldap_2.autoComplete.directoryServer",
>+                                                     this.mDirectoryServerObserver, false);
>+            }
When using multiple lines, one argument per line.

>+            if (this.mPrefBranchInternal) {
>+                this.mPrefBranchInternal.removeObserver("ldap_2.autoComplete.useDirectory", this.mDirectoryServerObserver);
>+                this.mPrefBranchInternal.removeObserver("ldap_2.autoComplete.directoryServer", this.mDirectoryServerObserver);

This might not really be part of this patch, but you dont need an extra var for nsIPrefBranch2. The following should work just fine:

this.mPrefs =  Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService).getBranch(null);
this.mPrefs.QueryInterface(Components.interfaces.nsIPrefBranch2);
this.mPrefs.addObserver(...);

Feel free to address here or elsewhere.


>+// update menu items that rely on focus
>+function goUpdateGlobalEditMenuItems()
>+{
Brackets on same lines as function here and elsewhere


>+  const SUNBIRD_ID = "{718e30fb-e89b-41dd-9da7-e25a45638b28}";
>+  var appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
>+                          .getService(Components.interfaces.nsIXULAppInfo);
>+  if (appInfo.ID == SUNBIRD_ID) {
>+    gIsSunbird = true;
>+    var removeElements = document.getElementsByAttribute("remove-on-sunbird", "true");
>+    while (removeElements.length != 0) {
>+      var parentNode = removeElements[0].parentNode;
>+      parentNode.removeChild(removeElements[0]);
>+    }
>+  }  
Would it be faster to use a css selector on this, similar to what you did with event-only and task-only? You could setAttribute("appType", "sunbird") or such, and then make a rule sunbird-only. I don't have a strong opinion on that though, I'd like to keep it consistent though.

> function onAboutDialog()
> {
>-  openDialog("chrome://messenger/content/aboutDialog.xul", "About", "modal,centerscreen,chrome,resizable=no");
>+  if (gIsSunbird) {
>+    openAboutDialog();
>+  } else {
>+    openDialog("chrome://messenger/content/aboutDialog.xul", "About", "modal,centerscreen,chrome,resizable=no");
>+  }
This is also in the scope of a different bug, but I'd like the openAboutDialog() function to move into base and work for sunbird and lightning. The same goes for onReleaseNotes, whereas openRegionURL can make heavy use of helper functions (the old launchBrowers, a modified version of calGetString).


>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/resources/jar.mn mozilla/calendar/resources/jar.mn
>+    content/calendar/sun-calendar-event-dialog.css  (/calendar/prototypes/wcap/sun-calendar-event-dialog.css)
>+    content/calendar/sun-calendar-event-dialog.js  (/calendar/prototypes/wcap/sun-calendar-event-dialog.js)
>+*   content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
>+    content/calendar/sun-calendar-event-dialog-attendees.xml  (/calendar/prototypes/wcap/sun-calendar-event-dialog-attendees.xml)
Can't this go into base/jar.mn? Same comment as above about the preprocessor here.

>+    skin/classic/calendar/abcard.png  (/calendar/base/themes/common/abcard.png)
>+    skin/classic/calendar/sun-calendar-event-dialog.css  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog.css)
>+    skin/classic/calendar/sun-calendar-event-dialog-attendees.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog-attendees.png)
>+    skin/classic/calendar/sun-calendar-event-dialog-toolbar.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog-toolbar.png)
>+    skin/classic/calendar/sun-calendar-event-dialog.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog.png)
>+    skin/classic/calendar/timezone_0h.png  (/calendar/prototypes/themes/winstripe/timezone_0h.png)
>+    skin/classic/calendar/timezone_1h.png  (/calendar/prototypes/themes/winstripe/timezone_1h.png)

Why only from winstripe? Either use __THEME__ and #expand, or this should move into some common directory. If the files are the same anyway I'm fine with keeping them there until they move out of /calendar/prototypes. A comment about this with a bug number would be nice though.


The dialog works as advertised, but r- for now to clarify the above issues.
Attachment #270577 - Flags: review?(philipp) → review-
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> (From update of attachment 270577 [details] [diff] [review])
> In general what I noticed is that the Attendee dialog is of course a bit
> bloated, since in sunbird we don't have an ldap service (correct me if I am
> wrong). It would be nice if we could have an attendee-dialog-light that shows
> if either no ldap service is available or there is no ldap server configured
> (or the calendar provider doesn't support attendee freebusy, but thats another
> issue). I'm perfectly happy if this happens in a spin-off bug, I even have the
> feeling there may already be one.
I don't think that a missing LDAP service does any harm, since in that case you just can enter the email address manually (the auto completion just uses any available services). But you're totally right that we already discussed the issue that the free/busy grid should be made optional, depending on the provider capabilities. I just filed bug #387997 to address this issue, as this is way out of scope of this bug here.
(Assignee)

Comment 13

10 years ago
Created attachment 272456 [details] [diff] [review]
patch v3

>-  content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
>+* content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
You're right, the extra preprocessing isn't really necessary here. I had to change onReleaseNotes() to accommodate this. I'm now using formatStringFromName() instead of getFormattedString(). I'd be curious on why I had to change this. If you know the details, it would be nice if you could shed some light on this.

>+            if (this.mPrefBranchInternal) {
>+                this.mPrefBranchInternal.removeObserver("ldap_2.autoComplete.useDirectory", this.mDirectoryServerObserver);
>+                this.mPrefBranchInternal.removeObserver("ldap_2.autoComplete.directoryServer", this.mDirectoryServerObserver);
I intentionally split this up. In case nsIPrefBranch2 is not implemented I'm just using the pref service. nsIPrefBranch2 just adds the capability to add an observer to those preference values. Of course I could use QueryInterface() to specifically ask for nsIPrefBranch2  but this would let all the code fail if just the new interface wouldn't be implemented.

>+  const SUNBIRD_ID = "{718e30fb-e89b-41dd-9da7-e25a45638b28}";
>+  var appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
>+                          .getService(Components.interfaces.nsIXULAppInfo);
>+  if (appInfo.ID == SUNBIRD_ID) {
>+    gIsSunbird = true;
>+    var removeElements = document.getElementsByAttribute("remove-on-sunbird", "true");
>+    while (removeElements.length != 0) {
>+      var parentNode = removeElements[0].parentNode;
>+      parentNode.removeChild(removeElements[0]);
>+    }
>+  }  
Nicely spotted, I changed that to use a css selector, I introduced a 'lightning-only' class in accordance to the 'event-only' and 'task-only' classes.

> function onAboutDialog()
> {
>-  openDialog("chrome://messenger/content/aboutDialog.xul", "About", "modal,centerscreen,chrome,resizable=no");
>+  if (gIsSunbird) {
>+    openAboutDialog();
>+  } else {
>+    openDialog("chrome://messenger/content/aboutDialog.xul", "About", "modal,centerscreen,chrome,resizable=no");
>+  }
I think it's reasonable to keep these functions in ...event-dialog.js as it currently stands. This keeps them close to their scope. Of course we could think about moving them to calUtils.js or somewhere else if they get used somewhere else in the application.

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/resources/jar.mn mozilla/calendar/resources/jar.mn
>+    content/calendar/sun-calendar-event-dialog.css  (/calendar/prototypes/wcap/sun-calendar-event-dialog.css)
>+    content/calendar/sun-calendar-event-dialog.js  (/calendar/prototypes/wcap/sun-calendar-event-dialog.js)
>+*   content/calendar/sun-calendar-event-dialog.xul  (/calendar/prototypes/wcap/sun-calendar-event-dialog.xul)
>+    content/calendar/sun-calendar-event-dialog-attendees.xml  (/calendar/prototypes/wcap/sun-calendar-event-dialog-attendees.xml)
This is intentionally kept in resources/jar.mn (the same files are referenced in lightning/jar.mn) as the files are currently living in the prototypes folder and I felt that it would be wrong to put files in base/jar.mn that don't live in calendar/base. I don't have a strong opinion on that, so I'm also willing to move them to base/jar.mn if you like. For now, I let them in the separate jar.mn's.

>+    skin/classic/calendar/abcard.png  (/calendar/base/themes/common/abcard.png)
>+    skin/classic/calendar/sun-calendar-event-dialog.css  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog.css)
>+    skin/classic/calendar/sun-calendar-event-dialog-attendees.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog-attendees.png)
>+    skin/classic/calendar/sun-calendar-event-dialog-toolbar.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog-toolbar.png)
>+    skin/classic/calendar/sun-calendar-event-dialog.png  (/calendar/prototypes/themes/winstripe/sun-calendar-event-dialog.png)
>+    skin/classic/calendar/timezone_0h.png  (/calendar/prototypes/themes/winstripe/timezone_0h.png)
>+    skin/classic/calendar/timezone_1h.png  (/calendar/prototypes/themes/winstripe/timezone_1h.png)
Why only from winstripe? I knew there was a reason why I did it like that. ;-) The define for THEME was missing from the makefile, and (lazy me) did not spot that during the first iteration and so I just used the files from winstripe. Now I added the missing define to the makefile and the jar.mn now correctly uses #expand and __THEME__.
Attachment #270577 - Attachment is obsolete: true
Attachment #272456 - Flags: review?(philipp)
Comment on attachment 272456 [details] [diff] [review]
patch v3

(In reply to comment #13)
> You're right, the extra preprocessing isn't really necessary here. I had to
> change onReleaseNotes() to accommodate this. I'm now using
> formatStringFromName() instead of getFormattedString(). I'd be curious on why I
> had to change this. If you know the details, it would be nice if you could shed
> some light on this.
formatStringFromName() is a method of nsIStringBundle, while getFormattedString() is a method of the xul <stringbundle> element.

> I intentionally split this up. In case nsIPrefBranch2 is not implemented I'm
> just using the pref service. nsIPrefBranch2 just adds the capability to add an
> observer to those preference values. Of course I could use QueryInterface() to
> specifically ask for nsIPrefBranch2  but this would let all the code fail if
> just the new interface wouldn't be implemented.
In what cases would nsIPrefBranch2 not be implemented? Many things would break if we couldn't rely on nsIPrefBranch2 being implemented. We depend on it in many other locations, i.e base/content/calendar-decorated-*-view.xml and also the Calendar Manager.

> I think it's reasonable to keep these functions in ...event-dialog.js as it
> currently stands. This keeps them close to their scope. Of course we could
> think about moving them to calUtils.js or somewhere else if they get used
> somewhere else in the application.
A such function is being used elsewhere. If you consolidate your onAboutDialog() and openAboutDialog() in calendar/resources/applicationUtil.js, then it can be used both in the prototype dialog and in sunbird/base/content/calendar.xul and sunbird/base/content/calendar-menubar.inc. I'm fine with a spinoff bug for this though.

> This is intentionally kept in resources/jar.mn (the same files are referenced
> in lightning/jar.mn) as the files are currently living in the prototypes folder
> and I felt that it would be wrong to put files in base/jar.mn that don't live
> in calendar/base. I don't have a strong opinion on that, so I'm also willing to
> move them to base/jar.mn if you like. For now, I let them in the separate
> jar.mn's.
Since the files don't live in resources either, I'd think its better to put the entries into base right away. This way we can save that step when moving the prototype dialog out of /prototypes/. Also it doesn't require modifying resources/Makefile.in.


Two sets of trailing whitespaces left in sun-calendar-event-dialog-attendees.xml lines 576 and 633, otherwise r+ with these issues fixed.
Attachment #272456 - Flags: review?(philipp) → review+
(Assignee)

Comment 15

10 years ago
Created attachment 273590 [details] [diff] [review]
patch v4

nsIPrefBranch2 - I got rid of the distinct fields for nsIPrefBranch and nsIPrefBranch2. I think you're right that it's just fine to rely on nsIPrefBranch2.

openAboutDialog() / openReleaseNotes() - I merged those functions from sun-calendar-event-dialog.js into the existing code in applicationUtil.js. Admittedly, it makes much more sense to consolidate this stuff. I added applicationUtil.js to /m/c/b/jar.mn in order to make this code available in Lightning.

/base/jar.mn - I moved all those files to base/jar.mn, it really makes more sense to add those files now.

I changed quite some code in order to consolidate the openAboutDialog() & openReleaseNotes() stuff, so I'm asking for review again.
Attachment #272456 - Attachment is obsolete: true
Attachment #273590 - Flags: review?(philipp)

Comment 16

10 years ago
(In reply to comment #15)
> Admittedly, it makes much more sense to consolidate this stuff. I added
> applicationUtil.js to /m/c/b/jar.mn in order to make this code available in
> Lightning.

applicationUtils.js was removed from Lightning just a few days ago with Bug 346762 on purpose.
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> applicationUtils.js was removed from Lightning just a few days ago with Bug
> 346762 on purpose.
Hm, good point. So, any suggestions where openAboutDialog() and openReleaseNotes() wants to live? Do we want to move it to calUtils.js? openReleaseNotes() needs also some function to start up the browser and stuff like  that. It's all about consolidating those functions and have a single location for them.
Comment on attachment 273590 [details] [diff] [review]
patch v4

> calendar.jar:
> % content calendar %content/calendar/
>+*   content/calendar/applicationUtil.js          (/calendar/resources/content/applicationUtil.js)
If the file is referenced in base/jar.mn I think the file should also live in base (with a different name). Can you file a followup bug to give applicationUtil.js an overhaul and to move it into base?

r=philipp
Attachment #273590 - Flags: review?(philipp) → review+

Comment 19

10 years ago
As said in Comment #16 applicationUtils.js was removed from Lightning so that it doesn't ship all the Sunbird only functions. If there are new functions that need to be shared between Sunbird and Lightning they should be added to a shared file instead of applicationUtils.js (probably calUtils.js I guess).
(Assignee)

Comment 20

10 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

10 years ago
I talked to philipp on IRC this morning about whether or not we want to address the issue regarding moving the shared code from applicationUtil.js to calUtils.js in this patch or file a follow-up bug on that. We decided to take the latter approach, that's why I just filed bug #389522.
Target Milestone: --- → 0.7
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6pre) Gecko/20070726 Calendar/0.7pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.