Closed Bug 615897 Opened 9 years ago Closed 9 years ago

Use notification text rather than title for android tickerText

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec-)

VERIFIED FIXED
Tracking Status
fennec - ---

People

(Reporter: wesj, Assigned: wesj)

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

Attached file Fix (obsolete) —
Currently we use the Android notification title for ticker text (text shown in the notification bar when the notification first appears). For consistency this is always "Downloads" or "Addons" or something pertaining to the component that's notifying.

It would (usually?) be more useful to show the text which tells the user what is happening. i.e. "Downloading: logo.png", "logo.png has finished downloading" "Downloading addon" "Installation complete. Restart Required". Rather than requiring them to open the notification bar to see what has happened.
Attached patch Patch (obsolete) — Splinter Review
Marked as a patch
Attachment #494408 - Attachment is obsolete: true
Attachment #494411 - Flags: ui-review?(madhava)
Attachment #494411 - Flags: review?(blassey.bugs)
Comment on attachment 494411 [details] [diff] [review]
Patch

The AlertNotification class still names this parameter and the associated member aTitle and mTitle. Please make it all consistent.
Attachment #494411 - Flags: review?(blassey.bugs) → review-
flagging for blocking
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0-
Keywords: polish
We'd probably take a patch, but it doesn't block
I had this in for a bit and liked it. The text actually IS used as a title in AlertNotification when a progress bar is shown. The more detailed text is helpful as a title during progress, as it has the filename.

Do we want AlertNotification to take in two parameters, one to be used for the titles of progress notifications, and one for the ticker text? And if so, can we still leave the one which will be used as the Title with the names aTitle/mTitle?
Attached patch Patch v2Splinter Review
Now with more stuff. Defaults to using the text if we have something. Otherwise uses the title.
Attachment #494411 - Attachment is obsolete: true
Attachment #498339 - Flags: review?(blassey.bugs)
Attachment #494411 - Flags: ui-review?(madhava)
Comment on attachment 498339 [details] [diff] [review]
Patch v2


>+        super(aIcon, (aText.length() > 0) ? aText : aTitle, aWhen);

>+            view.setTextViewText(R.id.notificationTitle, (mText.length() > 0) ? mText : mTitle);
can mText ever be null?
Attachment #498339 - Flags: review?(blassey.bugs) → review+
This is across the bridge, and as far as I know we force these strings to not be null. Do you want an extra check, just to be safe?
(In reply to comment #8)
> This is across the bridge, and as far as I know we force these strings to not
> be null. Do you want an extra check, just to be safe?

nope, that's fine
Comment on attachment 498339 [details] [diff] [review]
Patch v2

android only.  make sure you test locally alot before pushing. zero regressions.
Attachment #498339 - Flags: approval2.0+
Pushed:

http://hg.mozilla.org/mozilla-central/rev/2e64d11f6885
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee: nobody → wjohnston
VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a1) Gecko/20110530 Firefox/7.0a1 Fennec/7.0a1 

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a2) Gecko/20110529 Firefox/6.0a2 Fennec/6.0a2 

Device: HTC Desire Z (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.