Closed
Bug 896117
Opened 8 years ago
Closed 8 years ago
[guest] Add enter and exit dialogs for guest mode
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox25 verified)
VERIFIED
FIXED
Firefox 25
Tracking | Status | |
---|---|---|
firefox25 | --- | verified |
People
(Reporter: mfinkle, Assigned: wesj)
References
Details
Attachments
(3 files, 2 obsolete files)
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
Reporter | ||
Updated•8 years ago
|
Summary: Add enter and exit dialogs for guest mode → [guest] Add enter and exit dialogs for guest mode
Reporter | ||
Updated•8 years ago
|
Blocks: guest-mode
Assignee | ||
Comment 2•8 years ago
|
||
This adds some dialogs for entering and exiting guest mode. I'll post a couple screenshots.
Attachment #779369 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
I had to take a little license here, since we don't have a mockup.
Assignee | ||
Comment 5•8 years ago
|
||
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.
Attachment #779369 -
Attachment is obsolete: true
Attachment #779369 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → wjohnston
Comment 6•8 years ago
|
||
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.
Attachment #779373 -
Flags: feedback+
Reporter | ||
Comment 7•8 years ago
|
||
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.)
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #779373 -
Attachment is obsolete: true
Attachment #779603 -
Flags: review?(margaret.leibovic)
Comment 9•8 years ago
|
||
(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.
Flags: needinfo?(ibarlow)
Comment 10•8 years ago
|
||
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.
Attachment #779603 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b51cb3254d18
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b51cb3254d18
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment 13•8 years ago
|
||
Verified fixed on: Build: Firefox for Android 25.0a1 (2013-07-28) Device: LG Nexus 4 OS: Android 4.2.2
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
status-firefox25:
--- → verified
Updated•1 month 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
•