Closed Bug 413474 Opened 12 years ago Closed 12 years ago

Delete button in Mail mode does not delete events or tasks

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

References

Details

(Keywords: regression)

Attachments

(1 file)

When the focus within the mail-mode lies upon the agenda pane or task pane the delete-button of the mail toolbar should be enabled at all times. Activating the button should delete the selected elements of the focused pane and not the selected message.
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8? → blocking-calendar0.8?
It seems like a good idea but I foresee people accidentally deleting an event or task when they meant to delete an email.

I don't know if there's a good way to prevent that, but I think that one thing that should be done first is to make the selection colors in the today pane behave exactly like Thunderbird's folder/thread/message panes (which correctly use the OS-defined selection colors).  This would make it more obvious to the user which pane is currently focused.

The selection colors in the todo list don't currently use the OS colors at all.

The event list uses the correct system color when I click on an event, but when I click a different pane (e.g. the message pane) the background color of the event changes to white instead of to the system color (the "selected but not focused" color).
Not important enough for blocking-status.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
@Simon: Please take a look at bug 359443 (same bug, fixed in 0.7, blocking +) So I think this regression should fixed in 0.8, too.

STEPS TO REPRODUCE:
===================

- switch to mail mode
- open an eMail
- go to the today pane
- select an event or a task
- push the 'Delete' button in the Toolbar

RESULT:
=======

- this action deletes the eMail and not the selected event/task

EXPECTED RESULT:
================

- the selected event/task should be deleted

REPRODUCIBLE:
=============

- always 
Flags: blocking-calendar0.8- → blocking-calendar0.8?
Keywords: regression
I can confirm Andreas observation.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071031 Lightning/0.8pre (2008012818) Thunderbird/2.0.0.9 ID:2007103104
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Assignee: nobody → Berend.Cornelius
Attached patch patch v. 1Splinter Review
The patch attached should fix the problem. Yet it raises some questions that were already hit by Pete's comment #1. I agree that the selection color of the agenda-pane and the todo-pane are not consistent and I would like Christian to comment on that. With this patch it is likely that users will accidently delete an event or task. When testing my patch I noticed that that deleting a mail in mail mode will add an element to the thunderbird undo-redo stack but deleting an event or task dosn't, whereas in calendar or task-mode an action will be added to an own lightning-undo-redo stack. So we should improve our undo-redo -stack handling. I will file an issue for this.
Attachment #301645 - Flags: ui-review?(christian.jansen)
Attachment #301645 - Flags: review?
Blocks: 415900
Regarding the Undo issue for event/task changes in Mail Mode see Bug 398405.
Oh, yes you are right. I submitted anyway bug 415900 where I described our idea of the undo-redo-stack

Comment on attachment 301645 [details] [diff] [review]
patch v. 1

Actually I am not happy myself with the patch as it does not consider if selected agenda-items are readonly or not. Pleas wait until I deliver a new patch
Attachment #301645 - Flags: review? → review-
I tried to implement a second patch that considers my idea of comment #8. Yet I had some problems to get it working: I wanted to disable the delete-button  when the selected event of the agenda-listbox gets readonly. I debugged into Thunderbird's Code in "_getControllerForCommand" in globalOverlay.js and I could clearly see that the wrong commandcontroller was returned when the command is disabled. To solve the problem one would have to implement an own command updater which seems to be a bit too much for this comparatively small problem. Therefor I would like to submit a follow up issue for this and ask again for review of the first patch.

>   _getControllerForCommand: function(command) {
>     try {
>       var controller = 
>       top.document.commandDispatcher.getControllerForCommand(command);
>       if (controller && controller.isCommandEnabled(command))
>         return controller;
>    }
>     catch(e) {
>     }
>     var controllerCount = window.controllers.getControllerCount();
>     for (var i = 0; i < controllerCount; ++i) {
>       var current = window.controllers.getControllerAt(i);
>       try {
>         if (current.supportsCommand(command) && >current.isCommandEnabled(command))
>         return current;
>      }
>      catch (e) {
>      }
>    }
>    return controller || window.controllers.getControllerForCommand(command);
>   },
Attachment #301645 - Flags: review- → review?(philipp)
Comment on attachment 301645 [details] [diff] [review]
patch v. 1

>+                    if (aCommand == "cmd_delete" || aCommand == "button_delete") {
>+                        var focusedElement = document.commandDispatcher.focusedElement;
>+                        if (focusedElement) {
>+                            if (focusedElement.getAttribute("id") == "agenda-listbox") {
>+                                 return agendaListbox.isEventSelected();
>+                            } else if (focusedElement.className == "calendar-task-tree") {
>+                                 return this.writable &&
>+                                        this.todo_items_selected &&
>+                                        this.todo_items_writable;
>+                            }
>+                        }
>+                    }
Please add a comment why such special casing is needed for the delete button in at least one of isCommandEnabled or doCommand, preferably both.


r=philipp
Attachment #301645 - Flags: review?(philipp) → review+
Status: NEW → ASSIGNED
Target Milestone: Future → 0.8
Comment on attachment 301645 [details] [diff] [review]
patch v. 1

r=christian
Attachment #301645 - Flags: ui-review?(christian.jansen) → ui-review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

=>FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Checked in latest nightly build 20080021319 -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.