Review prototype event dialog

VERIFIED FIXED in 0.7

Status

Calendar
General
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Michael Büttner (no reviews TFN))

Tracking

Dependency tree / graph
Bug Flags:
blocking-calendar0.7 +

Details

(Whiteboard: [patch in hand])

Attachments

(5 attachments, 3 obsolete attachments)

The prototype event dialog should be reviewed (the code and the UI), and then we need to make sure it gets checked in.
The review should happen on the code in the location it has in the tree. This review can be done piece-by-piece, but once the review is done, it should land in all at once. Landing it piece-by-piece would be too much work. Problems found during the review can be fixed while it is still a prototype.
(Reporter)

Comment 1

11 years ago
This is my first list of things i noticed when looking at the UI and the code.

Main Window
- remove Privacy button from the default set
- remove the progress bar
- remove bar between start/end and repeat
- Investigate how the menubar works on Macs. Does it acutally work? Is it discoverable enough?
- Is it needed to ask if the changes should be saved, given that a) the buttons says 'save and close' and b) we have undo


Attendees
- Create a simple version of the window for use without freebusy
- Consider splitting the 'find a free spot' from 'invite somebody'

Alarms
- It's not clear the 'custom' makes you edit your custom rules. Maybe call it 'edit this list', and then add the default options to be edited?



The Code

sun-calendar-event-dialog.js
- There are a lot of small nits to fixed, like hardcoded strings etc
- updateStartDate, updateEndDate and updateDueDate look a bit too similar...

sun-calendar-event-dialog-recurrence.xul looks a bit weird. It doesn't really contain anything. 
....-recurrence.xml is a bit overkill as xml. I don't think that it will be reused anywhere, so I don't see the advantage of making it xbl.
same two things can be said about ...-attendees.xul.


...-recurrence-preview.xml contains an month grid. But we already have that, as minimonth. That should be reused (maybe with some added callbacks to allow coloring of certain days. That's needed anyway for the main views to indicate busy days. If you need other things, the minimonth widget isn't frozen. We can add stuff)
(Assignee)

Comment 2

11 years ago
I'll just comment on the code section, leaving the UI part for christian ;-)

> sun-calendar-event-dialog.js
> - There are a lot of small nits to fixed, like hardcoded strings etc
> - updateStartDate, updateEndDate and updateDueDate look a bit too similar...
of course, the hardcoded strings will be moved to the right place in no time. the updateXXXDate functions can probably merged to some degree, but I don't see a burning desire for it. What small nits do you mean exactly?

> sun-calendar-event-dialog-recurrence.xul looks a bit weird. It doesn't really
> contain anything. 
> ....-recurrence.xml is a bit overkill as xml. I don't think that it will be
> reused anywhere, so I don't see the advantage of making it xbl.
> same two things can be said about ...-attendees.xul.
Of course I can move the stuff from the bindings to the xul files. Again, I don't feel that the will break apart if it stays as it is. Just tell me whether or not we want to go for it.

> ...-recurrence-preview.xml contains an month grid. But we already have that,
> as minimonth. That should be reused (maybe with some added callbacks to allow
> coloring of certain days. That's needed anyway for the main views to indicate
> busy days. If you need other things, the minimonth widget isn't frozen.
> We can add stuff)
No problem, I totally agree with this. I copied the code since the minimonth widget didn't support several features and I didn't see a change to get this in within a reasonable timeframe.

mvl, what exactly do you suggest as the next steps? I really would like to push this forward. Please forgive me that I didn't comment on this bug earlier, but I didn't have the impression that your concerns were actually in a final shape so that I can provide a patch.
Assignee: nobody → michael.buettner
(Assignee)

Comment 3

11 years ago
Christia, could you please add your comments regarding the UI-relevant part?
(Reporter)

Comment 4

11 years ago
(In reply to comment #2)
> What small nits do you mean exactly?

There are code style things. I did not want to search for all of them, before we have the bigger issues fixed. The easiest way to fix those style things is likely for somebody to go through the code, and just fix all the issues, instead of a huuuge bug comment.

> Of course I can move the stuff from the bindings to the xul files. Again, I
> don't feel that the will break apart if it stays as it is. Just tell me whether
> or not we want to go for it.

It won't break apart, but it is easier to read code without xbl. And it might be faster to load, if there are not bindings to be attached.

> mvl, what exactly do you suggest as the next steps? I really would like to push
> this forward.

You can work on the minimonth issues for example, by adding them. In the meantime, i can continue reading the code.

Comment 5

11 years ago
An I'll comment just the UI stuff. 

This is my first list of things i noticed when looking at the UI and the code.

Main Window
Q: remove Privacy button from the default set
A: OK -> Move it to Customize

Q: remove the progress bar
A: OK

Q: remove bar between start/end and repeat
A: I'm not sure if we want that. Without that bar the ui looks more clutter see attachment.

Q: Investigate how the menubar works on Macs. Does it acutally work? Is it
discoverable enough?
A: I'll investigate soon-

Q: Is it needed to ask if the changes should be saved, given that a) the buttons
says 'save and close' and b) we have undo
A: The question should only occur if:
- Something got changed and users select File -> Close or hit the window closer.


