Closed Bug 1567424 Opened 1 year ago Closed 1 year ago

remove broadcasters from calendar/

Categories

(Calendar :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(5 files, 1 obsolete file)

Remove <broadcaster> usage from lightning.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9090312 - Flags: review?(paul)
Comment on attachment 9090314 [details] [diff] [review]
bug1567424_broadcaster_lightning.part3_calendarviewBroadcaster.patch

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

A few suggestions. r+ with those considered.

::: calendar/base/content/calendar-base-view.js
@@ +545,5 @@
> +     * @param {calICalendarView} [calViewElem] - A calendar view element.
> +     */
> +    onResize(calView) {
> +      // Child classes should provide the implementation.
> +      throw new Error(this.constructor.name + ".onResize not implemented");

Nice.  I like it.

::: calendar/base/content/calendar-chrome-startup.js
@@ +46,5 @@
>    getViewDeck().addEventListener("itemselect", calendarController.onSelectionChanged, true);
>  
>    // Start alarm service
>    Cc["@mozilla.org/calendar/alarm-service;1"].getService(Ci.calIAlarmService).startup();
> +  document.getElementById("calsidebar_splitter").addEventListener("command", e => {

Might as well do "event" instead of "e" here and in similar places.  Or better just use `() => {` since we're not using the event argument.

@@ +50,5 @@
> +  document.getElementById("calsidebar_splitter").addEventListener("command", e => {
> +    document.dispatchEvent(new CustomEvent("viewresize", { bubbles: true }));
> +  });
> +  window.addEventListener("resize", e => {
> +    document.dispatchEvent(new CustomEvent("viewresize", { bubbles: true }));

I'd favor using camelCase for this viewResize event name for better readability, unless there's a reason to keep it all lowercase.

::: calendar/base/content/calendar-event-column.js
@@ -166,5 @@
>        this.mEventMapTimeout = null;
>  
> -      // Sometimes we need to add resize handlers for columns with special
> -      // widths.  When we relayout, we need to cancel those handlers.
> -      this.mHandlersToRemove = [];

Would be nice if this indicated which columns with special width it was talking about.  Anyway, looks like nothing is ever added to this array, so I assume it must not be needed anymore.
Attachment #9090314 - Flags: review?(paul) → review+
Comment on attachment 9090312 [details] [diff] [review]
bug1567424_broadcaster_lightning.patch

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

A few nits and a broken task view search box.

::: calendar/base/content/calendar-task-view.js
@@ +348,5 @@
>  /**
>   * Updates the currently applied filter for the task view and refreshes the task
>   * tree.
>   *
> + * @param {String} [aFilter] - the filter name to set

aFilter -> filter  (Remove aArgs while we are here.)

@@ +353,5 @@
>   */
>  function taskViewUpdate(aFilter) {
>    let tree = document.getElementById("calendar-task-tree");
> +  let oldFilter = tree.getAttribute("filterValue");
> +  let filter = aFilter;

This assignment seems unnecessary.  The variable is never modified or reassigned, so it serves no purpose.

::: calendar/base/content/calendar-task-view.xul
@@ +51,5 @@
>                     placeholder=""
>                     emptytextbase="&calendar.task.text-filter.textbox.emptytext.base1;"
>                     keyLabelNonMac="&calendar.task.text-filter.textbox.emptytext.keylabel.nonmac;"
>                     keyLabelMac="&calendar.task.text-filter.textbox.emptytext.keylabel.mac;"
> +                   oncommand="taskViewUpdate(this.value);"/>

Passing `this.value` here doesn't make sense to me, and it apparently breaks the task view "Filter tasks" search textbox.  With patches applied, typing into the textbox causes an error in the console and the keyword search doesn't happen.
Attachment #9090312 - Flags: review?(paul)
Comment on attachment 9090313 [details] [diff] [review]
bug1567424_broadcaster_lightning.part2_unifinder-todo-filter-broadcaster.patch

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

A few nits. r+ with those addressed.

::: calendar/base/content/calendar-ui-utils.js
@@ -438,5 @@
> -  }
> -  return checkRadioControl(parent, value);
> -}
> -
> -/**

It makes me sad to see these helper functions go, but maybe it's for the best.  I know you don't like them.

::: calendar/base/content/calendar-unifinder-todo.js
@@ +13,5 @@
>   */
>  function prepareCalendarToDoUnifinder() {
>    // add listener to update the date filters
> +  getViewDeck().addEventListener("dayselect", event => {
> +    updateCalendarToDoUnifinder();

Can we add a comment to say that we're wrapping this function call in a function so the event is not passed as an argument.  I believe that's the reason.

@@ +22,5 @@
>  
>  /**
>   * Updates the applied filter and show completed view of the unifinder todo.
>   *
> + * @param {String} [aFilter] - the filter name to set

aFilter -> filter (or something not aArgs)
Attachment #9090313 - Flags: review?(paul) → review+
Comment on attachment 9090315 [details] [diff] [review]
bug1567424_broadcaster_lightning.part4_modeBroadcaster.patch

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

A few nits and suggestions. r+ with those considered/addressed.

::: calendar/base/content/today-pane.js
@@ +439,5 @@
>  
> +    TodayPane.updateDisplay();
> +    TodayPane.updateSplitterState();
> +    todaypane.width = todaypane.getModeAttribute("modewidths");
> +    TodayPane.previousMode = gCurrentMode;

While we're here let's rename todaypane to todayPanePanel (to avoid the confusing closeness of TodayPane and todaypane).

::: calendar/base/content/widgets/calendar-modebox.js
@@ +54,5 @@
>          }
>        }
>      }
>  
> +    attributeChangedCallback(name, oldValue, newValue) {

Nice use of this Custom Element feature.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +650,5 @@
> +      `menuitem[command="switch2calendar"],menuitem[command="switch2task"],
> +       toolbarbutton[command="switch2calendar"],toolbarbutton[command="switch2task"]`
> +    )
> +    .forEach(e => {
> +      e.setAttribute("checked", e.getAttribute("value") == gCurrentMode);

Can we use "elem" or "item" or something here (and below) instead of just "e"?

Looks like XUL supports |checked="false"| in cases like this, whereas in HTML you have to remove the "checked" attribute (sigh).  Elsewhere in these patches you've used removeAttribute() for the false case.  Should we do that here as well for consistency and to anticipate an HTML future beyond XUL?

@@ +666,5 @@
> +
> +/**
> + * Switches to the mail mode.
> + */
> +function ltnSwitch2Mail(mode = "calendar") {

Looks like this mode arg is unused and should be removed.
Attachment #9090315 - Flags: review?(paul) → review+

This is a nice improvement. Farewell broadcasters, and good riddance, we will be better off without you!

I searched for "broadcaster" and found a couple of unrelated leftovers (in mailnews/ not calendar/). Shall we remove them while we're at it?
https://searchfox.org/comm-central/search?q=broadcaster_attachSignature&path=

(In reply to Paul Morris [:pmorris] from comment #7)

I'd favor using camelCase for this viewResize event name for better
readability, unless there's a reason to keep it all lowercase.

Events should always be alllowercase: https://developer.mozilla.org/en-US/docs/Web/Events
(Not sure why, but they are, so for consistency)

(In reply to Paul Morris [:pmorris] from comment #9)

Can we add a comment to say that we're wrapping this function call in a
function so the event is not passed as an argument. I believe that's the
reason.

Yeah, that is the reason, but it would be confusing to add a comment to explain it. That would be explaining what used to be.

(In reply to Paul Morris [:pmorris] from comment #10)

  • todaypane.width = todaypane.getModeAttribute("modewidths");
  • TodayPane.previousMode = gCurrentMode;

While we're here let's rename todaypane to todayPanePanel (to avoid the
confusing closeness of TodayPane and todaypane).

That's indeed confusing!

Looks like XUL supports |checked="false"| in cases like this, whereas in
HTML you have to remove the "checked" attribute (sigh). Elsewhere in these
patches you've used removeAttribute() for the false case. Should we do that
here as well for consistency and to anticipate an HTML future beyond XUL?

I think it's easier to just tackle it later.

Attachment #9090312 - Attachment is obsolete: true
Attachment #9090829 - Flags: review?(paul)
Comment on attachment 9090829 [details] [diff] [review]
bug1567424_broadcaster_lightning.patch

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

LGTM.
Attachment #9090829 - Flags: review?(paul) → review+

Remove the leftovers Paul noticed. There don't do anything anymore. It's all handled by setupSignatureItems()

Attachment #9090930 - Flags: review?(jorgk)
Attachment #9090930 - Flags: review?(jorgk) → review+

So land the lot?

Yeah. I can do that now, or when do you prefer?

Sorry, I didn't see the reply on time. I'll land it later, or you can after the next merge around 18:00 CEST (GMT+2) (if they do one).

Keywords: checkin-needed

I'll go ahead and land it. There was some minor thing I had to change, but with all the patches I forget exactly which patch it was.

Keywords: checkin-needed

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/7d0f5d3a4d33
remove broadcasters from calendar/: remove filterBroadcaster. r=pmorris
https://hg.mozilla.org/comm-central/rev/db4c1ff93628
remove broadcasters from calendar/: remove unifinder-todo-filter-broadcaster. r=pmorris
https://hg.mozilla.org/comm-central/rev/dcba32994a29
remove broadcasters from calendar/: remove calendarviewBroadcaster. r=pmorris
https://hg.mozilla.org/comm-central/rev/077f00ae2ac9
remove broadcaster usage from calendar/: remove modeBroadcaster. r=pmorris
https://hg.mozilla.org/comm-central/rev/cb573c0a6d09
remove some leftover broadcaster references. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 71
Regressions: 1602652
You need to log in before you can comment on or make changes to this bug.