If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve experience when attempting to add a page that already exists in the Reading List

VERIFIED FIXED in Firefox 35

Status

()

Firefox for Android
Overlays
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: TeoVermesan, Assigned: ckitching)

Tracking

34 Branch
Firefox 35
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

138.43 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Build: Firefox for Android 34.0a1 (2014-09-02)

Steps to reproduce:
1. Open news.google.com
2. Go to Custom Menu -> Share -> Add to Nightly
3. Choose "Add to Reading List"
4. Repeat step 3

Actual result:
- After step 4, a pop-up notification appears: "Page added to your reading list".

Expected result:
- After step 4, a pop-up notification should appear: "Page already added to your reading list"

Note: The page doesn't appear duplicate in reading list.
This is not possible as this is outside the browser.
Component: General → Overlays
(Assignee)

Comment 2

3 years ago
(In reply to Aaron Train [:aaronmt] from comment #1)
> This is not possible as this is outside the browser.

Actually, this is possible: we can query the database to find out if something is already on the reading list (or in bookmarks) and take action.

But what action?

Show a different icon in the overlay? Hide the button? Do as described in comment 1 and just show a different toast? Something else...?
Flags: needinfo?(ywang)
(Assignee)

Updated

3 years ago
QA Contact: chriskitching
(Assignee)

Updated

3 years ago
Assignee: nobody → chriskitching
QA Contact: chriskitching
I think our best option is for the "Add to Reading List" button to be inactive, labeled "In Reading List" or "Already in Reading List". (The former might be enough if it's greyed out.)

Omitting the button would be confusing -- looks like a bug.

Showing a "already in reading list" toast after the action makes the user feel foolish, not empowered.

It's fine to refresh the label in an AsyncTask after immediately showing the UI, rather than blocking; it'll return quickly enough. If the user somehow triggers the add anyway, show the same "Already in Reading List" label.

Yuan, let me know if you disagree!
Hardware: ARM → All
Summary: Adding multiple times a page to reading list via share overlay should be announced by a pop-up → Improve experience when attempting to add a page that already exists in the Reading List
(Assignee)

Comment 4

3 years ago
Created attachment 8485349 [details] [diff] [review]
Disable buttons when items are already on lists.

Warmup intents now take urls/titles as parameters (which makes some sense now we reinitialise for each share), causing the dispatch of UI update events which notify clients of the already-on-list-ness of the page.
Currently, disabledness looks like this:

https://www.dropbox.com/s/zu0v2cnc60s4mjd/Disabled.png?dl=0

Thoughts?
Attachment #8485349 - Flags: review?(rnewman)
Comment on attachment 8485349 [details] [diff] [review]
Disable buttons when items are already on lists.

Review of attachment 8485349 [details] [diff] [review]:
-----------------------------------------------------------------

Fix up the horribly long commit message!

Did you see LocalBrowserDB.getItemFlags?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +99,5 @@
>       used in the share overlay menu to select an action. It is the verb
>       "to bookmark", not the noun "a bookmark". -->
>  <!ENTITY overlay_share_bookmark_btn_label "Bookmark">
>  <!ENTITY overlay_share_reading_list_btn_label "Add to Reading List">
> +<!ENTITY overlay_share_bookmark_btn_label_already "Already a Bookmark">

Already bookmarked

@@ +100,5 @@
>       "to bookmark", not the noun "a bookmark". -->
>  <!ENTITY overlay_share_bookmark_btn_label "Bookmark">
>  <!ENTITY overlay_share_reading_list_btn_label "Add to Reading List">
> +<!ENTITY overlay_share_bookmark_btn_label_already "Already a Bookmark">
> +<!ENTITY overlay_share_reading_list_btn_label_already "Already on Reading List">

Already in Reading List

::: mobile/android/base/overlays/OverlayConstants.java
@@ +23,5 @@
>       *
>       * Intent parameters:
>       *
> +     * $EXTRA_URL: URL of page to share.
> +     * $EXTRA_TITLE: Title of page to share

Be consistent: closing full stop or not.

::: mobile/android/base/overlays/service/OverlayActionService.java
@@ +92,3 @@
>          shareTypes.clear();
>  
> +        ShareData initialisationData = ShareData.fromIntent(intent);

final

::: mobile/android/base/overlays/service/ShareData.java
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

file, You can…

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +18,5 @@
>  
>  public class AddBookmark extends ShareMethod {
>      private static final String LOGTAG = "GeckoAddBookmark";
>  
> +    // The UI event intent extra value key indicating if the a page is already bookmarked.

"the a" => the

@@ +19,5 @@
>  public class AddBookmark extends ShareMethod {
>      private static final String LOGTAG = "GeckoAddBookmark";
>  
> +    // The UI event intent extra value key indicating if the a page is already bookmarked.
> +    public static final String ALREADY_BOOKMARKED = "ALREADY_BOOKMARKED";

ALREADY_HANDLED?

@@ +46,3 @@
>          super(context);
> +
> +        // Broadcast a UI event expressing if the URL is already on the reading list.

Cloned comment?

@@ +46,4 @@
>          super(context);
> +
> +        // Broadcast a UI event expressing if the URL is already on the reading list.
> +        LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);

final

@@ +49,5 @@
> +        LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> +
> +        final boolean alreadyBookmarked = browserDB.isBookmark(context.getContentResolver(), initData.url);
> +
> +        Intent uiStateIntent = new Intent(OverlayConstants.SHARE_METHOD_UI_EVENT);

final

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +28,5 @@
>  public class AddToReadingList extends ShareMethod {
>      private static final String LOGTAG = "GeckoAddToReadingList";
>  
> +    // The UI event intent extra value key indicating if the a page is already on the reading list.
> +    public static final String ALREADY_ON_READING_LIST = "ALREADY_ON_READING_LIST";

Reuse ALREADY_HANDLED. Put it in ShareMethod.

@@ +61,3 @@
>          super(context);
> +
> +        // Broadcast a UI event expressing if the URL is already on the reading list.

expressing that the URL

@@ +67,5 @@
> +
> +        Intent uiStateIntent = new Intent(OverlayConstants.SHARE_METHOD_UI_EVENT);
> +        uiStateIntent.putExtra(ALREADY_ON_READING_LIST, alreadyOnReadingList);
> +        uiStateIntent.putExtra(OverlayConstants.EXTRA_SHARE_METHOD, (Parcelable) Type.ADD_TO_READING_LIST);
> +        LocalBroadcastManager.getInstance(context).sendBroadcast(uiStateIntent);

All the same comments apply.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +96,5 @@
> +        bookmarkBtn.setText(getResources().getString(R.string.overlay_share_bookmark_btn_label_already));
> +        bookmarkBtn.setEnabled(false);
> +    }
> +
> +    private void handleAddToReadingListUIEvent(Intent intent) {

Can we share some logic here?

@@ +160,5 @@
>          title = subjectText;
>          url = pageUrl;
>  
> +        // Have the service start any initialisation work that's necessary for us to show the correct
> +        // UI. The results of such work will come in via the BroadcastListener.

Do we really need a broadcast here, rather than either an AsyncTask or a straightforward callback?
Attachment #8485349 - Flags: review?(rnewman) → feedback+
(Assignee)

Comment 6

3 years ago
(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 8485349 [details] [diff] [review]
> Disable buttons when items are already on lists.
> 
> Review of attachment 8485349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix up the horribly long commit message!
> 
> Did you see LocalBrowserDB.getItemFlags?

No, but it's excellent.

> @@ +160,5 @@
> >          title = subjectText;
> >          url = pageUrl;
> >  
> > +        // Have the service start any initialisation work that's necessary for us to show the correct
> > +        // UI. The results of such work will come in via the BroadcastListener.
> 
> Do we really need a broadcast here, rather than either an AsyncTask or a
> straightforward callback?

So, I think the high level question is this: how much do we want to keep our nice decoupledness (remembering the point of the UI broadcast event mechanism was to simplify adding more UI clients of the service in the future).

We could bolt the queries into ShareDialog and do a boring normal callback, but now the result isn't exposed in a way that other clients can read it. This might have implications when we come to add the VIEW handler or whatnot.
On the other hand, it would make the work right now vastly simpler... What do you think?
> We could bolt the queries into ShareDialog and do a boring normal callback,
> but now the result isn't exposed in a way that other clients can read it.
> This might have implications when we come to add the VIEW handler or whatnot.
> On the other hand, it would make the work right now vastly simpler... What
> do you think?

Do you really have to ask?
(Assignee)

Comment 8

3 years ago
Created attachment 8485365 [details] [diff] [review]
Nicer experience when dealing with items already bookmarked/reading-listified on overlays
Attachment #8485365 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8485349 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Well, that got a lot shorter.
(Assignee)

Comment 10

3 years ago
(In reply to Richard Newman [:rnewman] from comment #5)

> ::: mobile/android/base/overlays/service/ShareData.java
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> file, You can…

Are you complaining about the text flowing or the capitalisation? If the latter, that's the way it is all over the codebase, alas.

I'll fix both...

One of these days I'll have a patch that doesn't lead me to reconfigure my file templates. :P
(Assignee)

Updated

3 years ago
Depends on: 1063887
(Assignee)

Comment 11

3 years ago
Created attachment 8485366 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays
Attachment #8485366 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8485365 - Attachment is obsolete: true
Attachment #8485365 - Flags: review?(rnewman)
(In reply to Chris Kitching [:ckitching] from comment #10)

> Are you complaining about the text flowing or the capitalisation? If the
> latter, that's the way it is all over the codebase, alas.
> 
> I'll fix both...

No, this is a legal template. "You" is correct. For some reason you have a very old license template with incorrect wrapping.

https://www.mozilla.org/MPL/headers/
(Assignee)

Comment 13

3 years ago
Created attachment 8485481 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays. (V2)

Mopped up a few debugging lines I missed...
Attachment #8485366 - Attachment is obsolete: true
Attachment #8485366 - Flags: review?(rnewman)
Attachment #8485481 - Flags: review?(rnewman)
(Assignee)

Comment 14

3 years ago
Created attachment 8485483 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays. (V3)

Oh, right, yes, we're not supposed to touch the UI from the background thread, right?
.. So this is probably nearer what we want. UIAsyncTask for the win.

... I still don't like its API. Are parameters ever meaningful when you can just add ctor params yourself in the concrete instance?
Attachment #8485481 - Attachment is obsolete: true
Attachment #8485481 - Flags: review?(rnewman)
Attachment #8485483 - Flags: review?(rnewman)
(Assignee)

Comment 15

3 years ago
(In reply to Richard Newman [:rnewman] from comment #12)
> No, this is a legal template. "You" is correct. For some reason you have a
> very old license template with incorrect wrapping.
> 
> https://www.mozilla.org/MPL/headers/

Seems I copypasta'd from a file that had a dodgy template :/
Will address that...
(Assignee)

Comment 16

3 years ago
Created attachment 8485752 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Fixed license.
Attachment #8485752 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8485483 - Attachment is obsolete: true
Attachment #8485483 - Flags: review?(rnewman)
Comment on attachment 8485752 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Review of attachment 8485752 [details] [diff] [review]:
-----------------------------------------------------------------

Did you test this? It looks like you only ever disable the buttons, and given that they're disabled by default...

What happens if you share item X, add to bookmarks, share it again (expect disabled), then share item Y? Want to make sure the activity isn't preserved.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +99,5 @@
>       used in the share overlay menu to select an action. It is the verb
>       "to bookmark", not the noun "a bookmark". -->
>  <!ENTITY overlay_share_bookmark_btn_label "Bookmark">
>  <!ENTITY overlay_share_reading_list_btn_label "Add to Reading List">
> +<!ENTITY overlay_share_bookmark_btn_label_already "Already Bookmarked">

"Already bookmarked"

::: mobile/android/base/overlays/service/ShareData.java
@@ +23,5 @@
> +    public final Parcelable extra;
> +    public final ShareMethod.Type shareMethodType;
> +
> +    public ShareData(String url, String title, Parcelable extra, ShareMethod.Type shareMethodType) {
> +        this.url = url;

I suggest throwing IllegalArgumentException (or another exception) for a null URL, and catching it in handleShare. That is: don't allow invalid ShareData instances to be created.

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +34,5 @@
>  
>          LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
>  
>          ContentValues values = new ContentValues();
> +        values.put(Bookmarks.TITLE, shareData.title);

Be prepared for .title to be null. Either check for null and use "", or -- if the DB and consuming code are happy -- just omit the .put.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +136,5 @@
>  
>          title = subjectText;
>          url = pageUrl;
>  
> +        LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);

final.

Can we extract a non-static method in ShareDialog to retrieve the 'current' (always default for now) profile?

@@ +203,5 @@
> +            protected void onPostExecute(Void aVoid) {
> +                if (isBookmark) {
> +                    TextView bookmarkBtn = (TextView) findViewById(R.id.overlay_share_bookmark_btn);
> +                    bookmarkBtn.setText(getResources().getString(R.string.overlay_share_bookmark_btn_label_already));
> +                    bookmarkBtn.setEnabled(false);

Shouldn't these conditions be inverted?

@@ +209,5 @@
> +
> +                if (isOnReadingList) {
> +                    TextView readinglistBtn = (TextView) findViewById(R.id.overlay_share_reading_list_btn);
> +                    readinglistBtn.setText(getResources().getString(R.string.overlay_share_reading_list_btn_label_already));
> +                    readinglistBtn.setEnabled(false);

Likewise.
Attachment #8485752 - Flags: review?(rnewman) → feedback+
(Assignee)

Comment 18

3 years ago
(In reply to Richard Newman [:rnewman] from comment #17)
> Did you test this? It looks like you only ever disable the buttons, and
> given that they're disabled by default...

The issue is I omitted to make them disabled by default. It worked, but had the obvious race condition.
I've now turned it round (also addresses your final two comments).

> What happens if you share item X, add to bookmarks, share it again (expect
> disabled), then share item Y? Want to make sure the activity isn't preserved.

It's not, thankfully. That'd be mildly annoying.

> ::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
> @@ +34,5 @@
> >  
> >          LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> >  
> >          ContentValues values = new ContentValues();
> > +        values.put(Bookmarks.TITLE, shareData.title);
> 
> Be prepared for .title to be null. Either check for null and use "", or --
> if the DB and consuming code are happy -- just omit the .put.

They are not so happy: such a field is required.
On the bright side, they're also null-safe, so all is well.

> ::: mobile/android/base/overlays/ui/ShareDialog.java
> @@ +136,5 @@
> >  
> >          title = subjectText;
> >          url = pageUrl;
> >  
> > +        LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> 
> final.
> 
> Can we extract a non-static method in ShareDialog to retrieve the 'current'
> (always default for now) profile?

So this just becomes new LocalBrowserDB(getProfile())?
Naturally.

How do you envision the database interactions will work when we have profile-switchableness, then? Lightweight shortlived LocalBrowserDB objects that are created for each database interaction and thrown away?
(Assignee)

Comment 19

3 years ago
Created attachment 8485895 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays
Attachment #8485895 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8485752 - Attachment is obsolete: true
(In reply to Chris Kitching [:ckitching] from comment #4)
> Created attachment 8485349 [details] [diff] [review]
> Disable buttons when items are already on lists.
> 
> Warmup intents now take urls/titles as parameters (which makes some sense
> now we reinitialise for each share), causing the dispatch of UI update
> events which notify clients of the already-on-list-ness of the page.
> Currently, disabledness looks like this:
> 
> https://www.dropbox.com/s/zu0v2cnc60s4mjd/Disabled.png?dl=0
> 
> Thoughts?

I agree with changing the copy to "Already on Reading List" and "Already a Bookmark". I don't think we need to greyed out the actions(as the screenshot above). It looks like the colors are off. 

I think an updated state of the labels is quite straightforward. If the user attempts to hit the action, we could make the label pop once(a subtle pop please), showing a simple reaction to the attempt.
Flags: needinfo?(ywang)
Attachment #8485895 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8485895 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8487692 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays
Attachment #8487692 - Flags: review?(rnewman)
(Assignee)

Comment 22

3 years ago
Created attachment 8487693 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays vN
Attachment #8487693 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8487692 - Attachment is obsolete: true
Attachment #8487692 - Flags: review?(rnewman)
(Assignee)

Comment 23

3 years ago
That "pop" is very cool.

Unfortunately, implementing it needed quite a refactor: sorry Richard! The buttons are no longer TextViews, but a kind of LinearLayout holding an ImageView and TextView side by side. I bolted a bit of extra state onto them so they support different icon/text combinations based on enabledness, as well as allowing the icon to "pop".
I'm also sneaking in a new version of the "send tab" icon that I omitted to include in earlier patches. (s/computer/aeroplane/g)

The only thing that's not quite to Yuan's specification just yet is the thing about the reading list wanting to have a + symbol on it. Is there a standard plus icon we bolt onto things, were you going to draw an icon with a plus, or should I just crack out gimp and add a plus to the icon I have myself?
Flags: needinfo?(ywang)
(Assignee)

Comment 24

3 years ago
Created attachment 8487694 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays
Attachment #8487694 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8487693 - Attachment is obsolete: true
Attachment #8487693 - Flags: review?(rnewman)
(In reply to Chris Kitching [:ckitching] from comment #23)
> That "pop" is very cool.
> 
> Unfortunately, implementing it needed quite a refactor: sorry Richard! The
> buttons are no longer TextViews, but a kind of LinearLayout holding an
> ImageView and TextView side by side. I bolted a bit of extra state onto them
> so they support different icon/text combinations based on enabledness, as
> well as allowing the icon to "pop".
> I'm also sneaking in a new version of the "send tab" icon that I omitted to
> include in earlier patches. (s/computer/aeroplane/g)
> 
> The only thing that's not quite to Yuan's specification just yet is the
> thing about the reading list wanting to have a + symbol on it. Is there a
> standard plus icon we bolt onto things, were you going to draw an icon with
> a plus, or should I just crack out gimp and add a plus to the icon I have
> myself?

I'm talking with Anthony on whether to use the plus or not for Reading List. If we are going to use the icon with +, it will be a separate icon from the one without +. I will keep you posted.
Flags: needinfo?(ywang)
Attachment #8487693 - Attachment description: Handle items already in bookmarks/reading list in overlays → Handle items already in bookmarks/reading list in overlays vN
Comment on attachment 8487694 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Review of attachment 8487694 [details] [diff] [review]:
-----------------------------------------------------------------

Next pass should be a rubberstamp.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +99,5 @@
>       used in the share overlay menu to select an action. It is the verb
>       "to bookmark", not the noun "a bookmark". -->
>  <!ENTITY overlay_share_bookmark_btn_label "Bookmark">
>  <!ENTITY overlay_share_reading_list_btn_label "Add to Reading List">
> +<!ENTITY overlay_share_bookmark_btn_label_already "Already a bookmark">

"Already bookmarked"

The page isn't a bookmark; it's a page. It's been bookmarked.

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +10,2 @@
>  import android.os.Parcelable;
> +import android.support.v4.content.LocalBroadcastManager;

Clean up imports.

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +11,2 @@
>  import android.os.Parcelable;
> +import android.support.v4.content.LocalBroadcastManager;

Clean up imports.

::: mobile/android/base/overlays/ui/OverlayDialogButton.java
@@ +18,5 @@
> +import android.widget.ImageView;
> +import android.widget.LinearLayout;
> +import android.widget.TextView;
> +
> +public class OverlayDialogButton extends LinearLayout {

Brief block comment explaining what the hell this is.

@@ +61,5 @@
> +        icon = (ImageView) findViewById(R.id.overlaybtn_icon);
> +        label = (TextView) findViewById(R.id.overlaybtn_label);
> +    }
> +
> +    public void setLabelAndIcon(String s, Drawable d) {

setEnabledLabelAndIcon

@@ +92,5 @@
> +        }
> +    }
> +
> +    /**
> +     * Helper method to lazily-initialise the OnClickListener to perform the "pop" animation.

lazily initialize disabledOnClickListener...

`updateViews` will take care of actually making it the OnClickListener for this view.

@@ +99,5 @@
> +        if (disabledOnClickListener == null) {
> +            disabledOnClickListener = new OnClickListener() {
> +                @Override
> +                public void onClick(View view) {
> +                    Log.e("Fennec", "Pop called!");

Remove.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +156,5 @@
>          findViewById(R.id.sharedialog).startAnimation(anim);
>  
> +        // Configure buttons.
> +        final OverlayDialogButton bookmarkBtn = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
> +        bookmarkBtn.setLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label), getResources().getDrawable(R.drawable.overlay_bookmark_icon));

Lift out getResources, and use locals here to make my eyes hurt less.

@@ +157,5 @@
>  
> +        // Configure buttons.
> +        final OverlayDialogButton bookmarkBtn = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
> +        bookmarkBtn.setLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label), getResources().getDrawable(R.drawable.overlay_bookmark_icon));
> +        bookmarkBtn.setDisabledLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label_already), getResources().getDrawable(R.drawable.overlay_bookmarked_icon));

Same.

@@ +158,5 @@
> +        // Configure buttons.
> +        final OverlayDialogButton bookmarkBtn = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
> +        bookmarkBtn.setLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label), getResources().getDrawable(R.drawable.overlay_bookmark_icon));
> +        bookmarkBtn.setDisabledLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label_already), getResources().getDrawable(R.drawable.overlay_bookmarked_icon));
> +        bookmarkBtn.setOnClickListener(new View.OnClickListener() {

I suggest combining these three calls (once you assign the various arguments to parameters) to avoid having to updateView multiple times in a row.

@@ +185,5 @@
>          sendTabList.setSendTabTargetSelectedListener(this);
>      }
>  
>      /**
> +     * Disables the bookmark/reading list buttons if the given URL is already on the corresponding

We tend to use 'in' not 'on' for lists.

@@ +192,5 @@
> +    private void disableButtonsIfAlreadyAdded(final String pageURL, final LocalBrowserDB browserDB) {
> +        new UIAsyncTask.WithoutParams<Void>(ThreadUtils.getBackgroundHandler()) {
> +            // Flags to hold the result
> +            boolean isBookmark;
> +            boolean isOnReadingList;

Use `isReadingListItem` to match below.
Attachment #8487694 - Flags: review?(rnewman) → feedback+
(Assignee)

Comment 27

3 years ago
Created attachment 8488398 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays
Attachment #8488398 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8487694 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
(In reply to Richard Newman [:rnewman] from comment #26)
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +99,5 @@
> >       used in the share overlay menu to select an action. It is the verb
> >       "to bookmark", not the noun "a bookmark". -->
> >  <!ENTITY overlay_share_bookmark_btn_label "Bookmark">
> >  <!ENTITY overlay_share_reading_list_btn_label "Add to Reading List">
> > +<!ENTITY overlay_share_bookmark_btn_label_already "Already a bookmark">
> 
> "Already bookmarked"
> 
> The page isn't a bookmark; it's a page. It's been bookmarked.

I changed it to this due to Comment 20. I'll change it back...

> ::: mobile/android/base/overlays/ui/OverlayDialogButton.java
> @@ +18,5 @@
> > +import android.widget.ImageView;
> > +import android.widget.LinearLayout;
> > +import android.widget.TextView;
> > +
> > +public class OverlayDialogButton extends LinearLayout {
> 
> Brief block comment explaining what the hell this is.

AAA!
Definitely. Please shout whenever I forgot to do this.

> @@ +158,5 @@
> > +        // Configure buttons.
> > +        final OverlayDialogButton bookmarkBtn = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
> > +        bookmarkBtn.setLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label), getResources().getDrawable(R.drawable.overlay_bookmark_icon));
> > +        bookmarkBtn.setDisabledLabelAndIcon(getResources().getString(R.string.overlay_share_bookmark_btn_label_already), getResources().getDrawable(R.drawable.overlay_bookmarked_icon));
> > +        bookmarkBtn.setOnClickListener(new View.OnClickListener() {
> 
> I suggest combining these three calls (once you assign the various arguments
> to parameters) to avoid having to updateView multiple times in a row.

Being smarter about when to call updateView inside the view class seems a neat approach here.
(Assignee)

Comment 29

3 years ago
Created attachment 8488401 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Omitted to hg add some pngs. This is the only change from the previous revision.
Attachment #8488401 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8488398 - Attachment is obsolete: true
Attachment #8488398 - Flags: review?(rnewman)
Comment on attachment 8488401 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Review of attachment 8488401 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the resume change.

Reasoning behind that: you can invoke the share activity, task switch to Fennec, bookmark the site, and switch back. Yes, I tried it. Yes, it's an edge case. But might as well be thorough.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +140,5 @@
>          url = pageUrl;
>  
> +        LocalBrowserDB browserDB = new LocalBrowserDB(getCurrentProfile());
> +
> +        disableButtonsIfAlreadyAdded(url, browserDB);

You should trigger this in onResume, not onCreate, I think. The browserDB fetch can go with it, I think.
Attachment #8488401 - Flags: review?(rnewman) → review+
(Assignee)

Comment 31

3 years ago
(In reply to Richard Newman [:rnewman] from comment #30)
> You should trigger this in onResume, not onCreate, I think. The browserDB
> fetch can go with it, I think.

It'd be nice if we could keep the LocalBrowserDB around, but we'd need to re-do the getCurrentProfile() call and reconfigure the LocalBrowserDB.
I guess we can leave that to another poor sod to fix in the future. The cost isn't so great, we just end up re-initialising a bunch of URIs. *shrug*

But, yes, good idea, I should've thought of that :/
(Assignee)

Comment 32

3 years ago
Tested your edgecase: it now correctly switches to indicate the bookmarkedness of the thing.
(Assignee)

Comment 33

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/d996743af7a3
https://hg.mozilla.org/mozilla-central/rev/d996743af7a3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(Reporter)

Comment 35

3 years ago
After I open news.google.com, go to Custom Menu -> Share -> Add to Nightly, choose "Add to Reading List" and "Bookmark" and then try to perform these steps once again, the pop-up displays: "Already in Reading List" and "Already bookmarked" , so verified fixed on:

Build: Firefox for Android 35.0a1 (2014-09-14)
Status: RESOLVED → VERIFIED
Comment on attachment 8488401 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

Dependency for feature we'd like to turn on in 34. Sorry for the late approval request.
Attachment #8488401 - Flags: approval-mozilla-beta?
Blocks: 1092409
No longer blocks: 1092409
Comment on attachment 8488401 [details] [diff] [review]
Handle items already in bookmarks/reading list in overlays

This patch includes string changes, which we can't accept on Beta. Landing this feature will require an alternate solution that does not add new strings. Beta-
Attachment #8488401 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.