Closed Bug 1358268 Opened 4 years ago Closed 3 months ago

New E-Mail alert doesn't work

Categories

(Thunderbird :: General, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME
Thunderbird 55.0

People

(Reporter: jorgk-bmo, Unassigned)

References

Details

(Whiteboard: [patchlove])

Attachments

(1 file)

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.
This comes from bug 1352069:
https://hg.mozilla.org/mozilla-central/rev/77760e0b239f#l10.32

The new preference is now called toolkit.cosmeticAnimations.enabled.
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")) {
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
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.
https://hg.mozilla.org/comm-central/rev/ef68c4b126888f921f1525b027bf289beeb04b86

Looks like the purists want to take this further. For now, make Daily usable again.
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
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.
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [closeme 2017-06-01]
Target Milestone: --- → Thunderbird 55.0
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?
(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.
(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.
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.
(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?
Duplicate of this bug: 1358404
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.
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.
what's the next step?
Whiteboard: [closeme 2017-06-01]
Whiteboard: [patchlove]
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.