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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: antlam, Assigned: ahunt)
References
()
Details
Attachments
(7 files)
205.58 KB,
image/png
|
Details | |
177.23 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
91.88 KB,
image/jpeg
|
Details |
Noticed this was missing from Nightly last week: Reading List # items
Flags: needinfo?(ahunt)
Comment 1•8 years ago
|
||
It is too late to do this for 48, because it requires a string change.
Reporter | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
Here's a screenshot of this in action
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50863/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50863/
Comment 9•8 years ago
|
||
(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)
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51165/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51165/
Attachment #8749810 -
Flags: review?(liuche)
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/34b1700c8771af81becbb2bd1a1f577ce4537274 Bug 1269001 - Pre: Add strings for number of items in folder r=liuche
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6eafc23d044673e709bd215c4ab6511f8f552cb0 Bug 1269001 - Temporarily add string resources to UnusedResourcesUtil to satisfy lint r=me
Assignee | ||
Comment 24•8 years ago
|
||
(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)
Comment 25•8 years ago
|
||
Please include proper localization, https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34b1700c8771 https://hg.mozilla.org/mozilla-central/rev/6eafc23d0446
Comment 27•8 years ago
|
||
(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)
Assignee | ||
Comment 31•8 years ago
|
||
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.
Assignee | ||
Comment 32•8 years ago
|
||
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).
Assignee | ||
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
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/
Assignee | ||
Comment 35•8 years ago
|
||
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/
Assignee | ||
Comment 36•8 years ago
|
||
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/
Assignee | ||
Comment 37•8 years ago
|
||
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+
Assignee | ||
Comment 40•8 years ago
|
||
(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.
Comment 41•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4f0d003a90d1 https://hg.mozilla.org/integration/fx-team/rev/95739305c4d0 https://hg.mozilla.org/integration/fx-team/rev/c32981aa0f19 https://hg.mozilla.org/integration/fx-team/rev/94c6caa38706
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f0d003a90d1 https://hg.mozilla.org/mozilla-central/rev/95739305c4d0 https://hg.mozilla.org/mozilla-central/rev/c32981aa0f19 https://hg.mozilla.org/mozilla-central/rev/94c6caa38706
Assignee | ||
Updated•8 years ago
|
Comment 43•8 years ago
|
||
Verified as fixed using: Device: Moto X (Android 4.4) Build: Firefox for Android 49.0a1 (2016-05-17)
Assignee | ||
Updated•8 years ago
|
Attachment #8749810 -
Flags: approval-mozilla-aurora?
Comment 44•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/683a20539ada
status-firefox48:
--- → fixed
Comment 45•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(wkocher)
Flags: needinfo?(ahunt)
Assignee | ||
Comment 46•8 years ago
|
||
(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.)
Comment 47•8 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•