Closed Bug 1269001 Opened 8 years ago Closed 8 years ago

Reading List smart folder should indicate # of items inside

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: antlam, Assigned: ahunt)

References

()

Details

Attachments

(7 files)

Noticed this was missing from Nightly last week:

Reading List
# items
Flags: needinfo?(ahunt)
It is too late to do this for 48, because it requires a string change.
It looks like this was missed from the original design then. 

Barbara, let's get a card and get this in 49.
Flags: needinfo?(bbermes)
The string changes here are fairly simple, so we could potentially still request uplift - I'm not particularly familiar with the process though.

(2 strings: "1 item" and "N items")
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Flags: needinfo?(ahunt)
Here's a screenshot of this in action
We want to be able to show the numebr of items for certain folders (e.g.
the reading list smartfolder). The previous state list drawable was also
unnecessarily confusing, let's just reference the desired images directly.

We can do this largely by copying the existing TwoLinePageRow, modulo
the unneeded status / switch-to-tab icons.

Review commit: https://reviewboard.mozilla.org/r/50859/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50859/
Attachment #8749326 - Flags: review?(michael.l.comella)
Attachment #8749327 - Flags: review?(michael.l.comella)
Attachment #8749328 - Flags: review?(michael.l.comella)
For now we only need to support the bookmarks smartfolder, however
we might want to extend this to support "recent bookmarks" in future.

