1308324 - ShowAs (Transparency) of events is reset on calendar changes

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

In the event editing dialog, non-default show-as settings ("transparent" for regular events, "opaque" for all-day events) are reset when changing the calendar of the event.

STR:
0. Lightning 4.7, Thunderbird 45.3.0; with two empty local calendars
1. Select the first empty calendar
2. In the calendar tab, double-click to some empty date to open a new event dialog
3. Select the transparent show-as-status through the menu (rough translation from German: Settings|Show as|Free, not sure about English labels)
4. Select the second calendar in the drop-down menu of the dialog

Expected:
The event continues to be transparent (shown as free)

Actual:
The event is now opaque (shown as busy)


Technical reason:
updateCalendar (calendar-event-dialog.js, <https://dxr.mozilla.org/comm-release/source/calendar/base/content/dialogs/calendar-event-dialog.js#2402>) calls updateAllDay (calendar-event-dialog.js, <https://dxr.mozilla.org/comm-release/source/calendar/base/content/dialogs/calendar-event-dialog.js#1430>), which resets the transparency status of the event to the default value for the all-day value selected by the user.


I'd be willing to provide a patch altering updateAllDay to no longer call setShowTimeAs (and thus stick to its documented contract), and introduce a separate function called on changes of the all-day status.
If you're interested, please provide a reference to base the patch on (comm-central? Some structures seem to have changed, but the buggy code is still present in <https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning-item-iframe.js#1704>).

Comment 1

[[:MakeMyDay]]

Confirming. A patch for comm-central will be welcome, so I assign this bug to you.

Please see [1] how to create a patch appropiate for landing and don't forget to ask for review when uploading the patch (setting the r? flag - choose one of the proposed reviewers there).

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 2

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

Attachment: Fix

I could not get MozReview to work (debian's hg seems to be too old), thus I submitted the patch manually. For some reason 'mach test' doesn't like me as well, thus no automatic testing has been performed ("OSError: [Errno 2] No such file or directory", might be related to the chroot I build in).

I tested manually in the regular XUL UI, the 'new' UI (calendar.item.useNewItemUI) does not seem to work at all for me (independent of this change).

Comment 3

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

Comment on attachment 8799999 [details] [diff] [review]
Fix

Decathlon, can you take care of the review? I'm currently low on time. The code itself looks fine, but it would be great to give this a spin.

Dirk, no worries about mozreview, we don't use it in Calendar anyway. I think it is too clunky and slow.

Regarding testing, I don't think we have tests that cover this, but indeed mach test does not work well for comm-central. mach xpcshell-test calendar works, and so does make -C objdir-tb/calendar/test/mozmill mozmill

Comment 4

[Decathlon]

Comment on attachment 8799999 [details] [diff] [review]
Fix

Review of attachment 8799999 [details] [diff] [review]:
-----------------------------------------------------------------

The patch works as expected but since you deleted a call to a function from inside the function updateAllDay(), it has to be checked if it doesn't break something else. For now I've found out a little issue that needs a fix.

The button "Show Time as" in the dialog's toolbar never works at first attempt (customize the toolbar the button is absent: right click on the toolbar > "customize" > drag the button).
The button has a submenu Busy/free that works perfectly, but it allows also to switch the status between Busy and Free every time is clicked, and this last functionality doesn't work when first clicking the button after the the dialog has been opened.

It seems that gConfig.showTimeAs has not been initialized when used for the first time inside the function rotateShowTimeAs:
https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning-item-panel.js#712

That code in those files is recent and it might have its own issues maybe highlighted by your or other patch.

I set r- for now, would be great if you could take a look to this issue.

Comment 5

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

(In reply to Decathlon from comment #4)
> The button "Show Time as" in the dialog's toolbar never works at first
> attempt
I can reproduce that. The issue is not in the patch, but an unrelated bug triggered by the patch (imho): the function updateMarkCompletedMenuItem is called (in lightning-item-panel.js, <https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning-item-panel.js#768>), but a comment there states
> // Command only exists for tab case, function not called for dialog windows.
I can understand the first bit (the command fetched in the next line is indeed not found and causes the message handler to crash before setting the show-as value, causing the bug described), but not the second one: why shouldn't the method be called? Anything besides that comment seems as if it *should* be called in any case, but the comment is probably there for a reason... I thus don't know how the code should be fixed (but I think it should happen in a separate bug).

Independently of that, I thought about possible missing calls to setShowTimeAs. The only issue I could come up with, would be missing initialization for new events, so the patch *could* be amended with something like
> if (!gConfig.showTimeAs) {
>   setShowTimeAs(gStartTime && gStartTime.isDate);
> }
when initializing the dialog for stylistic reasons (loadDialog, after setting gConfig.showTimeAs in <https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning-item-iframe.js#792>). However, gConfig.showTimeAs is never falsy unless I forgot some corner case (as events are initialized as OPAQUE).

Comment 6

[Decathlon]

Comment on attachment 8799999 [details] [diff] [review]
Fix

Review of attachment 8799999 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to rsjtdrjgfuzkfg (Dirk Steinmetz) from comment #5)

> I can reproduce that. The issue is not in the patch, but an unrelated bug
> triggered by the patch (imho): the function updateMarkCompletedMenuItem is
> called (in lightning-item-panel.js,
> <https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/
> lightning-item-panel.js#768>)

Yes, you are right. It is caused by bug 1300849. Returning form updateMarkCompletedMenuItem() without doing nothing makes the button working always.

> > // Command only exists for tab case, function not called for dialog windows.
> I can understand the first bit (the command fetched in the next line is
> indeed not found and causes the message handler to crash before setting the
> show-as value, causing the bug described), but not the second one: why
> shouldn't the method be called?

This is due to the new implementation of new/edit dialog in tabs (see bug 1277972). Not everything is even clear to me so far.

> Independently of that, I thought about possible missing calls to
> setShowTimeAs. The only issue I could come up with, would be missing
> initialization for new events, so the patch *could* be amended with
> something like
> > if (!gConfig.showTimeAs) {
> >   setShowTimeAs(gStartTime && gStartTime.isDate);
> > }
> when initializing the dialog for stylistic reasons (loadDialog, after
> setting gConfig.showTimeAs in
> <https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/
> lightning-item-iframe.js#792>).

It seems that new events/tasks appear always correctly initialized even when changing the default transparency preferences for normal events and "all day" ones, so it should be useless.
I don't find nothing else wrong while testing, setShowTimeAs() has definitely to stay out of updateAllDay().

Another problem for the free/busy status, IMHO, is that it should not change anymore according to default settings for normal/"all day" events once the user has deliberately selected the status as he wants for that event, but this is definitely another issue.

If you don't have to add something else, please upload the patch with the reviewer r=Decathlon in the message header, so there's someone to blame if the patch will cause other issues ;)

Comment 7

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

Attachment: Fix with r=Decathlon

(In reply to Decathlon from comment #6)
> If you don't have to add something else, please upload the patch with the
> reviewer r=Decathlon in the message header, so there's someone to blame if
> the patch will cause other issues ;)
See updated attachment.

Comment 8

[Decathlon]

Setting dependence with bug 1300849 for the issue in the free/busy button as described in comment 4.

Comment 9

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/c04ea45a0f9eb31bed31eda09ad2fc00084b2469