Closed Bug 1205236 Opened 5 years ago Closed 5 years ago

History Panel: Update empty state

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: sebastian, Assigned: alex_johnson)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Follow-up of bug 1142171.

Our empty state for the history panel says: "Websites you visited most recently show up here.". This is not 100% correct for the new split-panel view introduced in bug 1142171. A history category can be empty because website have not been visited in this time frame.
No longer depends on: onboarding-defaults
I'd like to pick this up.
Assignee: nobody → me
Status: NEW → ASSIGNED
Attachment #8703249 - Attachment is obsolete: true
Attachment #8703249 - Flags: feedback?(s.kaspari)
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29329/diff/1-2/
Blocks: 1131119
Attached image Screenshot
Comment on attachment 8703295 [details]
Screenshot

Flagging antlam for UX feedback
Attachment #8703295 - Flags: feedback?(alam)
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

https://reviewboard.mozilla.org/r/29329/#review26355

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:98
(Diff revision 2)
> +    private boolean isCursorEmpty = false;

Do we need this? This boolean is only checked in resetEmptyText() - Shouldn't this only be called if the cursor is empty?

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:335
(Diff revision 2)
> +    private String getEmptyText() {
> +        String result = getMostRecentSectionTitle(selected);
> +        if (result.matches(".*7.*")) {
> +            result = (getString(R.string.in_the) + " " + result).toLowerCase();
> +        }
> +        else if (selected.toString().matches(".*SIX_MONTH.*")) {
> +            result = getString(R.string.history_six_months_ago);
> +        }
> +        else if (selected.toString().matches(".*MONTH.*")) {
> +            result = getString(R.string.in) + " " + result;
> +        }
> +        else {
> +            result = result.toLowerCase();
> +        }
> +        return result;
> +    }

This sentence composition works for english but is not going to work in a lot of other languages. You will probably have to create separate strings like we did for the sections.
Attachment #8703251 - Flags: review?(s.kaspari)
Comment on attachment 8703295 [details]
Screenshot

heh, nice!
Attachment #8703295 - Flags: feedback?(alam) → feedback+
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29329/diff/2-3/
Attachment #8703251 - Flags: review?(s.kaspari)
https://reviewboard.mozilla.org/r/29329/#review26355

> Do we need this? This boolean is only checked in resetEmptyText() - Shouldn't this only be called if the cursor is empty?

Ah.  I did not see that updateUiFromCursor() gets called after cursor transitions.  I was also calling it in the load() method as well.
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

https://reviewboard.mozilla.org/r/29329/#review27023

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:342
(Diff revision 3)
> +                    result.toLowerCase());

You are manually inserting today/yesterday/most recently into the string and run toLowerCase() on it. I'm not sure if this will create a valid translation for all languages. Let's bring in Axel and use his experience. :)
Attachment #8703251 - Flags: review?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Comment on attachment 8703251 [details]
> MozReview Request: Bug 1205236 - History Panel: Show section title in empty
> state. r?sebastian
> 
> https://reviewboard.mozilla.org/r/29329/#review27023
> 
> ::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:342
> (Diff revision 3)
> > +                    result.toLowerCase());
> 
> You are manually inserting today/yesterday/most recently into the string and
> run toLowerCase() on it. I'm not sure if this will create a valid
> translation for all languages. Let's bring in Axel and use his experience. :)

@Axel: For this screen[1] we basically have the following variations of strings:

* Websites you visited <today / yesterday / most recently> show up here.
* Websites you visited in <January / February / March / ..> show up here.
* Websites you visited in the last seven days show up here.
* Websites you visited more than six months ago show up here.

What's the best way to split them up for translating? Can we insert the names of months dynamically? How about "today", "yesterday" and "most recently"?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8703295
Flags: needinfo?(l10n)
(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> * Websites you visited <today / yesterday / most recently> show up here.
I would go with 3 separate strings here (keeping in mind the end of this comment).

> * Websites you visited in <January / February / March / ..> show up here.

The current implementation definitely doesn't work:
* Case: Italian (and several other European languages) don't use uppercase for a month name in the middle of a sentence.
* Declension: I assume the month you get from Android is some sort of nominative, for example 'in X' would require an ablative form in Latin.

Is there any chance to dramatically simplify this and display 'Websites you visited in the selected timeframe/period show up here."?
Flags: needinfo?(l10n)
Attachment #8703251 - Attachment description: MozReview Request: Bug 1205236 - History Panel: Show section title in empty state. r?sebastian → MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r?sebastian
Attachment #8703251 - Flags: review?(s.kaspari)
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29329/diff/3-4/
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

https://reviewboard.mozilla.org/r/29329/#review28145

r+ after addressing the mentioned points. Great patch! Much simpler now. :)

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:313
(Diff revision 4)
> +            if(selected == null || mRangeAdapter == null || mRangeList == null) {

NIT: if _space_ {

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java:315
(Diff revision 4)
> +            }
> +            else {

NIT: } else {

::: mobile/android/base/locales/en-US/android_strings.dtd:525
(Diff revision 4)
> +<!ENTITY home_selected_empty "Websites you visited in the selected timeframe/period show up here.">

We probably should decide to use either timeframe or period here. Flag antlam if you want to get help making a decision.
Attachment #8703251 - Flags: review?(s.kaspari) → review+
Comment on attachment 8703251 [details]
MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29329/diff/4-5/
Attachment #8703251 - Attachment description: MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r?sebastian → MozReview Request: Bug 1205236 - History Panel: A more appropriate empty state title. r+sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15d670dd8e15
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.