1318852 - Make labels of cmd_properties action-oriented and context-sensitive, e.g. "Edit Contact", "Edit List", etc. (port Thunderbird bug 1310442)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[rsx11m]

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

Changes/additions from mail/components/addrbook/content need to be ported to suite/ given https://hg.mozilla.org/comm-central/diff/71581aa20801/mailnews/addrbook/content/abResultsPane.js

(Quoting Frank-Rainer Grahl from bug 1315924 comment #2)
> Hmm it looks like it was updated probably by TB without porting all changes
> / strings to SeaMonkey. Maybe this is the problem. rsx11m could you check it?
> 
> Timestamp: 11/19/2016 12:30:54 PM
> Error: ReferenceError: goSetLabelAccesskeyTooltiptext is not defined
> Source File: chrome://messenger/content/addressbook/abResultsPane.js
> Line: 444
> 
> Timestamp: 11/19/2016 12:30:54 PM
> Error: An error occurred updating the cmd_properties command: [Exception...
> "[JavaScript Error: "goSetLabelAccesskeyTooltiptext is not defined" {file:
> "chrome://messenger/content/addressbook/abResultsPane.js" line:
> 444}]'[JavaScript Error: "goSetLabelAccesskeyTooltiptext is not defined"
> {file: "chrome://messenger/content/addressbook/abResultsPane.js" line:
> 444}]' when calling method: [nsIController::isCommandEnabled]"  nsresult:
> "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS
> frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line
> 84"  data: yes]
> Source File: chrome://global/content/globalOverlay.js
> Line: 89

(Quoting rsx11m from bug 1315924 comment #3)
> Bug 1310442 - Make labels of cmd_properties action-oriented and
> context-sensitive, e.g. "Edit Contact", "Edit List", etc. (consistent with
> improved dialogue titles from bug 1312262).
> 
> https://hg.mozilla.org/comm-central/rev/71581aa20801 only pushed strings for
> mail/ but not suite/.
> Confirming per code review.

Comment 1

[:aceman]

Accept my apologies for the bustage.
It would be great if the goSetLabelAccesskeyTooltiptext() could be placed at some shared place.

Comment 2

[rsx11m]

Attachment: Hotfix for suite

Since this was already merged into comm-aurora and we won't be able to port the string changes there, this patch simply disables the change for suite/ until a proper solution is found here.

Comment 3

[:aceman]

Comment on attachment 8812506 [details] [diff] [review]
Hotfix for suite

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

Could you just check for the existence of the function (typeof goSetLabelAccesskeyTooltiptext == "function") to avoid the preprocessing?

Comment 4

[rsx11m]

Attachment: Alternative patch

Like this? I didn't test this version...

Comment 5

[:aceman]

Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

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

Thanks, works for me in TB. Needs testing in SM.

::: mailnews/addrbook/content/abResultsPane.js
@@ +414,5 @@
>            }
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")

Missing { at the end.

@@ +415,5 @@
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")
> +          let labelAttr = "valueGeneric";

I assume this is a temporary measure and you will fix this shortly for SM in the proper way, so please do not reindent the whole block unnecessarily (you would then reindent it back later).

Comment 6

[Frank-Rainer Grahl (:frg)]

>> Since this was already merged into comm-aurora and we won't be able to port the string changes there,

Its still early in the c-a cycle and 52 is the next ESR so maybe we could get away with string changes this time only?

FRG

Comment 7

[rsx11m]

I won't be able to do anything this week.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

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

::: mailnews/addrbook/content/abResultsPane.js
@@ +414,5 @@
>            }
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")

Can't you just move the function from mail/ to mailnews/?

Comment 9

[:aceman]

I think not because the function calls the new strings and those are not in SM yet (at least not in 52 branch).

Comment 10

[Ian Neal]

Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

[Triage Comment]
r/a=me

Comment 11

[rsx11m]

Attachment: Hotfix as checked in (v3) [comment #11]

Thanks for the reviews, comment #5 addressed (no indentation).
Pushed as https://hg.mozilla.org/comm-central/rev/0b822264879a

Comment 12

[rsx11m]

Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

I've moved this to MailNews Core given that the final solution will involve moving that function on trunk. I won't get to this before January.

For SeaMonkey 2.49, we'll need this on aurora where it's essentially NPOTB for Thunderbird 52, but Joerg probably needs to re-approve (and will check in).

Approval Request Comment
[Feature/Bug causing the regression]: bug 1310442
[User impact if declined]: Bustage for SeaMonkey

Comment 13

[Ian Neal]

Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Think you meant this flag.

Comment 14

[rsx11m]

(In reply to Ian Neal from comment #13)
> Flags: approval-mozilla-aurora? → approval-comm-aurora?

Sorry, always screwing that up...  :-\

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

I'm really not so happy with this solution. You should have added a comment to say that this is a temporary measure (without proper indentation) and pointed to a bug number.

I will take the liberty to land a comment with rs=documentation-only.

Comment 16

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0b822264879a - Hotfix landed in comment #11
https://hg.mozilla.org/comm-central/rev/7c81d51a19386dfe7d2c24b2eac698d20b0af6df - Hotfix comment.

Comment 17

[rsx11m]

(In reply to Jorg K (GMT+1) from comment #15)
> I'm really not so happy with this solution. You should have added a comment
> to say that this is a temporary measure (without proper indentation) and
> pointed to a bug number.

Makes perfectly sense, apologies for that omission.
I assume that you'll land this on comm-aurora.

Comment 18

[Jorg K (CEST = GMT+2)]

Yep. I landed on Aurora yesterday, so unless you're in a hurry, it will wait a few days. How/where do you build SM, BTW?

Comment 19

[rsx11m]

No rush, next weekend will be fine. Building SM works the same way as TB, just change the target application from "mail" to "suite" (recently however trunk was frequently busted due to changes on mozilla-central). I usually build on 64-bit Linux, but use Windows for UI-relevant changes too.

Comment 20

[Jorg K (CEST = GMT+2)]

Aurora (TB 52/SM 2.49):
Hotfix:
https://hg.mozilla.org/releases/comm-aurora/rev/41cad873a9a3a0784c8d79cfcad3b7f1789cbb63
https://hg.mozilla.org/releases/comm-aurora/rev/0a6c3497c1e0736bfbc46ff0c49f844bd70e0023

Comment 21

[Jorg K (CEST = GMT+2)]

Marking this fixed for now so the Aurora query doesn't pick this up.

Comment 22

[Wayne Mery (:wsmwk)]

STM we should close this and make a new bug?

Comment 23

[Geoff Lankow (:darktrojan)]

This code disappeared a long time ago. I don't see anything left to do.