Attendees
Q: Create a simple version of the window for use without freebusy
A: Ok. Will do. I'll ping if I'm done.

Q: Consider splitting the 'find a free spot' from 'invite somebody'
A: What do you mean exactly?

Alarms
Q: It's not clear the 'custom' makes you edit your custom rules. Maybe call it
'edit this list', and then add the default options to be edited?

A: My we should call it 'Create custom reminder...'

Comment 6

11 years ago
Created attachment 257976 [details]
Event Dialog without horizonal bar between "End Date & Repeat" & without Privacy Button
(Reporter)

Comment 7

11 years ago
(In reply to comment #5)
> Q: remove bar between start/end and repeat
> A: I'm not sure if we want that. Without that bar the ui looks more clutter see
> attachment.

Without the extra bar, everything related to the time the event happens in nicely grouped. It doesn't look bad to me.

> Q: Consider splitting the 'find a free spot' from 'invite somebody'
> A: What do you mean exactly?

The window there is now to invite attendees is coupled with the freebusy part. The dialog is quite busy now. If those two things were two windows, it might be less cluttered. But maybe it conflicts with the workflow. The balance is what i think we should investigate.

> Alarms
> Q: It's not clear the 'custom' makes you edit your custom rules. Maybe call it
> 'edit this list', and then add the default options to be edited?
> 
> A: My we should call it 'Create custom reminder...'

Or 'Edit custom reminders...' ? Because that is what you will do.

Comment 8

11 years ago
Created attachment 258473 [details]
Code review of the event prototype dialog

This is my first r1-level review of the event dialog code. I've been disconnected for a few days so I reviewed the code that I last updated on March 9, 2007, I hope it is still up to date.
Attachment #258473 - Attachment mime type: text/plain → text/rtf
Created attachment 258475 [details]
Clint's comments, un-RTFed
Attachment #258473 - Attachment is obsolete: true
Blocks: 293195

Comment 10

11 years ago
My comments on Clint's comments:


>== sun-calendar-event-dialog.xul ==
>* I have to say that I do not like the drop down menu for the dialog.  On other OS's it's probably OK but on Mac that is not a usual paradigm and for the first several times I used the wcap extension I did not notice the bar.  That said, I do like the fact that I don't have to clutter the dialog with that stuff.  Can you do two toolbars one atop the other and put your drop down menus there? That way you stay off the system bar?  That'd be a mac only solution and you'd best check with a UI guru first!
>

I understand your concerns. But let me say that we got no negative feedback on the menu from users within Sun, even Mac users seem to have no problems with it.

Please keep also in mind that we've got with only 4 times negative (from 31 Comments ) feedback [1] regarding the menu.

Anyhow, We can go with a "no menu" solution, but this would means that we need to extend the toolbar by at least 2 items.

Namely "Importance & Show time as". All these items need to come with Drop Downs

Timezone "on/off" could be moved next to the hour picker

>* Alarm choices are too many: Can we condense the alarms down to an equal number per unit?
>For example: 5 min before 
>             10 min before 
>             30 min before
>             ------------------
>             1 hour before
>             2 hours before
>             15 hours before
>          	  -------------------
>             1 day before
>             2 days before
>             7 days before
>	        ----------------------
>                      custom -->
>

OK for me.
>* Status bar panel segment is hard to read spacing needed here

Agreed.

>
>== Overall thoughts ==
>* You have a ton of good stuff here.
>* We need a way to ensure that the attendees can easily be entered from the main pane for those providers that do not suport freebusy.

I'll work out a concept for that.

[1] http://weblogs.mozillazine.org/calendar/2007/01/calendar_sneak_preview_part_1.html#comments
(Assignee)

Comment 11

11 years ago
>* We need a way to ensure that the attendees can easily be entered from the main pane for those providers that do not suport freebusy.

Probably there's an easy solution to this problem. Admittedly, the free/busy grid is totally unnecessary in case we can't display those informations. Currently, this information is only available with the WCAP provider. This situation could probably change in future (google, for example). Thus, it's a property of the provider whether or not the free/busy grid makes sense. Therefore, the dialog could check whether or not the provider is able to deliver free/busy information and show/hide the grid accordingly. Basically, for event living anywhere else but on a sun calendar server the free/busy grid would be hidden, users would just be presented with the list of attendee-names. What I find particular great with this approach is that the dialog dynamically changes with the features a provider supports. In the long term we need such "capabilities" for providers in other areas as well (this is a wild guess, of course). Any comments on this?
(Reporter)

Comment 12

11 years ago
For now, setting an attribute on the provider would work. But, in theory, you can query for freebusy information by email. That's not tied to a provider. You don't need access to the other person's calendar for that. So, if we ever support that, we need to rethink the attribute and the UI. But that's food for later :)

Comment 13

