We have a nice feature in the Remote Tabs home panel: you can long click devices and you get an option to "Hide" this device [1]. If you hide a device, you get a bar at the bottom [2]; tapping the bar lets you unhide selected devices. If you haven't hidden a device, we don't show the bar. That is, we never show "0 devices hidden". This ticket tracks showing an informative message in the bar, like "Did you know? You can hide devices you don't want to see." When tapped, we should show a dialog or change the bar to say "Hide a device by long-tapping it and selecting 'Hide'". We'll want a button that says "Got it.". We want to show such a message: 1) only if no devices are hidden; 2) only once. So we'll need to add a preference that tracks whether the user has either ever hidden a device, or has tapped the "Got it." button. The logic for showing and hiding the bar at the bottom is at [3]. The preference for whether the user has ever hidden device should be added to [4]. This is a good mentor bug, but not a good first or second bug. [1] https://people.mozilla.org/~nalexander/screenshots/Remote.Tabs.Hide.Client.png [2] https://people.mozilla.org/~nalexander/screenshots/Remote.Tabs.Clients.Hidden.png [3] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsExpandableListFragment.java#304 [4] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsExpandableListState.java#30

Hey nalexander! I would love to work on this bug. I went through your comment, and I understand(atleast theoretically) what is it that we aim with this bug. I would try out the remote tabs home panel( haven't tried it many times) and go through the code segments marked more carefully. I am traveling right now and will get on to it by 12th(mostly). Any pointers for me other than the ones already mentioned? Thanks :)

Hey! I did not exactly understand where exactly is the message supposed to come. I have very little android experience and would love it if someone could guide me :) Thanks

Hey The way I am planning to go about this bug is: 1) add preference to RemoteTabsExapandableListState.java 2) add function to the same file, to get and set this boolean pref( plan to use object of Editor class in setting it) Am I in the correct direction?

(In reply to Prabhjyot Sodhi [:psd] from comment #3) > Hey > The way I am planning to go about this bug is: > 1) add preference to RemoteTabsExapandableListState.java > 2) add function to the same file, to get and set this boolean pref( plan to > use object of Editor class in setting it) > > Am I in the correct direction? Hey! Sorry for the delayed reply, I'm well under-water. I could see this ticket developing in several pieces: 1) adding preference/getter/setter to RemoteTabsELS, as you say; 2) updating the view logic, as I described in https://bugzilla.mozilla.org/show_bug.cgi?id=1112673#c0. Please flag me to comment on work-in-progress patches.

Just starting! btw I have a mozillians account: https://mozillians.org/en-US/u/pss/

Comment on attachment 8551983 [details] [diff] [review] 1112673.patch I wrongly typed my mozillians account: it is https://mozillians.org/en-US/u/psd/

Hey all: For the dialog box that is required. How should I go about it? Should I use AlertDialog.Builder, or make a new Dialog Fragment like RemoteClientsDialogFragment [1] ? [1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsExpandableListFragment.java#221

Comment on attachment 8551983 [details] [diff] [review] 1112673.patch Review of attachment 8551983 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/RemoteTabsExpandableListState.java @@ +71,5 @@ > } > } > > + boolean getBoolean(String pref) { > + return sharedPrefs.getBoolean(pref, false); Drive-by feedback: I don't think we need helper methods for these simple operations. If I were you, I would focus on figuring out where we should actually flip this pref when the user has hidden a device or interacted with an informative message. We'll also need to actually implement that message.

Comment on attachment 8551983 [details] [diff] [review] 1112673.patch Review of attachment 8551983 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the looooong delay; I've been busy and didn't realize how small this was. This will get/set prefs, yes; but like margaret said, we don't want to show these small helpers. We do want to show a nice API, something like: void setUserAcknowledgedHiddenDevices() boolean getUserHasAcknowledgedHiddenDevices() ::: mobile/android/base/home/RemoteTabsExpandableListState.java @@ +74,5 @@ > + boolean getBoolean(String pref) { > + return sharedPrefs.getBoolean(pref, false); > + } > + > + void updateUserNotifiedFeature(String pref, boolean Value) { Since you include the name of the pref in the function, you don't need to take a pref parameter.

I will add prefs for the strings and fix other errors is coding style if this patch is appropriate :)

