1439182 - calendar-properties-dialog does not open.

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Arshad Khan [:arshad]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

I double clicked on a calendar to open calendar properties dialog.


Actual results:

Calendar properties dialog didn't open. I checked console and i can see the error `window.openDialog is not a function`


Expected results:

Calendar properties dialog should have opened.

Comment 1

[Arshad Khan [:arshad]]

This error can be removed by either swapping function params on https://dxr.mozilla.org/comm-central/source/calendar/base/modules/calWindowUtils.jsm#32 or https://dxr.mozilla.org/comm-central/source/calendar/base/content/widgets/calendar-list-tree.xml#161

Comment 2

[Arshad Khan [:arshad]]

Attachment: calendar-list-tree.patch

This patch will do the work.

Comment 3

[Jorg K (CEST = GMT+2)]

Please request review for your patch from Philipp (:Fallen) or MakeMyDay.

Comment 4

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

Comment on attachment 8951942 [details] [diff] [review]
calendar-list-tree.patch

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

Thanks for the patch! Can you also take care of the other callsite at calendar/base/content/calendar-common-sets.js ? r+ with that taken care.

Comment 5

[Arshad Khan [:arshad]]

Attachment: cal-prop-dialog.patch

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8952024 [details] [diff] [review]
cal-prop-dialog.patch

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

::: calendar/base/content/calendar-common-sets.js
@@ +359,1 @@
>                  cal.window.openCalendarWizard(window);

You missed this one, no?

Comment 7

[Arshad Khan [:arshad]]

sure i did

Comment 8

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

I discussed with Arshad via IRC that there is more to this. The argument order was flipped in bug 1436488, because I thought it makes more sense to make the window optional and put it at the end. The migration code in calUtilsCompat was written after the fact, and didn't take this into account.

The likely right fix would be to create a separate migration function in calUtilsCompat.jsm for the swapped argument functions that flips the arguments correctly.

Comment 9

[Jorg K (CEST = GMT+2)]

Looks like there is another call site:
calendar/lightning/content/messenger-overlay-accountCentral.xul
27 onclick="window.parent.cal.openCalendarWizard(window);"/>

Comment 10

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

Attachment: Fix - v1

Thinking about it more I'm going with Arshad's suggestion to make the transition less confusing. Let's keep the window argument first, where ever this is called we will want to pass in a window anyway.

Comment 11

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

Attachment: Fix - v1

Comment 12

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

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8952222 [details] [diff] [review]
Fix - v1

Maybe a good idea not to repeat vanilla attachment names across bugs.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3b00b92e5342
Fix argument order for cal.window methods. r=MakeMyDay

Comment 15

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

(In reply to Jorg K (GMT+1) from comment #13)
> Comment on attachment 8952222 [details] [diff] [review]
> Fix - v1
> 
> Maybe a good idea not to repeat vanilla attachment names across bugs.

I would be interested to hear how this is affecting you. The filename I generate is from the hg bookmark, so it is often the same.

Comment 16

[Jorg K (CEST = GMT+2)]

It gives problems importing the patch with "hg import" (well, that offers to rename it), so I prefer to rename the file on BMO to minimise the possible source of error. Many patch authors give their patches a bug number or describe the content. "datautils-regression" in two bugs is something I wouldn't have done. Anyway, I'll cope.

Comment 17

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

I've written a thing that will generate a more verbose filename when exporting, next time you will get something like bug-1439182-fix-argument-order-for-calwindow-methods.diff

Comment 18

[Jorg K (CEST = GMT+2)]

Actually, that would be perfect since it contains the bug number and also what that bug is about. Thanks!