11 years ago
(In reply to comment #12)
> For now, setting an attribute on the provider would work. But, in theory, you
> can query for freebusy information by email. That's not tied to a provider. You
> don't need access to the other person's calendar for that. So, if we ever
> support that, we need to rethink the attribute and the UI. But that's food for
> later :)
> 
That's entirely true.  It's done via iTIP/iMIP. It's so far down my list I didn't think of it. :-S

Christian: with regard to the drop down menu system on the dialog: I should have also said that I'm a fairly new Mac user, and I am still getting accustomed to that landscape.  One thing I would hate to lose is the uncluttered nature of the dialog, so if we have to have the drop down menu list to prevent clutter then so be it.  There is some amount of precedent here with the Tbird compose window and several dialogs in Outlook use drop-down menus. It would be interesting to know whether or not the Outlook folks chose to use drop-down menus in their dialogs on their Mac port (I'd bet they did, given how much content is there).  

Comment 14

11 years ago
Due to the contention about adding this dialog, I'm marking this as a blocking bug so that we complete this review and address the comments before ship of 0.7.
Flags: blocking-calendar0.7+
(Assignee)

Comment 15

11 years ago
In order to get this resolved as quickly as possible, I'm going to file separate bugs that block this one. That way we can concentrate on single issues instead of trying to resolve everything in one large bug.
(Assignee)

Updated

11 years ago
Depends on: 389535
(Assignee)

Updated

11 years ago
Depends on: 389536
(Assignee)

Updated

11 years ago
No longer blocks: 293195
(Assignee)

Updated

11 years ago
Depends on: 389540
(Assignee)

Updated

10 years ago
Depends on: 391082
(Assignee)

Comment 16

10 years ago
Created attachment 276097 [details] [diff] [review]
UI review - patch v1

I'm going to separate the UI related stuff from the actual code review stuff by driving two distinct patches. This patch addresses the UI related bits and pieces.

Christian, this patch basically removes the progress bar from the dialog, nothing else. If there anything else that should be integrated in this patch in order to get a final decision on the UI for the event dialog? What bogs me most is the still unanswered question regarding the drop down menu for the dialog. Christian, please go through the comments from mvl and clint and provide us with a final decision on them.

Regarding the free/busy grid which shouldn't take up that much space in case we're scheduling an event on a calendar for which the provider doesn't support free/busy information, I already filed bug 387997. I would like to shift the discussion to this bug. This bug also waits on input from you, and it also depends on API changes from Daniel, see Bug 370148.

The other open questions can be summarized as follows:
- remove privacy button from default set?
- remove progress bar?
- remove separator between start/end and repeat?
- menu bar concerns on mac?
- ask if changes should be saved?
- split 'find free spot' and 'invite sombody'?
- custom alarm label?
- alarm choices are too many?
- status bar panel needs more spacing?
Attachment #276097 - Flags: ui-review?(christian.jansen)
Attachment #276097 - Flags: review?(philipp)
(Assignee)

Comment 17

10 years ago
Regarding the code review comments, I'm going to post a patch ASAP. As I already addressed a lot of this stuff in other patches I just would like to summarize what has been done elsewhere and what's going to end up in this one.

Bug 391082 - use customize-toolbar dialog from toolkit for event dialog

This was a valid request from Clint from his comments. It removes the duplicated code for the customize-toolbar dialog. It also makes the customization work in Sunbird.

Bug 389540 – eliminate superfluous bindings in recurrence dialog implementation

This was also a valid request from mvl and Clint. This addresses the removal of bindings related to the recurrence part of the dialog.

Bug 389536 – address style nits in all files located under prototypes/wcap

This one already landed at the time of this writing and addresses all style nit stuff in those files.

Bug 389535 – consolidate implementations for minimonth control

This was a request from mvl and combines the duplicated code of the minimonth control.

There were no comments from mvl that weren't also mentioned during Clints review or weren't addressed by above mentioned other bugs. So I'm going to concentrate on what Clint wrote for the upcoming patch. Please correct me if I'm wrong.
Comment on attachment 276097 [details] [diff] [review]
UI review - patch v1

looks fine, r=philipp
Attachment #276097 - Flags: review?(philipp) → review+

Comment 19