Comment on attachment 8557493 [details] [diff] [review] Patch_1112673.patch Review of attachment 8557493 [details] [diff] [review]: ----------------------------------------------------------------- This is well on it's way. Major asks: 1) include layout file; 2) extract strings; 3) extract helper function. Good work! ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java @@ +310,5 @@ > // differentiates the first from the latter two states. > boolean displayedSomeClients = false; > > if (hiddenClients == null || hiddenClients.isEmpty()) { > + final View mUserAcknowledgedView; Let's extract this whole thing to a helper function that makes the view, connects the clickable button, etc. @@ +313,5 @@ > if (hiddenClients == null || hiddenClients.isEmpty()) { > + final View mUserAcknowledgedView; > + if (!sState.getUserHasAcknowledgedHiddenDevices()) { > + mUserAcknowledgedView = LayoutInflater.from(getActivity()).inflate( > + R.layout.home_remote_tabs_user_acknowledged_hidden_devices, mList, false); Looks like this layout is missing from your patch. Can we put the messaging from the dialog into the layout, so there are fewer steps for the user? @@ +320,5 @@ > + view.setOnClickListener(new OnClickListener() { > + @Override > + public void onClick(View view) { > + final AlertDialog.Builder builder = new AlertDialog.Builder(getActivity()); > + builder.setTitle("Hide Unwanted Devices"); These will need to be extracted to strings -- see android_strings.dtd and strings.xml.in. @@ +333,5 @@ > + > + builder.create().show(); > + } > + }); > + mList.removeFooterView(mFooterView); I'm not sure this will work well on all Android devices. We may need to *update* the existing view rather than remove and add again, but we'll leave that until testing. @@ +337,5 @@ > + mList.removeFooterView(mFooterView); > + mList.addFooterView(mUserAcknowledgedView, null, true); > + } else { > + mList.removeFooterView(mFooterView); > + } nit: indentation. ::: mobile/android/base/home/RemoteTabsExpandableListState.java @@ +29,5 @@ > */ > public class RemoteTabsExpandableListState { > private static final String PREF_COLLAPSED_CLIENT_GUIDS = "remote_tabs_collapsed_client_guids"; > private static final String PREF_HIDDEN_CLIENT_GUIDS = "remote_tabs_hidden_client_guids"; > + private static final String PREF_USER_ACKNOWLEDGED_HIDDEN_DEVICES = nit: can we make this USER_HAS_ACKNOWLEDGED, so it's clear that it's in the past and a one time thing?

(In reply to Nick Alexander :nalexander from comment #11) > Can we put the messaging > from the dialog into the layout, so there are fewer steps for the user? I did not get what you were asking for, Can you please elaborate, so I can make the changes Thanks :)

Comment on attachment 8560976 [details] [diff] [review] 1112673.patch Review of attachment 8560976 [details] [diff] [review]: ----------------------------------------------------------------- Hey! Sorry for the delayed review. The code this is based on has changed a good deal, since vivek and I have been working on split pane remote tabs for tablets. This is looking pretty good -- I have some nits, and will want to test it locally and get UX input on the visual design -- but I'd like you to have a stab at rebasing it onto a current fx-team. Rebasing this won't be simple, because the files have moved and this will have to work in more situations, but vivek and I can help if you get stuck. Thanks! Looking good! ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java @@ +175,5 @@ > mCursorLoaderCallbacks = new CursorLoaderCallbacks(); > loadIfVisible(); > } > > + View userAcknowledgedView() { nit: protected, and use a verb like "make" or "create". @@ +190,5 @@ > + .setPositiveButton(getResources().getString(R.string.home_remote_tabs_acknowledge_hidden_devices_dialog_button), new DialogInterface.OnClickListener() { > + @Override > + public void onClick(DialogInterface dialogInterface, int i) { > + sState.setUserHasAcknowledgedHiddenDevices(); > + mList.removeFooterView(userAcknowledgedView); I wonder if this is enough. I feel like we should really go and update the list, even though this is likely to work... @@ +211,5 @@ > > if (hiddenClients == null || hiddenClients.isEmpty()) { > + if (!sState.getUserHasAcknowledgedHiddenDevices()) { > + > + final View view = userAcknowledgedView(); nit: remove extra blank lines. ::: mobile/android/base/home/RemoteTabsExpandableListState.java @@ +29,5 @@ > */ > public class RemoteTabsExpandableListState { > private static final String PREF_COLLAPSED_CLIENT_GUIDS = "remote_tabs_collapsed_client_guids"; > private static final String PREF_HIDDEN_CLIENT_GUIDS = "remote_tabs_hidden_client_guids"; > + private static final String PREF_SELECTED_CLIENT_GUID = "remote_tabs_selected_client_guid"; nit: this line is unchanged after you get rid of the extra trailing whitespace.

Hey Hey! Sorry for the enormous delay on this bug. I have tried multiple times to get my local repo fx-team repo set up, but I seem to be facing a rather strange error. I downloaded the fx-team bundle and unbundled it on my system, but I don't get the merge errors that I am supposed to be getting. I tried confirming the presence of these merge errors, thanks to kats, who pushed it in his local repo, and reported the expected merge errors. I am probably missing something obvious. Any suggestions, I have tried multiple times, that includes downloading multiple fx-team bundles, but in vain :( I am attaching the latest version of the patch!

(In reply to Prabhjyot Sodhi [:psd] from comment #16) > Hey Hey! > Sorry for the enormous delay on this bug. > I have tried multiple times to get my local repo fx-team repo set up, but I > seem to be facing a rather strange error. > I downloaded the fx-team bundle and unbundled it on my system, but I don't > get the merge errors that I am supposed to be getting. > I tried confirming the presence of these merge errors, thanks to kats, who > pushed it in his local repo, and reported the expected merge errors. > I am probably missing something obvious. > Any suggestions, I have tried multiple times, that includes downloading > multiple fx-team bundles, but in vain :( > I am attaching the latest version of the patch! Hey! Sorry for the delayed reply. This applies cleanly to a recent a fx-team, and builds fine. It's in my queue to test and review -- I don't have a charged device right this minute.

Hey Hey!! Awesome! :)

Hey! Is there something required here?

(In reply to Prabhjyot Sodhi [:psd] from comment #20) > Hey! > Is there something required here? Nope, this just fell off my queue. Looking now.

(Copied from wrong Bug 1128362.) psd: thanks for the commits! I had some issues with yours (not huge) and wanted to rework the visuals a bit, so I took your work and ran with it. Care to test this and provide feedback? margaret: could you review the last two commits? I don't know how to edit the RB request to be more sensible.

antlam: here's the visual: https://people.mozilla.org/~nalexander/screenshots/Hidden.Devices.Tip.png

Hm, I kind of question whether this bit of information is suitable to be displayed in our snippets. It seems too prominent. But maybe I need more context. Some questions: - How many devices do most of our users have synced? - Why is this ability to "hide" really that important to call out? After all, long-press is common amongst our Panels - We already have a snippet overload situation, it might be best to be more conservative when adding another. From a UX point of view, the snippet isn't really used (anywhere else) to show "tips" on a per Panel basis. Rather, we display "tips" in the empty state of Panels so that's seems more fitting. OR maybe we need to design a non-empty state way to display tips in our Panels.

(In reply to Nick Alexander :nalexander from comment #24) > Hm, I kind of question whether this bit of information is suitable to be > displayed in our snippets. It seems too prominent. But maybe I need more > context. This isn't a snippet, it's a footer in the Synced Tabs panel. You'd see it when you have devices until you a) dismiss it or b) hide a device. > > Some questions: > - How many devices do most of our users have synced? Most have very few, but there's a constant hum in #sync about old devices not getting removed quickly enough. We built the hide feature for these folks and they're not finding it. > - Why is this ability to "hide" really that important to call out? After > all, long-press is common amongst our Panels Is long-press used? (I honestly don't know.) I imagine knowledgable folks will long press URL-like things, but I expect nobody will be long-pressing devices. > - We already have a snippet overload situation, it might be best to be more > conservative when adding another. This *looks* like a snippet but it's really a part of the list. The visual is your territory. > From a UX point of view, the snippet isn't really used (anywhere else) to > show "tips" on a per Panel basis. Rather, we display "tips" in the empty > state of Panels so that's seems more fitting. OR maybe we need to design a > non-empty state way to display tips in our Panels. The empty state is not the right place for Synced Tabs, since it's only shown in the following rare situation: you have an Account and have no other devices. That almost never happens! I would be thrilled to have a non-empty state way to display tips. I've wanted this for a long time, so I want to do *something* in this area even if it's not used broadly, etc, etc. I would be happy to try to capture some Hide usage telemetry before/after adding the tip and back out the change if it's not valuable. antlam, how strong are your opinions on this?

Thanks for consolidating the comments Nick! Appreciate it. (In reply to Nick Alexander :nalexander from comment #25) > I would be thrilled to have a non-empty state way to display tips. I've > wanted this for a long time, so I want to do *something* in this area even > if it's not used broadly, etc, etc. I understand. I have a design in mind that can handle this but then, long-press is the right-click to the single press. So, I'm not that worried about the discover-ability of it to be honest. I'm more worried about over rotating on an issue that seems to not solve the source of the problem. > I would be happy to try to capture some Hide usage telemetry before/after > adding the tip and back out the change if it's not valuable. antlam, how > strong are your opinions on this? Fairly strong, heh - I really want to avoid adding something that's not great UX. I'd rather solve the problem at the source. Especially since it seems like the root of the problem is that older synced devices don't get removed quickly enough. Rather than even having to make users manage/"hide" their synced devices, we should just give them a quick and easy way to "recall" from their other devices. I feel like one of these "tips" gets in their way of that and might even draw attention to an area that might not even interest them. Telemtry first would be helpful. Thanks Nick!

I'm calling this discussion: we'll wait for a broader contextual hint effort to land such explanatory UX. psd, I'd like to thank you for your effort on this ticket and apologize that your work didn't land. These things happen, but I know it's frustrating. Hopefully we can move on this in the future.

Closing this out, since it's unlikely we'll do this, and more likely that we'll address the (very real) deficiencies with a centralized Device Manager service.