Closed Bug 1508209 Opened 11 months ago Closed 11 months ago

remove broadcasters from mail/components/addrbook/

Categories

(Thunderbird :: Address Book, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 65.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file, 3 obsolete files)

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

Remove <broadcaster> from mail/components/addrbook/
Attachment #9025995 - Flags: review?(jorgk)
Ah, noticed there's still one left in addrbook
Attachment #9025995 - Attachment is obsolete: true
Attachment #9025995 - Flags: review?(jorgk)
Attachment #9026015 - Flags: review?(jorgk)
Attachment #9026015 - Attachment is obsolete: true
Attachment #9026015 - Flags: review?(jorgk)
Attachment #9026364 - Flags: review?(jorgk)
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 on attachment 9026364 [details] [diff] [review]
bug1508209_de_broadcaster_cmd_properties-flavors.patch

Looks like I found a more capable reviewer ;-)
Attachment #9026364 - Flags: review?(jorgk) → review?(acelists)
(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...
(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.
(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.
Using Object.entries for more clarity
Attachment #9026364 - Attachment is obsolete: true
Attachment #9026364 - Flags: review?(acelists)
Attachment #9027832 - Flags: review?(acelists)
Attachment #9027832 - Flags: review?(acelists) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/214b702ef343
remove broadcasters from mail/components/addrbook/. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.