10 years ago
(In reply to comment #17)
*******************UI-Review**********************************

General:
We've received tons of positive feedback on the Proto Event dialog.
This UI-Review will focus on the open questions Mickey summarized in Comment #16 Plus some new issues. All new issues will be handled in separate bugs.

- remove privacy button from default set?
No, we should not do that. This is a frequently used feature. Especially by users who have stored their calendar on a calendar server. I know, in comment #5 I have decided to remove it but feedback showed me that this is an important feature which needs to be prominent.  

- remove progress bar?

Remove it. -> ui-r=christian for the attached patch

- remove separator between start/end and repeat?

This one should stay there. Removing the separator would cause unnecessary clutter, if a custom rule spans over 2 or 3 rows.

- menu bar concerns on mac?

We go with the menu. I am aware that this decision is not supported by everyone, but momentarily the menu gives us the flexibility we need to "hide" features which are not frequently used.

This prevents the dialog of being cluttered with 3rd priority but "must have features" like "Show time as". Another plus is that the Event Dialog is consistent with the Mail Compose - and the Address Book Window. Both Windows provide menus, even on Mac.

- ask if changes should be saved?
See Bug 385183

- split 'find free spot' and 'invite sombody'?

I'll comment on that in Bug 387997

- custom alarm label?
Change it to: Edit Custom Reminders...

- alarm choices are too many?
I filed bug 391673 on that 

- status bar panel needs more spacing?
should be fixed by removing the progress bar

Comment 20

10 years ago
Comment on attachment 276097 [details] [diff] [review]
UI review - patch v1

ui-r=Christian
Attachment #276097 - Flags: ui-review?(christian.jansen) → ui-review+
(Assignee)

Comment 21

10 years ago
> Egad, that is a big list of globals.
Clint, could you please explain why it's better to put these globals into an object and use getters/setters? It doesn't immediately make sense to me, so does it buying us?

> ...we clone the object since foreign X-props get lost during the roundtrip...
> What's the Bug number on this?
See Bug 358498 - Daniels first comment. The patch is already attached and fixes the problem, but it seems to be stuck in translation...

> What's the use of a and b here?
> We probably also want to make this a function on calUtils.js
> something like: function hasItemChanged(newItem, oldItem)
'a' and 'b' were just variables for debugging purposes. Unfortunately there were still several issues with the 'do you want to save?' dialog, respectively with finding out whether or not the item has been changed. There's a fix in the pipeline, see bug 385183.

> There are ways to reduce some duplicated code if you condense and factor
> out the similiar functionality in updateDueDate, updateEndTime,
> updateStartTime, loadDateTime etc.
Right, I'm going to address this in the upcoming patch.

> in loadRepeat, why can't you condense some of these if statements?
This has already been addressed with the style nit patch, see Bug 389536.

> on daily, this part doesn't make any sense at first glance...
>         for (var i = 0; i < 5; i++)
>           if (ruleComp[i] != i + 2)
>             break;
>         if (i==5)
>            recurrence on weekdays
This is just the way to detect whether or not the BYDAY array contains the set [2,3,4,5,6], which means 'recurrence every weekday'. I could also have used an array and compare them, but I found by using this for-loop I saved constructing the array.

> function checkRecurrenceRule(rule,array)
> Do we really need this function?
First, this function will move to /c/b/content/calendar-dialog-utils.js when Bug 361977 lands. Yes, this function serves the purpose to detect whether a given array of BY-rules are present in a recurrence rule. And yes, we need it. Assume we're faced with an ICS from any foreign source that contains some exotic recurrence rule. The dialog detects this case and simply gives up on displaying a natural language version of the pattern.

> What happens to that endTime.day+= 1 if your end time is not on the same
> day as your start time?
This is perfectly legal as this is how all day events are defined. Unfortunately there's an inconsistency between all day events and others. But that's not a problem of the dialog. Or did I misunderstand anything here? I can't find anything wrong with the code and it works as intended for all day events that span more than a single day. Btw, the attendee dialog works just the same.

> Why do you reset the endTimezone to the startTimezone if the endTimezone
> is UTC?
This works as intended, but is a bit involved. In this particular function I detect if start- and end-time span a valid timespan. The span is illegal in case  the end-time is before the start-time. Unfortunately, comparing dates doesn't work correctly if the dates live in different timezones, i.e. one of them is UTC. That's why I'm converting both times into the same timezone. Furthermore, this is temporarily inside this function and just to make comparing the dates work.

> function openNewMessage()
> function openNewCardDialog()
> This is not going to work on sunbird
The corrosponding menu entry is class "lightning-only" which makes it disappear on Sunbird. Furthermore, I addressed such issues in Bug 371917, which has already landed.

> function editURL()
> In the URL field, I really think you should ghost in the proper text...
That's a good idea, I'm going to address this in the upcoming patch.

> function onAboutDialog()
> Why are we opening the Thunderbird about dialog?
Since it's also accessible from the mail compose window, consistency reasons.

> OpenRegionURL
> This will break on sunbird
This has been moved to 'applicationUtil.js' with bug 371917.

> editStartTimezone
> Put a space above the function declaration it's hard to see.
addressed in style nit patch -> Bug 389536.

> Combine the edit*Timezone functions they are very similar
> and can be parametrized
Right, I'm going to address this in the upcoming patch.

> updateAttendees is completely unreadable
I hope this has been addressed in Bug 389536 to your liking. Please let me know if there are any issues left.

> updateRepeatDetails
> You have to if you're going to replace the current event dialog
The comment was regarding displaying the natural language form of the repeat pattern and not with respect to repeating tasks. But anyway, I'll address this in the patch.

> updateRepeatDetails
> A good portion of this function is basically translating recurrence
> information into "human readable recurrence rules" such as "every weekday".
> I think this should be moved into our recurrence interface.
Bug 361977 moves this function to /c/b/content/calendar-dialog-utils.js since it's also used in the summary dialog. But basically you're right, I'll ask Daniel if we want to add this to the recurrence interface or move it the ...-utils.js. Good point.

> It needs to be better structured so that the business logic code and the
> UI controller code is a bit better separated.
I know that everything is a bit tied-together, but what's the concrete suggestion?

> sun-messenger-overlay-sidebar.xul
> Obviously, this needs to be merged into
> lightning/content/messenger-sidebar-overlay.xul
This isn't part of the dialog as it just adds the invitations manager into Lightning by providing the 'invitations link'. This is some other story.

> I should declare that your timezone picker is my favorite part of the entire
> event dialog.  So, now you know my bias. :-)
> I want you to remove the "/mozilla.org/<timestamp>/" header from the
> "value" of the menuitems.  Instead, please prefix the value with the
> calIICSService::tzidPrefix attribute when you read the value in JS.
> That way, you will avoid hardcoding all these TZID's and you will not
> have to change this file every time a TZID definition changes.
Are you ok with addressing all the timezone picker stuff in a separate bug? I would like to convert it to a new control and use it in the preferences as well. The relevant patch should then address what you're suggesting here, ok?

