1358268 - New E-Mail alert doesn't work

Status: RESOLVED WORKSFORME

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

New E-Mail arlert doesn't work.

Leaves an empty box.

Console says:
NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]  newmailalert.js:128

  if (gUserInitiated ||
      Services.prefs.getBoolPref("alerts.disableSlidingEffect")) {
    alertContainer.setAttribute("noanimation", true);
    setTimeout(closeAlert, gOpenTime);
    return;
  }


I'm using today's Daily, the one before of 2017-04-16 worked, IIRC.

Let's fix this since it spoils all the fun of using Daily with that ugly box.

Comment 1

[Jorg K (CEST = GMT+2)]

This comes from bug 1352069:
https://hg.mozilla.org/mozilla-central/rev/77760e0b239f#l10.32

The new preference is now called toolkit.cosmeticAnimations.enabled.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1358268-animation.patch

Straight port of this:

https://hg.mozilla.org/mozilla-central/rev/77760e0b239f#l14.12
-    if (Services.prefs.getBoolPref("alerts.disableSlidingEffect")) {
+    if (!Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

Comment 3

[:aceman]

Thanks for analysing it.

We need to decide if this animation falls into the cosmetic group and if we want to hang onto the new pref.
If yes, we can also do a simple migration of the pref value as m-c has done.
If not we can initialize the old alerts.disableSlidingEffect in out mailnews.js.

Comment 4

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/ef68c4b126888f921f1525b027bf289beeb04b86

Looks like the purists want to take this further. For now, make Daily usable again.

Comment 5

[:aceman]

Let's leave some time to let anybody step in if anything more needs to be done (e.g. my comment 3). Otherwise we can close this bug in a month or so.

Comment 6

[Jim Porter (:squib)]

Sorry, I didn't realize TB used this. I would have given a heads-up otherwise.

(In reply to :aceman from comment #3)
> We need to decide if this animation falls into the cosmetic group and if we
> want to hang onto the new pref.

It does. Cosmetic animations are anything that doesn't indicate a status effect (e.g. a little "ping" when an action completes).

(In reply to Jorg K (GMT+2) from comment #4)
> Looks like the purists want to take this further. For now, make Daily usable
> again.

Can you elaborate? What do the purists want to do?

Comment 7

[:aceman]

(In reply to Jim Porter (:squib) from comment #6)
> Can you elaborate? What do the purists want to do?

So if we want the new pref name, we can do automatic migration of the value from the old pref to the new one.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Jim Porter (:squib) from comment #6)
> Can you elaborate? What do the purists want to do?
I don't know, you'd have to ask them. But by the looks of it a migration from alerts.disableSlidingEffect to toolkit.cosmeticAnimations.enabled. I wouldn't bother.

Comment 9

[Jim Porter (:squib)]

I'm the one who wrote the Firefox patch to change this. I'm not sure what your concerns are though.

For Firefox, the goal is to have a single pref to disable animations (largely for low-spec machines, but also for accessibility). `toolkit.cosmeticAnimations.enabled` is what we're calling it to distinguish it from other animations that we can't really disable (at least, not right now) such as the loading spinner or the download notification.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Jim Porter (:squib) from comment #9)
> I'm not sure what your concerns are though.
I believe Aceman is concerned that the preference value will be lost in TB. You've written a migration:
https://hg.mozilla.org/mozilla-central/rev/77760e0b239f#l6.31

Personally I don't understand how that can work if the preference has already been removed. What am I missing?

Comment 12

[Jorg K (CEST = GMT+2)]

Aceman said in a PM:

For users that toggled the value from the default, the pref still does exist. But we must fetch it inside a try/catch as it will throw for users that don't have it.

Comment 13

[Jim Porter (:squib)]

You don't need a try/catch. nsIPrefBranch.getBoolValue (and the other get methods) lets you specify a default value for when the pref doesn't exist. See my patch for how to do it.

Comment 14

[Wayne Mery (:wsmwk)]

what's the next step?

Comment 15

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/ec64095d118d299477d3e5f9fd2dc11912d0b573/mailnews/base/content/newmailalert.js#137
Not an issue anymore.