896117 - [guest] Add enter and exit dialogs for guest mode

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

The design has dialogs to help set context when enterin and leaving guest mode. Note: We do not intend to support sedning the guest profile data in v1. That is a separate bug for later.

https://wiki.mozilla.org/Mobile/Projects/Guest_mode

Comment 2

[Wesley Johnston (:wesj)]

Attachment: guestDialogs

This adds some dialogs for entering and exiting guest mode. I'll post a couple screenshots.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Entering screenshot

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Exiting screenshot

I had to take a little license here, since we don't have a mockup.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: guestDialogs

Grr. Missing parenthesis fixes.

Also, I used our Prompt system for doing this instead of AlertDialog.Builder or Acitvity.showDialog(). I'm not really sure there's a good reason to pick one over the other, and I'd be fine picking a different one if requested.

Comment 6

[:Margaret Leibovic]

Comment on attachment 779373 [details] [diff] [review]
guestDialogs

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

Giving feedback+ for now until we get some string feedback from ibarlow. I also have some comments about how we can make things a bit cleaner.

::: mobile/android/base/BrowserApp.java
@@ +1761,5 @@
> +            case R.id.enter_guest_mode: {
> +                final Prompt ps = new Prompt(this, new Prompt.PromptCallback() {
> +                    @Override
> +                    public void onPromptFinished(String result) {
> +                        int itemId = -1;

It doesn't look like there's any reason to declare this variable up here, as opposed to when you just get it's real value below.

@@ +1801,5 @@
> +                });
> +                Resources res = getResources();
> +                ps.setButtons(new String[] {
> +                    res.getString(R.string.guest_mode_exitdialog_continue),
> +                    res.getString(R.string.guest_mode_exitdialog_cancel),

I don't think we really need to make different strings for the continue/cancel buttons for each case. In fact, we could probably just use this button_cancel string (and add another generic string in here for a continue button):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#263

@@ +1809,2 @@
>                  return true;
> +            }

These two cases are so similar, I think it would be nice to pull this into a helper method that just shows different text/handles the "Continue" button click differently depending on whether we're entering or exiting.

::: mobile/android/base/strings.xml.in
@@ +327,5 @@
> +  <string name="guest_mode_exitdialog_continue">&guest_mode_exitdialog_continue;</string>
> +  <string name="guest_mode_exitdialog_cancel">&guest_mode_exitdialog_cancel;</string>
> +  <string name="guest_mode_exitdialog_title">&guest_mode_exitdialog_title;</string>
> +  <string name="guest_mode_exitdialog_text">&guest_mode_exitdialog_text;</string>
> +  

Nit: whitespace.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we could get by with this in the leaving dialog:

Firefox will now delete the browsing data from this session.

(which is the first sentence from Ian's mockup. need-info to Ian.)

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> I think we could get by with this in the leaving dialog:
> 
> Firefox will now delete the browsing data from this session.
> 
> (which is the first sentence from Ian's mockup. need-info to Ian.)

That's fine. Matej and I are going to do a copywriting polish pass later this week but let's land this for now.

Comment 10

[:Margaret Leibovic]

Comment on attachment 779603 [details] [diff] [review]
Patch

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

r+ but I still have some suggestions if you want to address those before landing (also make sure you got those title/text strings correct, looks like you reversed them).

::: mobile/android/base/BrowserApp.java
@@ +113,5 @@
>          public int parent;
>      }
>  
> +    // The types of guest mdoe dialogs we show
> +    enum GuestModeDialog {

This can be private and static.

@@ +115,5 @@
>  
> +    // The types of guest mdoe dialogs we show
> +    enum GuestModeDialog {
> +        ENTERING,
> +        EXITING

I wonder if we should change our variable names to LEAVING to match the strings that are used in the UI.

@@ +1769,2 @@
>                  return true;
> +            }

Nit: No braces, to be consistent with the rest of this method.

@@ +1786,5 @@
> +                    if (itemId == 0) {
> +                        String args = "";
> +                        if (type == GuestModeDialog.ENTERING) {
> +                            args = "--guest-mode";
> +                        }

How about using a ternary operator here? I don't have a huge preference, but that would be more concise.

@@ +1803,5 @@
> +            res.getString(R.string.guest_mode_dialog_cancel)
> +        });
> +
> +        int titleString = R.string.guest_mode_enter_title;
> +        int msgString = R.string.guest_mode_enter_text;

I know it would take more lines, but I feel like it would be better to set these in an else statement, rather than overwriting them when we're in ENTERING mode. Or use ternary operators.

@@ +1806,5 @@
> +        int titleString = R.string.guest_mode_enter_title;
> +        int msgString = R.string.guest_mode_enter_text;
> +        if (type == GuestModeDialog.ENTERING) {
> +            titleString = R.string.guest_mode_exit_title;
> +            msgString = R.string.guest_mode_exit_text;

Shouldn't these be the *_enter_* strings, since we're in ENTERING mode?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +351,5 @@
>  <!ENTITY updater_apply_title2 "Update available for &brandShortName;">
>  <!ENTITY updater_apply_select2 "Touch to update">
>  
>  <!-- Guest mode -->
> +<!ENTITY enter_guest_mode "Enter guest mode">

In the mockup this just says "Guest mode".

@@ +352,5 @@
>  <!ENTITY updater_apply_select2 "Touch to update">
>  
>  <!-- Guest mode -->
> +<!ENTITY enter_guest_mode "Enter guest mode">
> +<!ENTITY exit_guest_mode "Exit guest mode">

In the mockup this says "Leave guest mode".

I know that Ian and Matej are going to do a strings pass, but it might be worth matching the mockup now if it means there will be less to change in the next patch.

@@ +359,5 @@
> +<!ENTITY guest_mode_enter_title "Entering guest mode">
> +<!ENTITY guest_mode_enter_text "While in guest mode, the person using &brandShortName; will not be able to see any of your personal browsing data (like your saved passwords, history or bookmarks).\n\nWhen your guest is done, their browsing data will be deleted.">
> +
> +<!ENTITY guest_mode_exit_title "Leaving guest mode">
> +<!ENTITY guest_mode_exit_text "Since you\'re in guest mode, &brandShortName; will delete browsing data from this session.">

This string would also need to change to match what mfinkle said in comment 7.

::: mobile/android/base/strings.xml.in
@@ +325,5 @@
> +  <string name="guest_mode_enter_text">&guest_mode_enter_text;</string>
> +
> +  <string name="guest_mode_exit_title">&guest_mode_exit_title;</string>
> +  <string name="guest_mode_exit_text">&guest_mode_exit_text;</string>
> +  

Nit: whitespace.

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b51cb3254d18

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b51cb3254d18

Comment 13

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1 (2013-07-28)
Device: LG Nexus 4
OS: Android 4.2.2