> sun-calendar-event-dialog-recurrence.xml
> My first thought was that this was very akin to the updateRepeatDetails...
This function is similar to loadDialog() in the old calendar-recurrence-dialog.js. It just inspects the rules in order to initialize the controls just as the old dialog did. It has nothing to do with the natural language stuff in updateRepeatDetails(). At first glance I don't have a clever idea how to factor this into the recurrence interface and save a ton of code.

> looks like the onSave function is exactly the same as the
> calendar-recurrence-dialog.js
Yes, right. I think this will be addressed when the new dialog finally replaces the old one.

> sun-calendar-event-dialog-recurrence.js
> Why can't this logic be in the .xml file?
See Bug 389540.

> s-c-e-d-recurrence-preview.xml
> I would love to see this code merged into our standard minimonth
> control if possible
See Bug 389535.

> s-c-e-d-recurrence-datepicker.xml
> Does this have to be called "datepicker"?  Could we call it "daypicker"?
That's a good point, I'll address this in the patch.

> s-c-e-d-freebusy.xml
> change this to "infrastructure hasn't created the pref yet
Will be contained in the patch.

> <method name="onFreeBusy">
> This method uses the default timezone of the calendar for freebusy
> scheduling.  I think this is incorrect.
Good point, I'll add this to the patch.

> query for the wcap interface, the appropriate functionality...
> Can we go ahead and generalize this?
Daniel is taking care of this one, see bug 370148.

> s-c-e-d-attendees.xul
> The id on this dialog says "recurrence" it should probably be attendees.
Right...

> <xul:image id="attendeeCol1#1" anonid="icon"/>
> That id value is legal, of course, but I find it odd to
> have a # in an id value.
This is intentional as the lines get different number as they are cloned when more attendees get added to the listbox.

> Why do we need to update attendees every second?
Probably I didn't make myself clear in the comments. The free/busy information doesn't get refresh every second. Each line in the grid corresponds to some attendee name. If an attendee name gets changed (user types in a name), the line gets dirty and the 'continuous update chain' listens for this case and refreshes the free/busy information. But probably I can increase the delay...

> I think one of these statements is suficient, please remove the others
See style nit bug.

> If you are scheduling with FreeBusy support, then the organizer should show up in the list.
I does show up in the list.

> Please condense this to "// first row cloning problem" or some such.
Right, I'll add this to the patch.

> I think we should put a "type" attribute on our atttendee interface so
> that we can handle types of attendees id's for protocols other than mailto.
That's a good idea as I already have tons of locations where I need to handle a possibly present mailto-prefix. I think this is outside of the scope of this bug, so I'm going to file a new bug on this, ok?

> Here again, I think we should use the timezone that the event we
> are scheduling is *in*, not the default Calendar timezone.
Right, this will go into the patch.

> I think that it would be simpler to have a base attendee/freebusy page in
> XUL that lays out the XBL controls for the page.
This is perfectly valid, I'll add this to the patch.

> s-c-customize-toolbar.*
> Is there a toolkit widget/library that we can use for this?
See Bug 391082.
(Assignee)

Comment 22

10 years ago
Created attachment 276765 [details] [diff] [review]
code review - patch v1

This is the last piece of the puzzle, the last patch in order to get the review of the event dialog done. This patch contains the bits and pieces I said it will contain in my previous comment.

- updateStartTime() and updateEndTime() are not conndensed into dateTimeControls2State()
- updateEntryDate() and updateDueDate() are not factored into updateDateCheckboxes(). The names were indeed misleading.
- editStartTimezone() and editEndTimezone() are now combined into editTimezone()
- in the URL field I'm now ghosting in an example
- the repeat pattern is now correctly displayed in plain text for tasks
- renamed 'datepicker' into 'daypicker'
- corrected some comments
- removed 'attendee-page' binding in favor of plain xul

