Closed Bug 1580445 Opened 6 months ago Closed 6 months ago

clean up some unused and little used functionality in calendar

Categories

(Calendar :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(8 files)

No description provided.
Attachment #9092052 - Flags: review?(paul)
Attachment #9092054 - Flags: review?(paul)
Attachment #9092055 - Flags: review?(paul)
Attachment #9092057 - Flags: review?(paul)
Attachment #9092058 - Flags: review?(paul)
Attachment #9092059 - Flags: review?(paul)
Comment on attachment 9092054 [details] [diff] [review]
bug1580445_showElement.patch

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

I'm sad to see these go, I liked them a lot :) Anyway, a few comments on the patch:

::: calendar/base/content/dialogs/calendar-properties-dialog.js
@@ +58,5 @@
>  
>    // Set up the disabled checkbox
>    let calendarDisabled = false;
>    if (gCalendar.getProperty("force-disabled")) {
> +    document.getElementById("force-disabled-description").setAttribute("hidden", "false");

Any reason you set to "false" here? I think using removeAttribute makes more sense.

::: calendar/lightning/content/imip-bar.js
@@ +140,5 @@
> +      button.setAttribute("hidden", "true");
> +    });
> +    buttons.forEach(aButton => ltnImipBar.getMenuItems(aButton).forEach(button => {
> +      button.removeAttribute("hidden");
> +    }));

I'd suggest to turn these into for..of loops, which have better performance than the functional variant.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +3996,5 @@
>      let sendTentativeEmail = document.getElementById("attendee-popup-sendtentativeemail-menuitem");
>      let firstSeparator = document.getElementById("attendee-popup-first-separator");
> +    [sendEmail, sendTentativeEmail, firstSeparator].forEach(item => {
> +      item.removeAttribute("hidden");
> +    });

Maybe just turn these into three separate lines? I don't think we need array construction and the function call for each item.

@@ +4007,5 @@
>      let attendee = window.attendees.find(aAtt => aAtt.id == attId);
>      if (attendee) {
> +      [mailto, remove, secondSeparator].forEach(item => {
> +        item.removeAttribute("hidden");
> +      });

Same

@@ +4014,5 @@
>        remove.attendee = attendee;
>      } else {
> +      [mailto, remove, secondSeparator].forEach(item => {
> +        item.setAttribute("hidden", "true");
> +      });

Same
Attachment #9092054 - Flags: review?(paul) → review+
Comment on attachment 9092055 [details] [diff] [review]
bug1580445_uncollapseElement.patch

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

::: calendar/lightning/content/imip-bar.js
@@ +142,5 @@
> +    buttons.forEach(aButton =>
> +      ltnImipBar.getMenuItems(aButton).forEach(button => {
> +        button.removeAttribute("hidden");
> +      })
> +    );

Let's turn this into a for..of
Attachment #9092055 - Flags: review?(paul) → review+
Attachment #9092057 - Flags: review?(paul) → review+
Attachment #9092059 - Flags: review?(paul) → review+
Attachment #9092051 - Flags: review?(paul) → review+
Attachment #9092052 - Flags: review?(paul) → review+
Comment on attachment 9092058 [details] [diff] [review]
bug1580445_updateSelectedLabel.patch

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

::: calendar/base/content/calendar-ui-utils.js
@@ -495,5 @@
>  }
>  
>  /**
> - * Setting labels on a menuitem doesn't update the label that is shown when the
> - * menuitem is selected. This function takes care by reselecting the item

Is this code really no longer needed? Would be good to test it on a few OSes, seems like a hack necessary for only one of them.
Attachment #9092058 - Flags: review?(paul) → review+

I think that must have been an xbl related bug...

Summary: clean up some unused an little used functionality in calendar → clean up some unused and little used functionality in calendar

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/41554a073241
remove ref to no longer existing calendar-view-bindings.css. r=Fallen
https://hg.mozilla.org/comm-central/rev/9de382d47166
move calendar-unifinder-todo.js functioality into today-pane.js which is the only consumer. r=Fallen
https://hg.mozilla.org/comm-central/rev/40347deab546
remove showElement and hideElement. r=Fallen
https://hg.mozilla.org/comm-central/rev/d654a23f1716
remove unused uncollapseElement and almost unused collapseElement, as well as enableElement+disableElement. r=Fallen
https://hg.mozilla.org/comm-central/rev/144017b0f32f
remove unused uncheckChildNodes, processEnableCheckbox, updateListboxDeleteButton. r=Fallen
https://hg.mozilla.org/comm-central/rev/aed7b8031d12
remove no longer needed updateSelectedLabel. r=Fallen
https://hg.mozilla.org/comm-central/rev/8398f7ef2ca5
inline getOtherOrientation. r=Fallen

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 71

Care to fix the test failure?
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/views/testTaskView.js | testTaskView.js::testTaskView

Flags: needinfo?(mkmelin+mozilla)

There were some mixups in one file.

Flags: needinfo?(mkmelin+mozilla)

Thanks, fixes mozmake -C comm/calendar/test/mozmill SOLO_TEST=views/testTaskView.js mozmill-one.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5dd7e31170fc
fix disableElement/enableElement mix-ups noticed by the test. rs=bustage-fix DONTBUILD

I'm getting the following error when changing task list filter in today pane using Tb 71.0a1 (2019-09-14):

ReferenceError: updateCalendarToDoUnifinder is not defined messenger.xul:1:1
oncommand chrome://messenger/content/messenger.xul:1
uncaught exception: Object

Could be regressed by this change here:

move calendar-unifinder-todo.js functioality into today-pane.js which is the only consumer.

The method is used in calendar-common-sets.xul too according to https://searchfox.org/comm-central/search?q=updateCalendarToDoUnifinder

Flags: needinfo?(mkmelin+mozilla)

Yes. It is indeed used only for the today pane, but looks like I forgot to update how it's referenced.

Flags: needinfo?(mkmelin+mozilla)
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ce076d6ad3b8
followup to correct callers of moved updateCalendarToDoUnifinder. r=bustage-fix
You need to log in before you can comment on or make changes to this bug.