392021 - Event dialog: Option to delete event/task

Status: VERIFIED FIXED

(Calendar :: Dialogs, enhancement)

Description

[Roman Brandl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: 

when you open a detail dialog for an event or task, you should be able to delete this very event/task from within this dialog, e.g. a delete item in the menu and/or toolbar

Reproducible: Always

Steps to Reproduce:
1.open an event or task in lightning
2.in the dialog that opens there is not action to delete this item

Comment 1

[Martin Schröder [:mschroeder]]

Cc'ing Christian. Is this something a user expects from the event dialog and an option we should offer?

Comment 2

[Christian Jansen]

From my point of view this RFE makes totally sense. We should add a command to the toolbar & to the menu.

Comment 3

[Martin Schröder [:mschroeder]]

Confirming per comment#2.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Oops, sorry, my bad, returning to the right component, awfully sorry about stomping your target milestone, please send hatemail to this address :(

Comment 5

[Christian Jansen]

This request is also valid for the "read only" event dialog. From an user experience standpoint this is an important enhancement. It would be great to have this in .9

Comment 6

[Daniel Boelzle [:dbo]]

- So you think of a menu item like "Delete And Close"?
- How could we delete in the read-only case? Or do you think of the invitation scenario, i.e. deleting the invitation copy?

Comment 8

[Stefan Sitter [:ssitter]]

(In reply to comment #6)
> - So you think of a menu item like "Delete And Close"?

I suggest something similar to the Thunderbird message window:
- A menu entry "Edit -> Delete"
- A toolbar button "Delete" with tooltip "Delete selected event or task"
Choosing the delete command automatically closes the dialog.

> - How could we delete in the read-only case? Or do you think
>   of the invitation scenario, i.e. deleting the invitation copy?

In the read-only case the delete options should be either disabled or don't shown at all.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #8)
> - A menu entry "Edit -> Delete"

That menu entry would delete the current selection. In the event dialog, that means the selected text. You want "File -> Delete"

Comment 10

[Roman Brandl]

(In reply to comment #9)
> That menu entry would delete the current selection. In the event dialog, that
> means the selected text. You want "File -> Delete"

I second that, it should be in file, not edit.

Comment 11

[Pete Riley]

I agree that the delete command would be better in the File menu, but I also think it's wrong to have a File menu in the event dialog -- it's confusing and IMO it would be better to call it the Event menu or something.

I also think that "Print Setup", "Print", and "Exit" should be removed from this menu (unless this print command only prints this event).

Comment 12

[Christian Jansen]

(In reply to comment #11)
> I agree that the delete command would be better in the File menu, but I also
> think it's wrong to have a File menu in the event dialog -- it's confusing and
> IMO it would be better to call it the Event menu or something.

+1 for "File -> Delete"

New
------
Save
Delete Event
------
Close

Plus a Toolbar Button.

Order: Save, Invite, Privacy, Add, Delete

Tooltip: Delete this Event



> 
> I also think that "Print Setup", "Print", and "Exit" should be removed from
> this menu (unless this print command only prints this event).
> 

This makes also sense. I had the hope that we'll have Print for single events or tasks someday.

Comment 13

[Roman Brandl]

(In reply to comment #12)
Come to think of it, an EVENT Menu would make a lot of sense, not instead of FILE but in addition to it.  Application wide commands (like QUIT) should go to FILE but those scoped only to the EVENT (like DELETE, PRINT, ...) should go to EVENT.
just my 0.02$

in case this can/should not be done:
+1 for File->Delete
+1 for Toolbar button
as christian suggested in 

Comment 14

[F. G.]

Since nobody seems to have picked this up for 0.9, I would like to back up this request.
As for UI design, I think (meaning that I can't actually *quote* something), that a "File"-like menu is a widely used convention, where the actual Word "File" should be modified to reflect the item type being shown, i. e. "Event" in this case.  The German translation uses the subobtimal "Datei" (="File") for all these Menus.
The Print and Print setup entries are placed correctly in this Menu.
For clarity, the delete entry should be labelled "Delete Event...", with a confirmation dialog.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

This bug should probably be morphed to clean up the file menu (which may include renaming it). I'll leave that up to the implementer. This should be an easy bug to fix.

Comment 16

[Markus Adrario [:Taraman]]

My vote is for renaming to "event"-menu.

I will have a go on this one and prepare a dummy as preview.

Comment 17

[Markus Adrario [:Taraman]]

Attachment: Event Dialog Mockup V1

This incorporates most of comment #12 from Christian

I left out the "new" Popup in the Event-Menu, because I think when editing an Item, I won't need to create a new <anything> from within this dialog. It would simply lead me to editing 2 things at once which isn't favourable. If I absolutely wanted to do that, I can just click on new <anything> in the main window.

Pls comment on the mockup.

Comment 18

[Martin Schröder [:mschroeder]]

What is 'Edit > Select All' for? I also think we should drop the 'Help' menu! More to come...

Comment 19

[Roman Brandl]

Markus,
the mockup is for events only (but I like it)
for tasks the menu should then be called "Task" I guess and also include an item "Mark as Completed"... shouldn't it?

Comment 20

[F. G.]

Markus, thanks for picking this up.
The Event menu now lacks "Page Setup..." and "Print", which are present in 3.0.5.  The "New >" submenu is also gone, but this would be a bit confusing now: Event>New would be able to create new Tasks, for example.  But most inelligent life forms will cope.
Martin, "Edit > Select All" selects all text in a single input field, and it occurs rather frequently, if only to put the ^-a shortcut into the keymap.
Menu Entries that require further user input, such as "Invite Attendees" should be terminated with an ellipsis character ("...").
Stretching the topic of this bug, I would contend the placement of "invite Attendees" and the likes in the Options Menu.  An attendee is not an option of an event, but a rather central piece of content.  The only entry in this menu that I would classify as an option would be privacy.  The most simple fix for this would be to rename the menu to something like "Details" (another bad name, but it is at least not taken).  The real fix would be, IMHO, to move almost all this information to the dialog itself.
The "Help" Menu should never ever be dropped.  If a user is lost, that may be his last straw.

As for references, I have checked with http://developer.apple.com/documentation/userexperience/Conceptual/AppleHIGuidelines/XHIGIntro/XHIGIntro.html.

Comment 21

[Markus Adrario [:Taraman]]

(In reply to comment #19)
> the mockup is for events only (but I like it)
> for tasks the menu should then be called "Task" I guess and also include an
> item "Mark as Completed"... shouldn't it?

Forgot to mention...
Yes, I could have prepared another mockup for Tasks, but as its only a first look at it, I decided one can imagine the other Menu-Title in case of a task (which you did... ;-) )
The mark completed item is a good hint. Also needed in my opinion.

The other note is that the mockup is the Lightning Dialog. In sunbird it would lack some of the entries.

(In reply to comment #20)
> The Event menu now lacks "Page Setup..." and "Print", which are present in
> 3.0.5.
I deleted these because of comment #11 & comment #12.

> The "New >" submenu is also gone, but this would be a bit confusing
> now: Event>New would be able to create new Tasks, for example.  But most
> inelligent life forms will cope.
See my comment for the attachment...

Comment 22

[F. G.]

For printing, I then cross-reference bug 325137 - if that is fixed (for events and tasks), the respective menus should be expanded.  For the topic at hand, the menu entries were rightly removed.
I would still keep the "New >" submenu, in order not to constrain users working habits.  The application also makes a more coherent impression if keyboard shortcuts remain the same.

Comment 23

[Martin Schröder [:mschroeder]]

(In reply to comment #20)
> The "Help" Menu should never ever be dropped.  If a user is lost, that may be
> his last straw.

In this case an effectively broken last straw. Reading your comment, I doubt you have looked at the 'Help' menu. There is no real value in having the menu items in the 'Help' menu as they are now. ;-)

Comment 24

[Markus Adrario [:Taraman]]

So I try a short recap:
Facts:
- The File Menu should be renamed to "Event" or "Task" depending on the item.
- A "Delete..." Entry should be added - and also a button to the toolbar. I wouldn't call it "Delete Event/Task..." because it's already in the "Event/Task" Menu.

Things that need a decision:
Should
- we have a "mark completed" option in the task-menu? IMHO it's not necessary because this is already in the dialog.
- the Help menu be deleted (maybe until a real help is available)?
- the "new" submenu be kept?
- the printing items be kept for now or removed until printing is available?
- Is the options Menu named correctly or should the name be changed / some items moved to other menus or the Dialog?

Comment 25

[Markus Adrario [:Taraman]]

Attachment: Delete Button & MenuItem V0.8

This is a progress update on this Bug to get some feedback. It only incorporates the Delete Button & Menu Item.

Known problems with this patch:
- Delete does not work for single occurrences within a recurring event yet. I'm still tryin' to figure out how to do this. If anyone can give me a hint on that please mail me.

- The icon for "delete" in the "pinstripe" theme is not what I would have expected it to look like. Since I never worked with a Mac -sigh- I don't know if this is the delete Icon there.
Maybe someone could check that.

Notes:
1.) I used the "mode"-argument which is passed to the dialog to evaluate if the item is new and not "isMutable".
Reason is that "isMutable" can be changed and is not limited to a new item and the mode is easier to understand when reading the code.

2.) I'm not sure if the code should check if the Item is really deleted or if I can rely on the delete function.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #25)
> Created an attachment (id=380933) [details]
> Delete Button & MenuItem V0.8
> 
> This is a progress update on this Bug to get some feedback. It only
> incorporates the Delete Button & Menu Item.
> 
> Known problems with this patch:
> - Delete does not work for single occurrences within a recurring event yet. I'm
> still tryin' to figure out how to do this. If anyone can give me a hint on that
> please mail me.
Something like this should do it:

let listener = {
  onOperationComplete: function() {
      // Probably need more than this...
      window.close();
  }
};

let newItem = item.parentItem.clone();
newItem.recurrenceInfo.removeOccurrenceAt(item.recurrenceId);

window.opener.doTransaction("modify", newItem, newItem.calendar, item.parentItem, listener);


> 
> - The icon for "delete" in the "pinstripe" theme is not what I would have
> expected it to look like. Since I never worked with a Mac -sigh- I don't know
> if this is the delete Icon there.
> Maybe someone could check that.
I don't have a mac either.

> Notes:
> 1.) I used the "mode"-argument which is passed to the dialog to evaluate if the
Totally fine, I believe.

> 
> 2.) I'm not sure if the code should check if the Item is really deleted or if I
> can rely on the delete function.
In this case, the best way to check if the function succeeded is to use the listener. Please see calICalendar.idl on documentation how the listener is called in case of a delete (line ~370), and also how the listener looks like (line ~583).

Comment 27

[Markus Adrario [:Taraman]]

Attachment: Delete Button & MenuItem V1

First Patch for this Bug which only adds the Delete Functionality
The change of the Menus will be done in a seperate patch.

For additional comments see comment #25

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 381753 [details] [diff] [review]
Delete Button & MenuItem V1


>+    if (document.documentElement.getAttribute("confirmCancel") == "true") {
Instead of using an element attribute, I'd go for either a window global or a function that holds this variable.



>+    // hide the elements that are not needed for a newly created item (e.g. delete)
>+    if (window.mode == "new") {
>+        let hideElements = document.getElementsByAttribute("hide-on-new-item", "true");
>+            for (let i = 0; i < hideElements.length; i++) {
>+                hideElements[i].setAttribute('hidden', 'true');
>+        }
>+    }
I don't know how I did it, but at some point I got the delete item buttons even on a new event.
Anyway, I think we should allow the delete button even for new events, and then just close the window instead of deleting the item if the user chooses to delete the item.


>+ *
>+ */
>+
>+function onCommandDeleteItem() {
Remove extra newline


>+    // only ask for confirmation, if the User changed anything on a new item or we modify an existing item
>+    if (isItemChanged() == true || window.mode != "new") {

I just found this now, but calendarViewController has a deleteOccurrences function that takes care of prompting and deleting automatically, using the standard dialogs. This will cause the user to be asked if he wants to delete this occurrence or the whole series. Not sure if this is wanted, I guess thats up to clarkbw.

Anyway, we might want to extract this function and put it somewhere common, maybe some javascript module. What do you think?

>+        } else if (cal.isTask(window.calendarItem)) {
Isn't the function called isToDo ? Please doublecheck

>+            } else {
>+                // do nothing
>+            }
Just remove the else block here.
ues in the Dialog have changed. False otherwise.

>+ */
>+
>+function isItemChanged() {
Remove extra newline.



>+        <key id="delete-key"
>+             modifiers="accel"
>+             key="D"
>+             command="cmd_item_delete"/>
I realize this isn't your fault, but we really need to make these keys localizable. Maybe you could file a followup bug and/or take care?



>+<!ENTITY  event.toolbar.delete.label                      "Delete Item">
Personally I'd prefer just "Delete" her, similar to the main toolbar. This makes the toolbar be less long.


r- for now to sort out issues. Looking forward to your next patch.

Comment 29

[Markus Adrario [:Taraman]]

Attachment: l10n part of the patch

I splitted of the l10n part, because I heard rumors there might be a string freeze soon. ;-)
so just in case...

I corrected the following from the last patch:
>> +<!ENTITY  event.toolbar.delete.label                      "Delete Item">
> Personally I'd prefer just "Delete" her, similar to the main toolbar. This
> makes the toolbar be less long.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 384410 [details] [diff] [review]
l10n part of the patch

(In reply to comment #28)
> >+<!ENTITY  event.toolbar.delete.label                      "Delete Item">
> Personally I'd prefer just "Delete" her, similar to the main toolbar. This
> makes the toolbar be less long.

agreed.

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #28)
> >+    // only ask for confirmation, if the User changed anything on a new item or we modify an existing item
> >+    if (isItemChanged() == true || window.mode != "new") {
> 
> I just found this now, but calendarViewController has a deleteOccurrences
> function that takes care of prompting and deleting automatically, using the
> standard dialogs. This will cause the user to be asked if he wants to delete
> this occurrence or the whole series. Not sure if this is wanted, I guess thats
> up to clarkbw.

if I understand this correctly that function would give us some consistency with other ways of deleting this event / occurrences of events so that sounds like a good option.

Comment 32

[Markus Adrario [:Taraman]]

I think, we shouldn't prompt for single occurence or whole series, because this is done on starting the edtit-dialog.
If we decided to edit a single instance on opening the dialog, why should we bother the user again?
IMHO its more consistent to just ask if the item should be deleted or not.

Comment 33

[Bryan Clark (DevTools PM) [:clarkbw]]

ugh, i keep forgetting that we ask about single instance / occurrence *before* you edit the event...  It would probably be a bad idea to ask that question again after we've already asked the question initially.

Comment 34

[Markus Adrario [:Taraman]]

Attachment: Delete Button & MenuItem V2 w/o l10n part

Corrected as per the Issues from Comment #28.

Except:
>>+    // only ask for confirmation, if the User changed anything on a new item or we modify an existing item
>>+    if (isItemChanged() == true || window.mode != "new") {
> I just found this now, but calendarViewController has a deleteOccurrences
> function that takes care of prompting and deleting automatically, using the
> standard dialogs. This will cause the user to be asked if he wants to delete
> this occurrence or the whole series. Not sure if this is wanted, I guess thats
> up to clarkbw.
Left as is after comments #31-33

>>+        <key id="delete-key"
>>+             modifiers="accel"
>>+             key="D"
>>+             command="cmd_item_delete"/>
>I realize this isn't your fault, but we really need to make these keys
>localizable. Maybe you could file a followup bug and/or take care?
Filed as Bug 500283, which I will take care of, when this one is finished.

Comment 35

[Roman Brandl]

actually I think we *should* re-prompt for single/series... once the dialog is open, the user has no clue whether this is the single event or the series. taking a drastic action IMHO allows to ask again...

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #35)
> actually I think we *should* re-prompt for single/series... once the dialog is
> open, the user has no clue whether this is the single event or the series.
> taking a drastic action IMHO allows to ask again...

Although I agree its not quite obvious, when the options for recurrence are disabled, you are editing a single occurrence of a series.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 384970 [details] [diff] [review]
 Delete Button & MenuItem V2 w/o l10n part

Looks good, r=philipp

I'm checking in the string part asap, I realize this is late, but I think its worth it.

Comment 38

[Markus Adrario [:Taraman]]

Two more thoughts on that matter:
1.: With the current delete function this can't be undone --> point for the above comment.

2.: The DeleteOccurences does not support a listener or return a value to make sure the operation succeeded. --> point for the current patch.

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

I went ahead and checked in both to make sure nothing goes wrong w.r.t missing entities. Sorry for taking away your checkin :-}

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/dbe3d9086bf3> and <http://hg.mozilla.org/comm-central/rev/0f609ed27df3>

-> FIXED

Comment 40

[Markus Adrario [:Taraman]]

no problem.
But this is not yet ebntirely fixed. There is still the question how the "file"-menu should be renamed and which options should be taken out or not.
See comment #24 for that.

Comment 41

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #24)
> Things that need a decision:
> Should

> - we have a "mark completed" option in the task-menu? IMHO it's not necessary
> because this is already in the dialog.
> - the Help menu be deleted (maybe until a real help is available)?
I agree

> - the "new" submenu be kept?
I think so.

> - the printing items be kept for now or removed until printing is available?
Unless this makes the menu look very small and useless, yes

> - Is the options Menu named correctly or should the name be changed / some
> items moved to other menus or the Dialog?
I think its fine.

Maybe Bryan wants to comment since they are UI issues though.

Aside from that, its up to clarkbw. Markus, the way things are checked in, do you think its in a state we can keep for beta1?

Comment 42

[Markus Adrario [:Taraman]]

We have 3 followup Bugs from this now:
Bug 500283, Bug 500397, Bug 500399

For the current patch:
The only flaw at the moment is, that there is no undo available when deleting an item from the dialog.
I had a short try with the deleteOccurrences Function:

let items = new Array();
items[0] = window.calendarItem.clone();
window.opener.calendarViewController.deleteOccurrences(items.length, items, false, false);

It works for deleting, but on undo, I get a MODIFICATION_FAILED error, although the deleted items are restored correctly.

Since we explicitly ask the user if he wants to delete I believe we could live without undo - at least for the beta.

Comment 43

[Markus Adrario [:Taraman]]

Attachment: Patch V2.1 - fix undo

I found the error.
when doing the deletion via the transaction manager, the undo works.

window.opener.doTransaction("delete", window.calendarItem,
                       window.calendarItem.calendar, null, deleteListener);

Comment 44

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 385119 [details] [diff] [review]
Patch V2.1 - fix undo

Yes, always use the transaction manager if you need undo/redo.

Comment 45

[Martin Schröder [:mschroeder]]

Comment on attachment 384970 [details] [diff] [review]
 Delete Button & MenuItem V2 w/o l10n part

Some (late) nits that should be fixed in a followup patch:

>+var gConfirmCancel = "true";

This should be a primitive boolean... var gConfirmCancel = true;


>+    if (isItemChanged() == false) {
>         return true;
>     }

No comparison needed... if (!isItemChanged()) {


>+    if (gConfirmCancel == "true") {
>+        var result = onCommandCancel();
>+        if (result == true) {
>+            dispose();
>+        }
>+        return result;
>+    } else {
>         dispose();
>+        return true;
>     }

Following shorter code should be semantically equivalent:

if (!gConfirmCancel || (gConfirmCancel && onCommandCancel())) {
    dispose();
    return true
}
return false;


>+function onCommandDeleteItem() {
>+    // only ask for confirmation, if the User changed anything on a new item or we modify an existing item
>+    if (isItemChanged() == true || window.mode != "new") {

if (isItemChanged() || window.mode != "new") {


>+        if (answerDelete == false) {
>+            return;
>+        }

if (!answerDelete) {


>+            onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+                if (aId == window.calendarItem.id && aStatus == 0) {
>+                    gConfirmCancel = "false";
>+                    document.documentElement.cancelDialog();
>+                }
>+            }

gConfirmCancel = false;


>+    } else {
>+        gConfirmCancel = "false";
>+        document.documentElement.cancelDialog();
>+    }
> }

gConfirmCancel = false;


>+    if ((newItem.calendar.id == oldItem.calendar.id) &&
>+        compareItemContent(newItem, oldItem)) {
>+        return false;
>+    }
>+    return true;

Semantically equivalent:
return (newItem.calendar.id != oldItem.calendar.id) || !compareItemContent(newItem, oldItem);

Comment 46

[Markus Adrario [:Taraman]]

Attachment: Patch V2.2 - fix undo + style cleanup from comment #45

Thanks for the hints Martin

>>+    if ((newItem.calendar.id == oldItem.calendar.id) &&
>>+        compareItemContent(newItem, oldItem)) {
>>+        return false;
>>+    }
>>+    return true;

>Semantically equivalent:
>return (newItem.calendar.id != oldItem.calendar.id) ||
>!compareItemContent(newItem, oldItem);

Right. I left this code as it is, since I just copied this into a new function.
I also left it for now, because its a lot better readable - although your version is more elegant.

-            window.calendarItem.calendar.deleteItem(window.calendarItem, deleteListener);
+            window.opener.doTransaction("delete", window.calendarItem, window.calendarItem.calendar,
+                                        null, deleteListener);
These lines are from patch 2.1 which inot yet checked in. So it is in here again.

Comment 47

[Martin Schröder [:mschroeder]]

Comment on attachment 385651 [details] [diff] [review]
Patch V2.2 - fix undo + style cleanup from comment #45

With javascript.options.script set to true, I get following warning in the Error Console twice when I undo deleting an event:

Warning: reference to undefined property window.calendarItem
Source File: chrome://calendar/content/calendar-event-dialog.js
Line: 2215

The deleteListener can't use window.calendarItem.id because the dialog is gone. It seems the transaction manager calls the saved listener when executing an undo.

This doesn't happen if I undo creating or modifying an event.

Comment 48

[Markus Adrario [:Taraman]]

I looked a bit more into this.

The problem is, that when the window is closed, the variables used in a listener are no more present.
In the Function onCommandSave() there is a workaround [1].
But this is not working properly as well.

If you first save the item via File-->Save and then close the window with File-->Close. the above Function also uses a listener and the undo even gives you an Error: 
Error: '[JavaScript Error: "Components is not defined" {file: "chrome://calendar/content/calendar-event-dialog.js" line: 2158}]'

So it seems to be the only chance to not use a listener and pass the information to the Callback function [2]

My proposed solution is to change the callback function as follows:
  let onModifyItem = function(action, item, calendar, originalItem, listener) {
        doTransaction(action, item, calendar, originalItem, listener); }
So we could also pass 'delete' as action.
The downside of this is, that there wont be a listener. So we could also replace the argument 'listener' with NULL or keep it for the case it becomes useful again.

The Function onCommandSave would have to be changed as well and I think when we leave it all to the Callback, we would have to remove the "Save"-only option from the Menu.
Or we could store the information in a copy of the item and then pass it to the Callback when the window is closing and the item changed.

What do you think about these solution-options?


[1]:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.js#2179 

[2] Callback function in case of modifying event:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-item-editing.js#206

Comment 49

[Martin Schröder [:mschroeder]]

Philipp, it would be better if you answer the questions, and discuss the options from comment#48. Thanks!

Comment 50

[Philipp Kewisch [:Fallen][📱📆]]

Both listeners are only needed if the dialog is still open, so try adding the condition |window && window.calendarItem| to the outermost if() block. This way on an undo operation, the condition will just evaluate to false and the rest will be ignored.

Comment 51

[Markus Adrario [:Taraman]]

Sometimes it's the easy solutions that don't come to mind... ;-)

Unlike comment #47 stated its not only a warning but an error I get on windows.

I tried it and this way there is no more error, but we still get a warning.
Is this acceptable?

Comment 52

[Philipp Kewisch [:Fallen][📱📆]]

What kind of warning is it? window.calendarItem is undefined? Maybe something like |typeof window.calendarItem == "Object"| works (not sure, maybe its "object", please check.

Comment 53

[Markus Adrario [:Taraman]]

Yes, it's 'window.calendarItem is undefined'.

I try with == 'Object', else I test for window.opener, since the main window (in case of undo) does not have one, but it is defined in both cases.

Comment 54

[Markus Adrario [:Taraman]]

Attachment: Patch V2.3 - fix undo + style cleanup from comment #45

if (typeof(window.calendarItem) == 'object') worked.

I fixed the listener in onCommandSave as well.

I tested with local calendar and calDav.

Comment 55

[Markus Adrario [:Taraman]]

Attachment: Patch V2.3a - Fix one more style nit

Use
if ("calendarItem" in window)
instead of
if (typeof(window.calendarItem) == 'object')

Comment 56

[Martin Schröder [:mschroeder]]

Comment on attachment 386963 [details] [diff] [review]
Patch V2.3a - Fix one more style nit

r=mschroeder, with just a single nit:

Don't remove the empty line. :)

>+                if ("calendarItem" in window) {
>+                    if (aId == window.calendarItem.id && Components.isSuccessCode(aStatus)) {
>+                        gConfirmCancel = false;
>+                        document.documentElement.cancelDialog();
>+                    }
>                 }
>             }
>         };
>-
>         if (window.calendarItem.parentItem.recurrenceInfo && window.calendarItem.recurrenceId) {
>             // if this is a single occurrence of a recurring item
>             let newItem = window.calendarItem.parentItem.clone();
>             newItem.recurrenceInfo.removeOccurrenceAt(window.calendarItem.recurrenceId);

Comment 57

[Markus Adrario [:Taraman]]

OK, Patch checked in,
added empty line again. ;-)

Comment 58

[Markus Adrario [:Taraman]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/8d44f1f7427e>

-> FIXED

Comment 59

[Roman Brandl]

excellent, thanks al lfor fixing this.

Comment 60

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100223 Calendar/1.0b2pre

Comment 61

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".