Review commit: https://reviewboard.mozilla.org/r/50861/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50861/
(In reply to Andrzej Hunt :ahunt from comment #4)
> The string changes here are fairly simple, so we could potentially still
> request uplift - I'm not particularly familiar with the process though.
> 
> (2 strings: "1 item" and "N items")

We should ask Delphine about the process here.
Flags: needinfo?(lebedel.delphine)
If this can get landed by tomorrow then we are ok, since it's still fairly early in the cycle and this is minimal change. thanks for asking!
Flags: needinfo?(lebedel.delphine)
Comment on attachment 8749326 [details]
MozReview Request: Bug 1269001 - Pre: use two line layout for BookmarkFolderView r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50859/diff/1-2/
Comment on attachment 8749327 [details]
MozReview Request: Bug 1269001 - Introduce BrowserDB.getBookmarkCountForFolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50861/diff/1-2/
Comment on attachment 8749328 [details]
MozReview Request: Bug 1269001 - Show number of items for reading list smartfolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50863/diff/1-2/
Comment on attachment 8749810 [details]
MozReview Request: Bug 1269001 - Post: remove now used strings from UnusedResourcesUtil r=me

https://reviewboard.mozilla.org/r/51165/#review47865
Attachment #8749810 - Flags: review?(liuche) → review+
Comment on attachment 8749810 [details]
MozReview Request: Bug 1269001 - Post: remove now used strings from UnusedResourcesUtil r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51165/diff/1-2/
Comment on attachment 8749326 [details]
MozReview Request: Bug 1269001 - Pre: use two line layout for BookmarkFolderView r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50859/diff/2-3/
Comment on attachment 8749327 [details]
MozReview Request: Bug 1269001 - Introduce BrowserDB.getBookmarkCountForFolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50861/diff/2-3/
Comment on attachment 8749328 [details]
MozReview Request: Bug 1269001 - Show number of items for reading list smartfolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50863/diff/2-3/
https://hg.mozilla.org/integration/fx-team/rev/34b1700c8771af81becbb2bd1a1f577ce4537274
Bug 1269001 - Pre: Add strings for number of items in folder r=liuche
Keywords: leave-open
Comment on attachment 8749810 [details]
MozReview Request: Bug 1269001 - Post: remove now used strings from UnusedResourcesUtil r=me

I'm requesting uplift of just the strings for this Bug - the remainder is still awaiting review. I'd like to uplift the strings ASAP as per Delphine's comments above.

Approval Request Comment
[Feature/regressing bug #]: /
[User impact if declined]: New reading list folder does not have an item count, meaning you have to open the folder to verify whether you have any reading list items.
[Describe test coverage new/current, TreeHerder]: manual testing.
[Risks and why]: This commit: low risk, string addition. Entire bug: medium risk: it adds a new and simple database query to count the number of items in a folder, and also refactors/simplifies the folder drawable state logic to allow directly setting the folder type.
[String/UUID change made/needed]: Two new strings: "1 item", "N items"
Attachment #8749810 - Flags: approval-mozilla-aurora?
The strings apparently broke lint tests on Android.

If you do a Ctrl-F for | severity="Error" | in https://public-artifacts.taskcluster.net/LNBNODSuQLKCXmdkCak6kQ/0/public/android/lint/lint-results-automationDebug.xml you can see the two errors:

<issue id="UnusedResources" severity="Error" message="The resource `R.string.bookmark_folder_items` appears to be unused" category="Performance" priority="3" summary="Unused resources" explanation="Unused resources make applications larger and slow down builds." errorLine1="  <string name="bookmark_folder_items">%d items</string>" errorLine2="          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~"><location file="/home/worker/workspace/build/src/obj-firefox/gradle/build/mobile/android/app/generated/source/preprocessed_resources/values/strings.xml" line="135" column="11"/></issue>

<issue id="UnusedResources" severity="Error" message="The resource `R.string.bookmark_folder_one_item` appears to be unused" category="Performance" priority="3" summary="Unused resources" explanation="Unused resources make applications larger and slow down builds." errorLine1="  <string name="bookmark_folder_one_item">1 item</string>" errorLine2="          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"><location file="/home/worker/workspace/build/src/obj-firefox/gradle/build/mobile/android/app/generated/source/preprocessed_resources/values/strings.xml" line="136" column="11"/></issue>



I'm not sure what the solution would be to fix this.
Flags: needinfo?(ahunt)
https://hg.mozilla.org/integration/fx-team/rev/6eafc23d044673e709bd215c4ab6511f8f552cb0
Bug 1269001 - Temporarily add string resources to UnusedResourcesUtil to satisfy lint r=me
(In reply to Wes Kocher (:KWierso) from comment #22)
> The strings apparently broke lint tests on Android.
> 
> If you do a Ctrl-F for | severity="Error" | in
> https://public-artifacts.taskcluster.net/LNBNODSuQLKCXmdkCak6kQ/0/public/
> android/lint/lint-results-automationDebug.xml you can see the two errors:

This should be fixed now with the commit above - I should have added the (temporarily unused) strings to UnusedResourcesUtil to avoid these warnings.
Flags: needinfo?(ahunt)
(In reply to Stefan Plewako [:stef] from comment #25)
> Please include proper localization,
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_and_Plurals

Sadly Android doesn't support plural forms.
Comment on attachment 8749326 [details]
MozReview Request: Bug 1269001 - Pre: use two line layout for BookmarkFolderView r?mcomella

https://reviewboard.mozilla.org/r/50859/#review48249

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:56
(Diff revision 3)
> +        setGravity(Gravity.CENTER_VERTICAL);
> +

nit: This should go in the xml.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:59
(Diff revision 3)
> +        // Merge layouts lose their padding, so set it dynamically.
> +        setPadding(0, 0, (int) getResources().getDimension(R.dimen.page_row_edge_padding), 0);

Are you sure that's true? I'd expect this to work if you put it on the parent in `bookmark_folder_row.xml`, but not in the actual `<merge>` tag.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:66
(Diff revision 3)
> +
> +        mTitle = (TextView) findViewById(R.id.title);
> +        mSubtitle = (TextView) findViewById(R.id.subtitle);
> +        mIcon =  (ImageView) findViewById(R.id.icon);
> +
> +        mSubtitle.setVisibility(GONE);

nit: set this in the xml

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:77
(Diff revision 3)
>          }
>  
> -        return drawableState;
> +        mSubtitle.setVisibility(GONE);
>      }

This won't compile.

::: mobile/android/base/resources/layout/bookmark_folder_row.xml:10
(Diff revision 3)
>  
>  <org.mozilla.gecko.home.BookmarkFolderView xmlns:android="http://schemas.android.com/apk/res/android"
>                                             style="@style/Widget.FolderView"
>                                             android:layout_width="match_parent"
>                                             android:paddingLeft="20dp"
>                                             android:drawablePadding="20dp"

I think you can remove this

::: mobile/android/base/resources/layout/two_line_folder_row.xml:13
(Diff revision 3)
> +               android:layout_width="@dimen/folder_bg"
> +               android:layout_height="@dimen/folder_bg"

nit: I think you should just write the `24dp` in here directly. Using resources (e.g. `dimen`) makes the layout hard to follow because you have to jump files so I only use them when necessary – e.g. re-use or the value changes with different configs.

::: mobile/android/base/resources/layout/two_line_folder_row.xml:37
(Diff revision 3)
> +
> +        <org.mozilla.gecko.widget.FadedSingleColorTextView android:id="@+id/subtitle"
> +                  style="@style/Widget.TwoLinePageRow.Url"
> +                  android:layout_width="match_parent"
> +                  android:layout_height="wrap_content"
> +                  android:maxLength="1024"

Why do we have a limit? In theory, we could have insanely large screens...

That being said, if we have a limit here, we should probably have a limit on the title view.
Attachment #8749326 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8749327 [details]
MozReview Request: Bug 1269001 - Introduce BrowserDB.getBookmarkCountForFolder r?mcomella

https://reviewboard.mozilla.org/r/50861/#review48253

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:890
(Diff revision 3)
> +    @Override
> +    public int getBookmarkCountForFolder(ContentResolver cr, long folderID) {
> +        if (folderID == Bookmarks.FAKE_READINGLIST_SMARTFOLDER_ID) {
> +            return getUrlAnnotations().getAnnotationCount(cr, BrowserContract.UrlAnnotations.Key.READER_VIEW);
> +        } else {
> +            throw new IllegalArgumentException("Retrieving bookmark count for folder with ID=" + folderID + " not supported yet");

nit: Is there a bug filed to support this? If so, you should add the bug # here.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:236
(Diff revision 3)
> +
> +    public int getAnnotationCount(ContentResolver cr, Key key) {
> +        final String countColumnname = "count";
> +        Cursor c = null;
> +        try {
> +            c = queryByKey(cr,

nit: you can move this outside of the try, allowing you to make `Cursor` final and preventing you from catch an Exception you may not have meant to.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:241
(Diff revision 3)
> +            c = queryByKey(cr,
> +                    key,
> +                    new String[] {
> +                            "COUNT(*) AS " + countColumnname
> +                    },
> +                    "");

nit: I think `null` is more correct here – the docs explicit mention it as an option.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:252
(Diff revision 3)
> +            if (c != null) {
> +                c.close();
> +            }
> +        }
> +
> +        return 0;

We don't catch an exception so this line isn't necessary. It's also error prone to leave it in because if we add a catch, the dev won't be warned by the compiler if they forget to handle the error case.
Attachment #8749327 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8749328 [details]
MozReview Request: Bug 1269001 - Show number of items for reading list smartfolder r?mcomella

https://reviewboard.mozilla.org/r/50863/#review48255

I'd like to see the background thread changes before r+.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:88
(Diff revision 3)
>      private void setTitle(String title) {
>          mTitle.setText(title);
>      }
>  
> +    private void setID(final int folderID) {
> +        for (final int id : FOLDERS_WITH_COUNT) {

nit: a Set [1] is more correct, but it's probably faster to iterate over an array given our small data size. I'd prefer the Set for correctness but if you want to do the array, comment it to explain why you used it.

[1]: You can use `Collections.unmodifiableSet`.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:91
(Diff revision 3)
>  
> +    private void setID(final int folderID) {
> +        for (final int id : FOLDERS_WITH_COUNT) {
> +            if (folderID == id) {
> +
> +                ThreadUtils.postToBackgroundThread(new Runnable() {

This code causes a memory leak and could potentially crash – it's holding onto an implicit reference to `this` and later updates the UI, which can throw if the Activity is not displayed.

Also, you should override [AsyncTask](https://www.youtube.com/watch?v=jtlRNNhane0&list=PLWz5rJ2EKKc9CBxr3BVjPTPoDPLdPIFCE&index=4) to do this more cleanly.

To avoid the memory leak/crash, you should use a `WeakReference` to the Context.
Attachment #8749328 - Flags: review?(michael.l.comella)
https://reviewboard.mozilla.org/r/50859/#review48249

> This won't compile.

In this case this works since we're in a View subclass. But it's probably confusing to rely on that (plus a few lines above I've use View.VISIBLE), and it probably helps to be consistent across places where we do need the prefix.
https://reviewboard.mozilla.org/r/50861/#review48253

> nit: Is there a bug filed to support this? If so, you should add the bug # here.

I don't think we want to support counts for arbitrary folders - this is mainly intended for development, i.e. when you add a new folder to the list in BookmarkFolderView this exception will remind you to add the necessary DB support. (I think I've seen mocks where the "recent bookmarks" smartfolder might show a count).
Comment on attachment 8749326 [details]
MozReview Request: Bug 1269001 - Pre: use two line layout for BookmarkFolderView r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50859/diff/3-4/
Attachment #8749810 - Attachment description: MozReview Request: Bug 1269001 - Pre: Add strings for number of items in folder r?liuche → MozReview Request: Bug 1269001 - Post: remove now used strings from UnusedResourcesUtil r=me
Attachment #8749328 - Flags: review?(michael.l.comella)
Comment on attachment 8749327 [details]
MozReview Request: Bug 1269001 - Introduce BrowserDB.getBookmarkCountForFolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50861/diff/3-4/
Comment on attachment 8749328 [details]
MozReview Request: Bug 1269001 - Show number of items for reading list smartfolder r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50863/diff/3-4/
Comment on attachment 8749810 [details]
MozReview Request: Bug 1269001 - Post: remove now used strings from UnusedResourcesUtil r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51165/diff/2-3/
https://reviewboard.mozilla.org/r/50863/#review48255

> nit: a Set [1] is more correct, but it's probably faster to iterate over an array given our small data size. I'd prefer the Set for correctness but if you want to do the array, comment it to explain why you used it.
> 
> [1]: You can use `Collections.unmodifiableSet`.

So I thought I could use an ArraySet here, which shouldn't make performance much worse, but that needs Android M. I've gone for a TreeSet since that should actually be faster if we do end up adding more items. I doubt the performance impact of either of these over a raw array is really worth worrying about?
https://reviewboard.mozilla.org/r/50859/#review48249

> In this case this works since we're in a View subclass. But it's probably confusing to rely on that (plus a few lines above I've use View.VISIBLE), and it probably helps to be consistent across places where we do need the prefix.

It wasn't this line, but it was syntactically incorrect – there were unmatched braces. Probably a version control mistake.
Comment on attachment 8749328 [details]
MozReview Request: Bug 1269001 - Show number of items for reading list smartfolder r?mcomella

https://reviewboard.mozilla.org/r/50863/#review49263

Nice!

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:92
(Diff revision 4)
>          mTitle.setText(title);
>      }
>  
> +    private static void updateItemCount(final WeakReference<TextView> textViewReference,
> +                                        final int folderID) {
> +        (new UIAsyncTask.WithoutParams<Integer>(ThreadUtils.getBackgroundHandler()) {

nit: I'd prefer this to be a static inner class rather than an anonymous class in a static method because if it gets refactored to be a non-static method, it's likely someone will forget to move the AsyncTask out of the method.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:131
(Diff revision 4)
> +
> +    private void setID(final int folderID) {
> +        if (FOLDERS_WITH_COUNT.contains(folderID)) {
> +            final WeakReference<TextView> subTitleReference = new WeakReference<TextView>(mSubtitle);
> +
> +            updateItemCount(subTitleReference, folderID);

nit: I think the previous implementation called setVisibility(GONE) before starting the async update, probably to clear any existing state from this potentially re-used subtitle view – do we still need that to avoid showing stale data and updating it asynchronously?
Attachment #8749328 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #39)
> > +    private void setID(final int folderID) {
> > +        if (FOLDERS_WITH_COUNT.contains(folderID)) {
> > +            final WeakReference<TextView> subTitleReference = new WeakReference<TextView>(mSubtitle);
> > +
> > +            updateItemCount(subTitleReference, folderID);
> 
> nit: I think the previous implementation called setVisibility(GONE) before
> starting the async update, probably to clear any existing state from this
> potentially re-used subtitle view – do we still need that to avoid showing
> stale data and updating it asynchronously?

I decided to remove that call because it caused flickering if we have a DB change after rendering.

It was most obvious on loading a new URL, and switching to a new tab:
1) We draw the list once
2) Page finishes loading -> history is updated -> DB fires a update notification -> Loader reloads bookmarks list
3) List is updated -> we set a new ID -> hide # of items -> we refetch the count -> show # of items (brief flicker)

I think the scenarios that we need to handle are:

1. Bookmarks reload, RL folder still in the same position -> don't hide the count, this will flicker
2. Reload / change of folder: RL folder no longer visible -> we hide the count immediately
3. Reload / change of folder: RL folder is replaced by a different folder with a count -> don't hide, this will flicker (not supported yet, but might arrive sooner or later)

We could optimise case (1) by checking if the ID hasn't changed, and only hide the count row if the ID has changed, but then that causes issues for case 3. (We still want to reload the count, even in case 1, since this refresh could have been caused by sync, which could add items to the RL folder - we can only optimise the hiding). Either way we're choosing between momentarily stale data and a flicker: to me the flicker was much more annoying, and since the data will almost never become stale during a reload I optimised for removing the flicker.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 49.0a1 (2016-05-17)
Depends on: 1274238
Attachment #8749810 - Flags: approval-mozilla-aurora?
Tested using ONE A2001 (Android 5.1.1) on latest Aurora: 48.0a2 (2016-05-29), Reading List smart folder does not indicate the number of items inside.
Flags: needinfo?(wkocher)
Flags: needinfo?(ahunt)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #45)
> Tested using ONE A2001 (Android 5.1.1) on latest Aurora: 48.0a2
> (2016-05-29), Reading List smart folder does not indicate the number of
> items inside.

Sorry, the flags got messed up here. This bug did not land on Aurora. (One of the patches from this bug did get uplifted for the purposes of Bug 1267580, however we didn't uplift the part that displays folder counts.)
Flags: needinfo?(wkocher)
Flags: needinfo?(ahunt)
QA work done by Teo is tracked here: https://wiki.mozilla.org/QA/Fennec/Reading_List_smart_folder_should_indicate_number_of_items_inside
QA Contact: teodora.vermesan
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.