Regarding moving translation of recurrence information into 'human readable recurrence rules' and the idea to move this into out recurrence interface. I talked about this with Daniel and we came to the conclusion that it shouldn't move into the interface as it's not a part of the model. It really is part of the UI and should therefore live in a shared file that can be used where needed.  Bug 361977 moves this function to /c/b/content/calendar-dialog-utils.js. If there are no further objections I would like to take this road.

Regarding the comments that the information in the free/busy grid is displayed in the wrong timezone I'm unsure whether or not it's really true. I thought it's just right to always display the times in the default timezone (just as the views do), in order to stay consistent. Please correct me if this appear to be wrong, but I didn't change anything to this particular part of the dialog due to this reasoning.

Clint, I'm asking you for review since you brought up the largest part of the comments which also contained the necessary detail to address them. As far as I can tell I have no further outstanding pieces that hinder getting the final review for the event dialog.
Attachment #276765 - Flags: review?(ctalbert)

Comment 23

10 years ago
Created attachment 277550 [details]
Some review comments on sun-calendar-event-dialog-attendees.js

These are the first run comments from my work last night.  I think before going much farther, I'd like to get a patch that applies cleanly.  I'd like to run the dialog while I work through the review so that I don't end up asking a lot of stupid questions. The notes from the failure to apply the patch come from attempting to patch the MOZILLA_1_8_BRANCH tree checked out on August 21, 2007.
(Assignee)

Updated

10 years ago
Whiteboard: [patch in hand]
(Assignee)

Comment 24

10 years ago
Created attachment 277877 [details] [diff] [review]
code review - patch v2

This is an updated version of the patch that cleanly applies (again).

> It seems that startHourDefault, endHourDefault globals should be controlled by preferences.
I introduced these values when Thomas landed his 24 hours patch, which made the relevant preference values vanish (calendar.view.defaultstarthour, calendar.view.defaultendhour). It wasn't clear when the new preferences will come up and what their names will be. So I decided to do what is today available with getPrefSafe(). I changed the code to take that into account and also read from the new preference names. This code is already a year old...

> It seems that DisplayTimezone global should be controlled by preferences.
No, in this case not. This flag decides about whether or not the timezone information will be displayed next to the start/endtime. This information is made persistent with the relevant menuitem. Furthermore, this gives me a chance to explain why we need the g*Date variables at all. They contain the dates in the calendar default timezone, while the (original) timezone of these dates is contained in g*Timezone. If the dialog should display the times with default timezone it displays the g*Date variables. If the dialog should display the times in their respective timezone, it uses g*Timezone to get the relevant information from.

> Also, can you detail what "up to date" means.
I tried to elaborate a bit on that...

> This error is not localizable.  It needs to be in a properties file somewhere.
Already done, see bug 377620.
Attachment #276765 - Attachment is obsolete: true
Attachment #277877 - Flags: review?(ctalbert)
Attachment #276765 - Flags: review?(ctalbert)

Comment 25

10 years ago
There's no option to set status of the event in prototype (I mean confirmed, tentative, etc.). It's impossible to change it.

Comment 26

10 years ago
Just filed a bug for this: bug 394149
(Assignee)

Comment 27

10 years ago
Clint, meanwhile all dependent bugs have been resolved. Any chance that you get to the review anytime soon?

Comment 28

10 years ago
Mickey, I've finished the review.  I got the code working and played with it.  I found some minor issues here and there (and one major) but overall the code seems good.  I went through the patch, and also hit a few of the other files in the event dialog as I needed to re-review my old comments and refresh myself with the code.  So, you'll see a few comments on files that weren't specifically in the patch as well as comments about items in the patch.

I just have a few questions before granting the review, and a couple of nits. I also cataloged all the bugs that I encountered along the way.  I'll be going through this list of bugs and filing any that aren't already filed tomorrow.

== Possibly known bugs found During Review ==
* Attendee icon in the F/B pane should have a tooltip describing what it is.
* Clicking the lock itself on the toolbar of event dialog should set the event
to "private" mode.  Needing to click the pull down menu to set any type of privacy is a little cumbersome and not user friendly
* How does one turn on support for timezones for both the start AND end times?
You can't do it through the Options->Timezones setting on the event dialog.
* -- MAJOR -- Attempting to change the timezone results in an error:
Error: window.onAcceptCallback is not a function Source File: chrome://calendar/content/sun-calendar-event-dialog-timezone.js
Line: 124
* In the recurrence dialog, the preview is not updating as I change the recurrence pattern.
* Recurrence dialog should also seed itself with the current data.  For example,
I first click on a recurrence pattern of yearly, and then I click on "custom".
The recurrence dialog defaults to "Every 1st of January".  It should at least make the assumption that I want to use my date settings from the event dialog and default to "every 5th of September".  The same holds true for month, and week repetition (day of month and day of week should be the defaults there).
** This also happens if you select "custom" recurrence type as the first recurrence selection you make, so it doesn't even matter if you had a previous setting.
* Are print and page setup really valid options in the File drop down menu for the event dialog?  If we *do* have a valid reason for those, then how do you enable the "print" element of the menu?  It seems permanently disabled.
** Additionally, if print is disabled, then "page setup" should probably also be
disabled. 
* When in Task mode, the Privacy drop down should probably say "Public Task,
Private Task" instead of "Private Event", "Public Event".
* About the privacy setting, isn't an event/task with no privacy attribute
considered public?  Shouldn't we then always start out by displaying everything
as a "public event"? (i can't remember the spec's position on this one)

