Menu command status is not updated on calendar property changes

RESOLVED FIXED in 3.1

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

Tracking

Lightning 2.6.3

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

1. Go to calendar tab
2. Open property dialog of a calendar
3. Change read-only status (as only this property has a visible effect in the UI)
4. Click OK 


Actual results:

The new item menu commands do not change their status (enabled/disabled) after closing the property dialog. Currently the change is only reflected if you change the focus to another UI element and then back to the calendar which properties you edited.



Expected results:

The menu command status should change immediately after closing the property dialog.

This is caused by a missing implementation of the onPropertyChanged function in the compositeObserver in calendar-management.js
Assignee

Updated

6 years ago
Attachment #8345926 - Flags: review?(bv1578)
Attachment #8345926 - Attachment is patch: true
Attachment #8345926 - Attachment mime type: text/x-patch → text/plain
Assignee: nobody → makemyday

Comment 1

6 years ago
The patch fixes the behavior mentioned in the steps to reproduce but the problem, is that I'm not sure if it's always a bug, or better, if this is a consequence of bug 818688 (or maybe even the same).

The behavior is strictly related with bug 818688 because it depends on the selected calendar in the calendars list so, if you have e.g. three calendars, one read-only and two writable, when you change the read-only status of one of the two writable calendars (and doing that you must select the calendar), the status enabled/disabled of the commands doesn't change and this actually should be the correct behavior for Lightning because there is another calendar that is still writable. Instead the behavior is wrong when you change the read-only status of the not writable calendar because the commands stay disabled when there are no more read-only calendars in the list.

The anomalous behavior is that the action of selecting a calendar changes the enabled/disabled status of the commands for new event according to the read-only status of that particular calendar. Lightning shouldn't work in that way, and it never worked in that way before the patch for bug 586276, those commands should change status according to the read-only status of *all* the calendars in the list because they allow to open a dialog where the user can select any calendar he wants inside a list of *writable* calendars, so, as long as a writable calendar exist, those commands should allow to open the dialog independently from the selected calendar.

This patch changes the behavior because it calls the function calendarUpdateNewItemsCommand() which links the variable CalendarNewItemsCommandEnabled to the selected calendar (see bug 818688 comment 1) so the calendar just edited changes the enabled/disabled status of the commands because it is selected (to open the property dialog you have to select it).

This is a fix that would be useful inside the anomalous behavior reported with bug 818688, but it wouldn't change that anomalous behavior.

Philipp, what's your opinion?
Flags: needinfo?(philipp)
Assignee

Comment 2

6 years ago
I guess the behaviour is not strictly related to the bug 818688, even though its occurence will be limited to the case of disabling the last or enabling the first writable calender in list once bug 818688 is fixed correcty. Therefore I assume it will be needed anyway.

I have also already investigated what needs to be fixed for bug 818688 - I will comment my finding on that one.
Did you see this part: 
http://mxr.mozilla.org/comm-central/source/calendar/base/content/widgets/calendar-list-tree.xml#140

I agree its not the best that its spread so far apart, but at least in theory, when readOnly is changed then the commands should be updated. Wouldn't this patch just cause double updates? I wonder why its not working already?
Flags: needinfo?(philipp)
Assignee

Comment 4

6 years ago
Indeed, this would cause double updates. Did not see the observer code before.

Key of the patch and also the reason, why the observer code does not work as expected is calling calendarUpdateNewItemsCommand() (to take care of the ACL checks and set command status accordingly) before updateCommands().

Patch is updated.

Maybe it's useful to insert a comment inside the interface implementation in calendar-management.js hinting to the observer code in calendar-list-tree.xml as well?
Attachment #8349301 - Flags: review?(bv1578)
Attachment #8345926 - Attachment is obsolete: true
Attachment #8345926 - Flags: review?(bv1578)

Comment 5

6 years ago
Comment on attachment 8349301 [details] [diff] [review]
CommandsNotUpdatedOnPropertyChanged-V2.patch

Philipp didn't make any remark about the way the fix works, so I give it a r+ since the call to calendarUpdateNewItemsCommand() now is in the right place.
I strongly hope that the patch you attached for bug 818688 will change the global behavior because IMHO the way the commands currently work, relatively to the read-only/writable status of the calendars, needs to be changed.


>                 case "readOnly":
>+                	calendarUpdateNewItemsCommand();
>                     document.commandDispatcher.updateCommands("calendar_commands");

please always use spaces instead of tabs to indent the code.

r+ with the indentation fixed.

Do you have an account to push the patch on the repository?
Attachment #8349301 - Flags: review?(bv1578) → review+
Assignee

Comment 6

6 years ago
Patch with the indentation fixed.
Attachment #8349301 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8350983 - Flags: checkin?
Assignee

Updated

6 years ago
Keywords: checkin-needed
Comment on attachment 8350983 [details] [diff] [review]
CommandsNotUpdatedOnPropertyChanged-V3.patch

In the future, you can just use the checkin-needed bug keyword. Also, please make sure that future patches contain all the required metadata needed for checkin. Makes life much easier for those landing on your behalf! Thanks for the patch :)

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8350983 - Flags: checkin?
https://hg.mozilla.org/comm-central/rev/b68211fc52a3
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.9
Target Milestone: 2.9 → 3.1
You need to log in before you can comment on or make changes to this bug.