1523604 - [de-xbl] Remove calendar-subscriptions-richlistbox binding.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: calendar-subscriptions-richlistbox.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9041716 [details] [diff] [review]
calendar-subscriptions-richlistbox.patch

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

Where/how can I access this dialog? Please add that to the documentation

::: calendar/base/content/dialogs/calendar-subscriptions-dialog.js
@@ +15,5 @@
> +if (!customElements.get("richlistbox")) {
> +    delete document.createElement("richlistbox");
> +}
> +
> +class MozCalendarSubscriptionsRichlistbox extends customElements.get("richlistbox") {

please add documentation, as in "This widget is typically found at....   and used to ...."

@@ +16,5 @@
> +    delete document.createElement("richlistbox");
> +}
> +
> +class MozCalendarSubscriptionsRichlistbox extends customElements.get("richlistbox") {
> +    addCalendar(aCalendar, bSubscribed) {

remove a and b conventions

Comment 3

[Arshad Khan [:arshad]]

I am not sure here as well, when the Find Calendar option in list-calendar-context-menu(https://searchfox.org/comm-central/source/calendar/base/content/calendar-calendars-list.xul#32) is enabled. Clickin on that opens the dialog which has caleendar-subscription-richlistbox.

Comment 4

[Magnus Melin [:mkmelin]]

So how did you test it? Please find out and document it.

Comment 5

[Arshad Khan [:arshad]]

hey when does find calendar option visible(not collapsed) in the popup menu? I have setup caldav calendar for my google account.

Comment 6

[Arshad Khan [:arshad]]

this is the popup menu I am talking about - https://searchfox.org/comm-central/source/calendar/base/content/calendar-calendars-list.xul#32

Comment 7

[[:MakeMyDay]]

As far I can see, currently only the wcap provider uses that feature, so maybe setting up a fake subscription could trigger enabling that menu item. I'm not aware of any public wcap calendar services. If that doesn't work, you can trigger the dialog that gets opened by the menu item from the error console.

Maybe Philipp's wip patch for caldav calendar discovery that missed the TB60 train might also enable the button. You may want to ping him for a debitrotted patch to check it.

Comment 8

[Arshad Khan [:arshad]]

(In reply to [:MakeMyDay] from comment #7)

As far I can see, currently only the wcap provider uses that feature, so maybe setting up a fake subscription could trigger enabling that menu item. I'm not aware of any public wcap calendar services. If that doesn't work, you can trigger the dialog that gets opened by the menu item from the error console.

Maybe Philipp's wip patch for caldav calendar discovery that missed the TB60 train might also enable the button. You may want to ping him for a debitrotted patch to check it.

Already did, I think he is busy.

Comment 9

[Arshad Khan [:arshad]]

Attachment: calendar-subscriptions-richlistbox.patch

The patch is same as calendar-invitations-richlistbox because the the element is same except the name. I tried hacking a few things and adding richlistitem inside it. The UI to me look ok and the vertical alignment of label and checkbox that you see in the screenshot is not related to this patch.

Comment 10

[Arshad Khan [:arshad]]

Attachment: cal_subscriptions_dialog.png

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9045631 [details] [diff] [review]
calendar-subscriptions-richlistbox.patch

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

I think it makes sense to inline this too, like the other one this is also just used once, and there is not much behavior

Comment 12

[Arshad Khan [:arshad]]

Attachment: calendar-subscriptions-richlistbox.patch

Comment 13

[Arshad Khan [:arshad]]

Comment on attachment 9045905 [details] [diff] [review]
calendar-subscriptions-richlistbox.patch

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

::: calendar/base/content/dialogs/calendar-subscriptions-dialog.xul
@@ +51,5 @@
>                   value="&calendar.subscriptions.dialog.select.label.value;"
>                   crop="end"/>
>          </row>
>          <row flex="1">
> +          <richlistbox id="subscriptions-listbox" flex="1"/>

NOTE: this will add those scrollbar and opther elements that get appended by richlistbox ce. I dont think they might cause any problem.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9045905 [details] [diff] [review]
calendar-subscriptions-richlistbox.patch

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

Didn't test it, but looks correct to me.

Comment 15

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

Comment on attachment 9045905 [details] [diff] [review]
calendar-subscriptions-richlistbox.patch

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

::: calendar/base/content/dialogs/calendar-subscriptions-dialog.js
@@ +102,5 @@
>      let richListBox = document.getElementById("subscriptions-listbox");
> +
> +    while (richListBox.hasChildNodes()) {
> +        richListBox.lastChild.remove();
> +    }

You can use this helper: https://searchfox.org/comm-central/source/calendar/base/content/calendar-ui-utils.js#226

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #15)

You can use this helper:
https://searchfox.org/comm-central/source/calendar/base/content/calendar-ui-utils.js#226

Helpers that do that little should really be killed off asap. That just adds complexity and making it harder for people to work on the coede. It's like having a helper for a for loop. Wait, that's not like - it is a helper for a loop!

Comment 17

[Arshad Khan [:arshad]]

Try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=086c53899682590e960501279bd9e0d72bdb72d5

Comment 18

[Arshad Khan [:arshad]]

Attachment: calendar-subscriptions-richlistbox.patch

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/42de459b3e60
Migrate calendar-subscriptions-richlistbox binding to custom element. r=philipp