= Comments on the Patch = 

== s-c-event-dialog.js ==
Line 88: Why do you need getString?  It seems you can use calUtils.js just fine
(it is used for createRecurrenceRule).  And this code is exactly identical to
calGetString in calUtils.js.

dateTimeControls2State
I like this function.  But I have one question.  At the end here:
    if (warning) {
        var callback = function func() {
            var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"]
                                .getService(Ci.nsIPromptService);
            promptService.alert(
                null,
                document.title,
                calGetString("calendar", "warningNegativeDuration"));
        }
        setTimeout(callback, 1);
    }
Why are we doing this setTimeout(callback, 1) business?  Why not just use
promptService.alert directly?

== s-c-event-dialog-timezone.js ==
License header still says original code is Oracle code.  Is that right?

== s-c-e-d-reminder.js ==
Still says Oracle as original creator of code, is that correct?

== s-c-e-d-attendees.js ==
+/**
+ * assumes that gStartDate and gEndDate have been correctly initialized,
+ * either by having called loadDateTime() or having read the information
+ * from the controls due to recent user input. gStartDate and gEndDate are
+ * assumed to always be in the default timezone while the actual timezones
+ * are expected at gStartTimezone and gEndTimezone. this function attempts
+ * to copy these state related informations to the dialog controls, which
+ * makes it read from the variables and write to the controls (i.e. it
+ * doesn't change the state).
+ */
+function updateDateTime() {
Is there any usefulness in attempting to detect if gStartDate and gEndDate have
been initialized?  Perhaps we should try to detect an uninitialized state and
throw an error? Or if they are uninitialized, can we initialize them from the widget controls at the beginning of this function?

+function timezoneString(date) {
+    var fragments = date.split('/');
+    var num = fragments.length;
+    if (num <= 1) {
+        return fragments[0];
+    }
+    return fragments[num - 2] + '/' + fragments[num - 1];
+}
 -- MAJOR -- This type of string will break if you try to convert back to the full TZID for "3 slash" TZIDs like:
/mozilla.org/20070129_1/America/Argentina/Buenos_Aires
This will be handled by the calITimezoneResolver patch in the future, but in the
meantime, I think if you convert this back all folks in one of these 3 slash zones will lose their TZID when you convert back to the full TZID by prepending the "mozilla" prefix.

== s-c-d-e-freebusy.xml ==
line 48: 
<handlers>
      <handler event="click" action="alert('mickey');"/>
    </handlers>
Debug code? ;-) Please remove.

