1246243 - Indicate that the "Reading List" folder is different

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_bookmarks_root_migrate6.png

Should be the same asset from bug 1234328, same padding of 15dp from the right, vertically centered.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_bookmarks_root_migrate6.png

We can use the Reader View icon here as an indicator that it's different to a traditional "folder" but also that it's related to Reader View.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_RLfolder.zip

Optimized these for use in the folder list item.

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1246243 - Use special icon for reading list folder r?liuche

This approach is extensible and would allow easy addition of special icons for e.g. the
screenshots folder.

Review commit: https://reviewboard.mozilla.org/r/45855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45855/

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8740655 [details]
MozReview Request: Bug 1246243 - Use special icon for reading list folder r?liuche

https://reviewboard.mozilla.org/r/45855/#review43445

r+ with some naming and comment updates.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:20
(Diff revision 1)
>  public class BookmarkFolderView extends TextView {
> -    private static final int[] STATE_OPEN = { R.attr.state_open };
> +    public enum FolderState {
> +        /**
> +         * A standard folder, i.e. a folder in a list of bookmarks and folders.
> +         */
> +        FOLDER(0),

Let's rename these so they're a little more explicit. What do you think about changing them to FOLDER, PARENT, READING_LIST ?

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:23
(Diff revision 1)
> +         * A standard folder, i.e. a folder in a list of bookmarks and folders.
> +         */
> +        FOLDER(0),
> +
> +        /**
> +         * A currently opened folder, i.e. this indicates that you are able to return to the previous

Update this comment - I think it makes more sense to refer to this as the parent folder versus "A currently opened folder" because clicking on it takes you back, and the text also says "Go back to ...".

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:41
(Diff revision 1)
> +        FolderState(final int state) { this.state = state; }
> +    }
> +
> +    private FolderState mState;
>  
> -    private boolean mIsOpen;
> +    private static final int[] STATE_OPEN = { R.attr.state_open };

I don't think you use this anymore because you reference the attr directly in the enum.

::: mobile/android/base/resources/values/attrs.xml:140
(Diff revision 1)
>               background is full alpha and we need to copy the background underneath. -->
>          <attr name="fadeBackgroundColor" format="dimension"/>
>      </declare-styleable>
>  
>      <declare-styleable name="BookmarkFolderView">
>          <attr name="state_open" format="boolean"/>

Same.

Comment 5

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/98e3ee7643cd665502c97d96b8c6f8369c781b5b
Bug 1246243 - Use book icon for reading list folder r=liuche

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98e3ee7643cd

Comment 7

[Sorina Florean [:sflorean]]

Verified as fixed in build 48.0a2 2016-04-27;
Device: Asus Transformer Pad (Android 4.2.1).