1508209 - remove broadcasters from mail/components/addrbook/

Status: RESOLVED FIXED

(Thunderbird :: Address Book, task)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1489447 +++

Remove <broadcaster> from mail/components/addrbook/

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1508209_de_broadcaster_cmd_properties-flavors.patch

Comment 2

[Magnus Melin [:mkmelin]]

Ah, noticed there's still one left in addrbook

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1508209_de_broadcaster_cmd_properties-flavors_etc.patch

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1508209_de_broadcaster_cmd_properties-flavors.patch

Comment 5

[:aceman]

Comment on attachment 9026364 [details] [diff] [review]
bug1508209_de_broadcaster_cmd_properties-flavors.patch

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

Oh the progress from m-c, more open-coding again.

::: mail/components/addrbook/content/abCommon.js
@@ +141,3 @@
>          }
> +        let enabled = (selectedDir != null);
> +        document.querySelectorAll("[command=cmd_properties]").forEach(e => {

This pattern is repeated 3 times in this patch, can't it be put into the goSetLabelAccesskeyTooltiptext function? Or a more universal one, that would just take the attrs object and set the attributes?

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9026364 [details] [diff] [review]
bug1508209_de_broadcaster_cmd_properties-flavors.patch

Looks like I found a more capable reviewer ;-)

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #5)
> This pattern is repeated 3 times in this patch, can't it be put into the
> goSetLabelAccesskeyTooltiptext function? 

I'd try to avoid helper functions that are complex and offers little value. Background knowledge of how the function works or that it exists shouldn't really be needed to look at this code.

goSetLabelAccesskeyTooltiptext is/was really quite horrible... You may note that the equivalent number of lines of code actually halved by opening it up. 

I believe the two controllers should be possible to unify (at least partly) to just one, but that is out of scope for this bug. The rabbit hole is deep...

Comment 8

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #7)
> I'd try to avoid helper functions that are complex and offers little value.
> Background knowledge of how the function works or that it exists shouldn't
> really be needed to look at this code.

The point of helper functions is to contain complex code that you do then do not need to copy elsewhere and introduce bugs and deviations.
 
> goSetLabelAccesskeyTooltiptext is/was really quite horrible... You may note
> that the equivalent number of lines of code actually halved by opening it
> up. 

It didn't really, the function just had comment, while yours doesn't. e.getAttribute(attrs[attr])) isn't really obvious at first sight. Also, there are many more broadcasters to kill, will you copy this code to replace each of them?
A function broadcast_attributes([element set], {attribute set}) would be a simple and short replacement.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #8)
> (In reply to Magnus Melin [:mkmelin] from comment #7)
> > I'd try to avoid helper functions that are complex and offers little value.
> > Background knowledge of how the function works or that it exists shouldn't
> > really be needed to look at this code.
> 
> The point of helper functions is to contain complex code that you do then do
> not need to copy elsewhere and introduce bugs and deviations.

Yes, but a helper function needs to be understood, and then for each new case evaluated if it fits. Then someone want to expand functionality, and you need to go back and audit callers.

> > goSetLabelAccesskeyTooltiptext is/was really quite horrible... You may note
> > that the equivalent number of lines of code actually halved by opening it
> > up. 
> 
> It didn't really, the function just had comment, while yours doesn't.

Comments that need to be thoroughly understood is still LOC. There's really no need to comment when the code is fairly obvious.

> e.getAttribute(attrs[attr])) isn't really obvious at first sight. 

I'll see if I can make it even more clear.

> Also,
> there are many more broadcasters to kill, will you copy this code to replace
> each of them?

These are the last ones for mail/ actually. Sharing a function means global sharing, and atm, there would be only two callers, which if we can unify these controllers would be reduced to one call.

> A function broadcast_attributes([element set], {attribute set}) would be a
> simple and short replacement.

In one way, but it's still mental overhead given the little usage. I'm open to doing something like this if we end up with too many open coded cases.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1508209_de_broadcaster_cmd_properties-flavors.patch

Using Object.entries for more clarity

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/214b702ef343
remove broadcasters from mail/components/addrbook/. r=aceman