== s-c-d-e-recurrence.js ==
@@ -186,18 +186,18 @@ function onSave(item) {
     if (recurrenceInfo) {
         recurrenceInfo = recurrenceInfo.clone();
         var rrules = splitRecurrenceRules(recurrenceInfo);
         if (rrules[0].length > 0) {
             recurrenceInfo.deleteRecurrenceItem(rrules[0][0]);
         }
     } else {
         recurrenceInfo = createRecurrenceInfo();
-        recurrenceInfo.item = item;
     }
+    recurrenceInfo.item = item;
What happens if item is null?  I see at the top of onSave the lines:
    if (item.parentItem != item) {
        return null;
    }
If item comes in as null, I think item.parentItem will throw, saving us from further execution in the function.  Is that the desired behavior or should we add the null check to this if statement too?

(In reply to comment #28)
Thanks Clint for spending so much time on the review. IMO some of your mentioned issues should be handled in spin-off bugs, so we get the overall code review done in this bug soon, i.e.

> * Attendee icon in the F/B pane should have a tooltip describing what it is.
> * Clicking the lock itself on the toolbar of event dialog should set the event
> to "private" mode.  Needing to click the pull down menu to set any type of
> privacy is a little cumbersome and not user friendly
> * How does one turn on support for timezones for both the start AND end times?
> You can't do it through the Options->Timezones setting on the event dialog.
...
> * Recurrence dialog should also seed itself with the current data.  For
> example,
> I first click on a recurrence pattern of yearly, and then I click on "custom".
> The recurrence dialog defaults to "Every 1st of January".  It should at least
> make the assumption that I want to use my date settings from the event dialog
> and default to "every 5th of September".  The same holds true for month, and
> week repetition (day of month and day of week should be the defaults there).
> ** This also happens if you select "custom" recurrence type as the first
> recurrence selection you make, so it doesn't even matter if you had a previous
> setting.
> * Are print and page setup really valid options in the File drop down menu for
> the event dialog?  If we *do* have a valid reason for those, then how do you
> enable the "print" element of the menu?  It seems permanently disabled.
> ** Additionally, if print is disabled, then "page setup" should probably also
> be
> disabled. 
> * When in Task mode, the Privacy drop down should probably say "Public Task,
> Private Task" instead of "Private Event", "Public Event".
> * About the privacy setting, isn't an event/task with no privacy attribute
> considered public?  Shouldn't we then always start out by displaying everything
> as a "public event"? (i can't remember the spec's position on this one)

Comment 30

10 years ago
(In reply to comment #29)
> (In reply to comment #28)
> Thanks Clint for spending so much time on the review. IMO some of your
> mentioned issues should be handled in spin-off bugs, so we get the overall code
> review done in this bug soon, i.e.

Yes definitely.  I have found or filed bugs for these items:
> 
> > * Attendee icon in the F/B pane should have a tooltip describing what it is.
This is now bug 395281

> > * Clicking the lock itself on the toolbar of event dialog should set the event
> > to "private" mode.  Needing to click the pull down menu to set any type of
> > privacy is a little cumbersome and not user friendly
This is now bug 395283

> > * How does one turn on support for timezones for both the start AND end times?
This is now bug 395287

> > * Recurrence dialog should also seed itself with the current data.  For
This is bug 394010

> > * Are print and page setup really valid options in the File drop down menu for
> > the event dialog?  If we *do* have a valid reason for those, then how do you
> > enable the "print" element of the menu?  It seems permanently disabled.
> > ** Additionally, if print is disabled, then "page setup" should probably also
> > be
> > disabled. 
Could this be bug 394358?


> > * When in Task mode, the Privacy drop down should probably say "Public Task,
> > Private Task" instead of "Private Event", "Public Event".
> > * About the privacy setting, isn't an event/task with no privacy attribute
> > considered public?  Shouldn't we then always start out by displaying everything
> > as a "public event"? (i can't remember the spec's position on this one)
This is bug 395283
 
> > * -- MAJOR -- Attempting to change the timezone results in an error:
> > Error: window.onAcceptCallback is not a function Source File:
> > chrome://calendar/content/sun-calendar-event-dialog-timezone.js
> > Line: 124
I cannot reproduce this issue with a current nightly. It might have been an error with the patch.

Comment 31

10 years ago
> > * -- MAJOR -- Attempting to change the timezone results in an error:
> > Error: window.onAcceptCallback is not a function Source File:
> > chrome://calendar/content/sun-calendar-event-dialog-timezone.js
> > Line: 124
> I cannot reproduce this issue with a current nightly. It might have been an
error with the patch.

Might have been an error with the patch application or with the patch itself. Needs further investigation.

(Sorry hit submit too quickly)

Comment 32

10 years ago
Mickey, I believe we have the "bugs" issue addressed with the post above.  I'd like to hear your responses on the nits and small issues mentioned in Comment 28.

(Assignee)

Comment 33

10 years ago
Created attachment 280467 [details] [diff] [review]
code review - patch v3

Clint, thanks again for going through all this and sorry for the delay, my bug pipeline is still not getting any shorter...

> Line 88: Why do you need getString?  It seems you can use calUtils.js just
> fine (it is used for createRecurrenceRule).  And this code is exactly
> identical to calGetString in calUtils.js.
Right, addressed in this patch.

> Why are we doing this setTimeout(callback, 1) business?  Why not just use
> promptService.alert directly?
In case this warning gets triggered the code sets both controls that in turn triggers this warning once more. The setTimeout()-trick shields the function against this in order to achieve that the message box pops up only once. There are probably other ways to achieve this, but I found this appealing. Otherwise we would need to introduce some sort of flag or some such.

> Is there any usefulness in attempting to detect if gStartDate and gEndDate
> have been initialized?  Perhaps we should try to detect an uninitialized
> state and throw an error? Or if they are uninitialized, can we initialize
> them from the widget controls at the beginning of this function?
updateDateTime() is just an internal function (only called from other public functions). I think it's quite common to assume some preconditions for such functions.

> MAJOR -- This type of string will break if you try to convert back to the
> full TZID for "3 slash" TZIDs
This strings is used for display purposes only, the timezone itself is always handled by its full specification. Even if this function does anything wrong, we don't loose any date, the display would be just a bit wrong. Admittedly, in case we are facing a '3 slash' timezone this function wouldn't display the correct result, but we also have the problem that those strings won't be localized. Since this is already filed as bug 393360 I suggest to handle the '3 slash' part in this bug as well, if that's ok for you.

> If item comes in as null, I think item.parentItem will throw, saving us from
> further execution in the function.  Is that the desired behavior or should we
> add the null check to this if statement too?
I added the necessary check at the top of this function.

I addressed all the rest of your comments, and thanks again for filing all those spin-off bugs...
Attachment #277877 - Attachment is obsolete: true
Attachment #280467 - Flags: review?(ctalbert)
Attachment #277877 - Flags: review?(ctalbert)

Comment 34

10 years ago
Comment on attachment 280467 [details] [diff] [review]
code review - patch v3

Thanks for the new patch and the answers to the questions. This patch looks good.
r=ctalbert
Attachment #280467 - Flags: review?(ctalbert) → review+
(Assignee)

Comment 35

10 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 36

10 years ago
Cheched in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.