Closed
Bug 413474
Opened 16 years ago
Closed 16 years ago
Delete button in Mail mode does not delete events or tasks
Categories
(Calendar :: Calendar Frontend, defect)
Calendar
Calendar Frontend
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: berend.cornelius09, Assigned: berend.cornelius09)
References
Details
(Keywords: regression)
Attachments
(1 file)
5.79 KB,
patch
|
Fallen
:
review+
chris.j.bugzilla
:
ui-review+
|
Details | Diff | Splinter Review |
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?
Updated•16 years ago
|
Flags: wanted-calendar0.8? → blocking-calendar0.8?
Comment 1•16 years ago
|
||
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).
Comment 2•16 years ago
|
||
Not important enough for blocking-status.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Comment 3•16 years ago
|
||
@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
Comment 4•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → Berend.Cornelius
Assignee | ||
Comment 5•16 years ago
|
||
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?
Comment 6•16 years ago
|
||
Regarding the Undo issue for event/task changes in Mail Mode see Bug 398405.
Assignee | ||
Comment 7•16 years ago
|
||
Oh, yes you are right. I submitted anyway bug 415900 where I described our idea of the undo-redo-stack
Assignee | ||
Comment 8•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #301645 -
Flags: review? → review-
Assignee | ||
Comment 9•16 years ago
|
||
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); > },
Assignee | ||
Updated•16 years ago
|
Attachment #301645 -
Flags: review- → review?(philipp)
Comment 10•16 years ago
|
||
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+
Updated•16 years ago
|
Status: NEW → ASSIGNED
Target Milestone: Future → 0.8
Comment 11•16 years ago
|
||
Comment on attachment 301645 [details] [diff] [review] patch v. 1 r=christian
Attachment #301645 -
Flags: ui-review?(christian.jansen) → ui-review+
Assignee | ||
Comment 12•16 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH =>FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 13•16 years ago
|
||
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.
Description
•