Bug tracking work on the share overlay frontend, as pictured: http://delivery.yuuuan.com/du9yadac Splitting from Bug 948509 to track the development of this particular version of the idea: getting away from the historical discussion in the other bug.

Comment on attachment 8467510 [details] [diff] [review] Add share overlay frontend Throwing Brian on for a second set of front-end eyes and knowledge transfer

Comment on attachment 8467510 [details] [diff] [review] Add share overlay frontend Review of attachment 8467510 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +89,5 @@ > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_send_tab_btn_label "Send tab to Firefox Sync"> > + > +<!ENTITY overlay_share_bookmark_toast_done "Added to bookmarks!"> > +<!ENTITY overlay_share_reading_list_toast_done "Reading list item added!"> Reuse bookmark_added and reading_list_added instead of adding these strings. ::: mobile/android/base/overlays/OverlayIntentConstants.java @@ +1,1 @@ > +package org.mozilla.gecko.overlays; Nit: Add MPL (here and all other new files). ::: mobile/android/base/overlays/ShareDialog.java @@ +22,5 @@ > +/** > + * A transparent activity that displays the share overlay. > + */ > +public class ShareDialog extends Activity { > + private static final String LOGTAG = "LFloatyFox"; s/LFloatyFox/GeckoShareDialog/ @@ +38,5 @@ > + > + // The URL is usually hiding somewhere in the extra text. Extract it. > + String pageUrl = extractUrlFromString(intent.getStringExtra(Intent.EXTRA_TEXT)); > + if (TextUtils.isEmpty(pageUrl)) { > + Log.e(LOGTAG, "Unable to process shared intent. No URL found!"); So we create a dialog here but bail immediately during it's creation if the intent doesn't have a URL? That will look broken, and since there's no guarantee that a android.intent.action.SEND will have a URL, we should handle this more gracefully. Perhaps we can have a simple message in the dialog explaining that the share content requires a URL. @@ +97,5 @@ > + // Since send-tab takes over the whole screen, we don't bother to prettily animate away. > + finish(); > + } > + > + public void addReadingListItem(View v) { I think I've heard this discussed before, but what happens if the URL isn't readerable? Do we just bail with a toast, fall back to some kind of save as PDF mechanism, or are we waiting for the headless Gecko work so we can parse while we show the dialog? @@ +136,5 @@ > + } > + > + @Override > + public void onAnimationEnd(Animation animation) { > + findViewById(R.id.sharedialog).setVisibility(View.GONE); What does this accomplish? finish() is called immediately after, so I wouldn't expect this to be meaningful. @@ +174,5 @@ > + * Some apps, notably twitter, give us a string like: > + * Check out @nytimes's Tweet: https://twitter.com/nytimes/status/486336152631529473 > + * And we have to sort out the mess. > + */ > + public String extractUrlFromString(String str) { There's a more thorough implementation of this at [1]; see example usage at [2]. Rather than reinventing the wheel, it'd be nice if we could move that class out of Sync and make it available as a more general-purpose Fennec utils class. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/activities/WebURLFinder.java [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/activities/SendTabData.java#54 ::: mobile/android/base/resources/layout/overlay_share_dialog.xml @@ +45,5 @@ > + > + <!-- Title --> > + <TextView > + android:id="@id/title" > + android:layout_width="fill_parent" fill_parent is deprecated. Use match_parent here and elsewhere.

Thanks for the comments: all these are now fixed, uploading new patch in a jiffy. (In reply to Brian Nicholson (:bnicholson) from comment #3) > @@ +38,5 @@ > > + > > + // The URL is usually hiding somewhere in the extra text. Extract it. > > + String pageUrl = extractUrlFromString(intent.getStringExtra(Intent.EXTRA_TEXT)); > > + if (TextUtils.isEmpty(pageUrl)) { > > + Log.e(LOGTAG, "Unable to process shared intent. No URL found!"); > > So we create a dialog here but bail immediately during it's creation if the > intent doesn't have a URL? That will look broken, and since there's no > guarantee that a android.intent.action.SEND will have a URL, we should > handle this more gracefully. Perhaps we can have a simple message in the > dialog explaining that the share content requires a URL. Ah! Perhaps the neatest approach is just to delay setContentView until after we check for a URL and then display a toast if no URL is provided? > @@ +97,5 @@ > > + // Since send-tab takes over the whole screen, we don't bother to prettily animate away. > > + finish(); > > + } > > + > > + public void addReadingListItem(View v) { > > I think I've heard this discussed before, but what happens if the URL isn't > readerable? Do we just bail with a toast, fall back to some kind of save as > PDF mechanism, or are we waiting for the headless Gecko work so we can parse > while we show the dialog? From the frontend's point of view: we don't care, we just pass the request to the service and let it deal with it. As for what the service is going to do about it... We cannot determine "readerableness" without loading the page. We cannot load the page without headless Gecko. We cannot have headless Gecko at all soon, it turns out. Our only option, then, seems to be to allow adding of arbitrary URLs to the reading list. We could then fall back to ordinary web view (or an error) if the page, when eventually loaded, turns out to be unreaderable. > @@ +136,5 @@ > > + } > > + > > + @Override > > + public void onAnimationEnd(Animation animation) { > > + findViewById(R.id.sharedialog).setVisibility(View.GONE); > > What does this accomplish? finish() is called immediately after, so I > wouldn't expect this to be meaningful. Uh, indeed. Whoops. > @@ +174,5 @@ > > + * Some apps, notably twitter, give us a string like: > > + * Check out @nytimes's Tweet: https://twitter.com/nytimes/status/486336152631529473 > > + * And we have to sort out the mess. > > + */ > > + public String extractUrlFromString(String str) { > > There's a more thorough implementation of this at [1]; see example usage at > [2]. Rather than reinventing the wheel, it'd be nice if we could move that > class out of Sync and make it available as a more general-purpose Fennec > utils class. > > [1] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/ > activities/WebURLFinder.java > [2] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/ > activities/SendTabData.java#54 I get the impression this sort of thing happens a lot, since there's no index of this sort of "generally useful code". It'd be nice if we had a javadoc somewhere listing such reusable utility classes so people don't end up re-inventing the wheel (or, as I did here, re-inventing half of the wheel, badly). > ::: mobile/android/base/resources/layout/overlay_share_dialog.xml > @@ +45,5 @@ > > + > > + <!-- Title --> > > + <TextView > > + android:id="@id/title" > > + android:layout_width="fill_parent" > > fill_parent is deprecated. Use match_parent here and elsewhere. Righto. 115 other instances of this problem exist in the codebase.

> > There's a more thorough implementation of this at [1]; see example usage at > > [2]. Rather than reinventing the wheel, it'd be nice if we could move that > > class out of Sync and make it available as a more general-purpose Fennec > > utils class. Fennec depends on android-sync; broadly speaking, it's acceptable for anything in Fennec to use a-s classes. We've mostly avoided it for arbitrary cleanliness reasons. There's an argument to be made for moving much of the stuff in .activities into one of the a-s utility packages; pull requests welcome. There's another argument to be made for pulling much of the non-sync utility code -- e.g., Logger, the HTTP client, various handy DB utils -- into a separate library, and then lifting stuff out of Fennec and into that library so both Sync and Fennec can use it. We haven't done so thus far because it's more adminstrative overhead. (Ideally we'd have a directed-triangle dependency chain here.) > It'd be nice if we had a javadoc somewhere listing such reusable utility > classes so people don't end up re-inventing the wheel (or, as I did here, > re-inventing half of the wheel, badly). Everything is reusable! Now, if only we had Hoogle… > > fill_parent is deprecated. Use match_parent here and elsewhere. > > Righto. 115 other instances of this problem exist in the codebase. File a bug!

Comment on attachment 8467510 [details] [diff] [review] Add share overlay frontend Clearing review until feedback addressed.

Meeting thoughts: Since this is done using an activity, it must be determined if this has a disadvantageous effect on the lifecycles of other apps (we don't particularly want them to stop on our account). For future work that does use the overlay animation library, hardware acceleration would likely reduce observed animation choppiness.

(In reply to Chris Kitching [:ckitching] from comment #7) > Since this is done using an activity, it must be determined if this has a > disadvantageous effect on the lifecycles of other apps (we don't > particularly want them to stop on our account). When an activity sends a share intent, the Android system "pick share target" dialog pops up. At this point, the activity gets an onPause event. When they select "Send to Firefox", if we keep the current implementation that uses an activity for the share overlay, the dispatching activity doesn't get onResume until after the interaction with the overlay is complete. On the other hand, if we tweak and use the overlay mechanism to draw on top of the other activity, they'll get an onResume as soon as they select "Send to Firefox". I'm pretty sure, since the share overlay takes over almost the entire screen, we don't want to resume the dispatching activity until the user is done with it. What if they're watching a video and choose to share it? Their video would resume while the overlay is still covering it. So, usefully, it looks like that's a non-problem. I love it when that happens.

Adjusted to match yuan's new graphics, fixed review comments, hidden intent handler behind build flag.

Comment on attachment 8471349 [details] [diff] [review] V2: Add share overlay frontend Review of attachment 8471349 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with changes below, assuming the interaction actually works! Find someone to make sure your XML is good -- might be some reuse/cross-linkage to Fennec's styles that we should take advantage of. If that's good, land it (turned off) so we can iterate. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +87,5 @@ > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_send_tab_btn_label "Send tab to Firefox Sync"> > +<!ENTITY overlay_share_no_url "Firefox can\'t handle a share that doesn\'t include a URL. Sorry!"> &brandShortName; instead of Firefox. ::: mobile/android/base/moz.build @@ +334,5 @@ > 'NSSBridge.java', > 'OrderedBroadcastHelper.java', > + 'overlays/OverlayIntentConstants.java', > + 'overlays/OverlayIntentHandler.java', > + 'overlays/ShareDialog.java', Build flag these. See the #if CONFIG[] pattern elsewhere in the file. ::: mobile/android/base/overlays/OverlayIntentConstants.java @@ +7,5 @@ > + > +/** > + * Constant values used to configure intents relating to the overlay service. > + */ > +public class OverlayIntentConstants { OverlayConstants? ::: mobile/android/base/overlays/ShareDialog.java @@ +19,5 @@ > +import android.view.View; > +import android.view.animation.Animation; > +import android.view.animation.AnimationUtils; > +import android.widget.TextView; > +import org.mozilla.gecko.sync.setup.activities.WebURLFinder; File a bug to move this, mark me as mentor. @@ +38,5 @@ > + Intent intent = getIntent(); > + > + // The URL is usually hiding somewhere in the extra text. Extract it. > + String extraText = intent.getStringExtra(Intent.EXTRA_TEXT); > + String pageUrl = (new WebURLFinder(extraText)).bestWebURL(); s/Url/URL throughout. These can be final. @@ +56,5 @@ > + > + // If provided, we use the subject text to give us something nice to display. > + // If not, we wing it with the URL. > + // TODO: Consider polling Fennec databases to find better information to display. > + // TODO: Extract the name of Sync's default send target device somehow and maybe use it? Before we ship, we want to have your Sync devices in-line in the list, and do away with SendTabActivity. File a bug for this, note it here. @@ +57,5 @@ > + // If provided, we use the subject text to give us something nice to display. > + // If not, we wing it with the URL. > + // TODO: Consider polling Fennec databases to find better information to display. > + // TODO: Extract the name of Sync's default send target device somehow and maybe use it? > + String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT); You are a bad member of The Finalistas. mcomella and I will string you up. It'll be... final. @@ +62,5 @@ > + if (subjectText != null) { > + ((TextView) findViewById(R.id.title)).setText(subjectText); > + } > + > + // Assign the derived url/title to fields. // Increment x by 1. @@ +67,5 @@ > + title = subjectText; > + url = pageUrl; > + > + // Set the subtitle text on the view and cause it to marquee if it's too long (which it will > + // be, since it's a URL). Make sure this is what yuan wants. ::: mobile/android/base/resources/layout/overlay_share_toast.xml @@ +38,5 @@ > + android:drawableLeft="@drawable/overlay_check"/> > + > + </LinearLayout> > + > + <!-- Various tutorials. TODO Bug 1048645 --> What?

Comment on attachment 8471349 [details] [diff] [review] V2: Add share overlay frontend Review of attachment 8471349 [details] [diff] [review]: ----------------------------------------------------------------- diff --git a/overlay_slide_down.xml b/overlay_slide_down.xml diff --git a/overlay_slide_up.xml b/overlay_slide_up.xml What's the deal with these resources? Why aren't they in m/a/b/resources/anim/? ::: mobile/android/base/overlays/ShareDialog.java @@ +9,5 @@ > +import android.view.LayoutInflater; > +import android.view.ViewGroup; > +import android.widget.Toast; > +import org.mozilla.gecko.R; > +import android.app.Activity; Nit: Alphabetize imports. ::: mobile/android/base/resources/layout/overlay_share_dialog.xml @@ +14,5 @@ > + android:layout_marginBottom="-12dp" > + android:paddingTop="30dp" > + android:layout_gravity="bottom|center" > + android:clipChildren="false" > + android:clipToPadding="false"> Why do you need these clip attributes? @@ +61,5 @@ > + <!-- Buttons --> > + <LinearLayout > + android:layout_width="match_parent" > + android:layout_height="wrap_content" > + android:background="#FFD0CECB" Factor this out into colors.xml. ::: mobile/android/base/resources/layout/overlay_share_toast.xml @@ +43,5 @@ > + <LinearLayout > + android:layout_width="match_parent" > + android:layout_height="wrap_content" > + android:background="#FFD0CECB" > + android:visibility="gone" So this layout is unused now? Remove it until it's actually needed in bug 1048645.

(In reply to Brian Nicholson (:bnicholson) from comment #12) > diff --git a/overlay_slide_down.xml b/overlay_slide_down.xml > diff --git a/overlay_slide_up.xml b/overlay_slide_up.xml > > What's the deal with these resources? Why aren't they in > m/a/b/resources/anim/? VCS fail. These are duplicates of the real ones in the correct place. Blurgh. > ::: mobile/android/base/resources/layout/overlay_share_dialog.xml > @@ +14,5 @@ > > + android:layout_marginBottom="-12dp" > > + android:paddingTop="30dp" > > + android:layout_gravity="bottom|center" > > + android:clipChildren="false" > > + android:clipToPadding="false"> > > Why do you need these clip attributes? Check out the image UX provided of what this should look like: https://bug948509.bugzilla.mozilla.org/attachment.cgi?id=8470342 The clipping attributes are necessary for the Firefox logo at the top to not be clipped.

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #11) > Find someone to make sure your XML is good -- might be some > reuse/cross-linkage to Fennec's styles that we should take advantage of. Isn't that what Brian's here for? > ::: mobile/android/base/overlays/ShareDialog.java > @@ +19,5 @@ > > +import android.view.View; > > +import android.view.animation.Animation; > > +import android.view.animation.AnimationUtils; > > +import android.widget.TextView; > > +import org.mozilla.gecko.sync.setup.activities.WebURLFinder; > > File a bug to move this, mark me as mentor. Bug 1053599 It may end up landed before you get back, given how rapidly a wild contributor appeared :D > @@ +56,5 @@ > > + > > + // If provided, we use the subject text to give us something nice to display. > > + // If not, we wing it with the URL. > > + // TODO: Consider polling Fennec databases to find better information to display. > > + // TODO: Extract the name of Sync's default send target device somehow and maybe use it? > > Before we ship, we want to have your Sync devices in-line in the list, and > do away with SendTabActivity. File a bug for this, note it here. Bug 977161 makes testing this *annoying*. We'll need to figure out with yuan what we want to do in the (really rare) situation that the user has lots of devices. We probably don't want to show arbitrarily-many of them inline. Maybe the first 1 or 2 inline and then some way of launching a menu showing the complete list (plus some heuristic for selecting the luck 1 or 2). > @@ +38,5 @@ > > + Intent intent = getIntent(); > > + > > + // The URL is usually hiding somewhere in the extra text. Extract it. > > + String extraText = intent.getStringExtra(Intent.EXTRA_TEXT); > > + String pageUrl = (new WebURLFinder(extraText)).bestWebURL(); > > These can be final. > > @@ +57,5 @@ > > + // If provided, we use the subject text to give us something nice to display. > > + // If not, we wing it with the URL. > > + // TODO: Consider polling Fennec databases to find better information to display. > > + // TODO: Extract the name of Sync's default send target device somehow and maybe use it? > > + String subjectText = intent.getStringExtra(Intent.EXTRA_SUBJECT); > > You are a bad member of The Finalistas. > > mcomella and I will string you up. It'll be... final. I'm curious about the benefits you think this produces. So there's the usual uses for "final": making some sorts of concurrency problem impossible, clarity, constants, immutable fields, blah-de-blah. But what about local variables like these ones? Making *all* your local variables final certainly seems like it doesn't help clarity more than it adds clutter (though marking *some* local variables certainly is helpful). Maybe you think there's a performance benefit to doing this? The fact a variable is declared final does find its way into the bytecode, where it may prove useful to the JIT. It doesn't have the time to do dataflow analysis and work out if a variable is effectively-final, so this could help it to produce more efficient output. However, Proguard uses dataflow analysis to mark as final as anything it can prove is effectively-final (where "effectively-final" means "not declared final but only ever written once"). As a result, when using Proguard, the only situation in which you get a performance benefit from using "final" on a local variable is when your variable is effectively-final but Proguard cannot prove it is so. Such situations are quite rare, so I claim that the really, ridiculously miniscule benefit you get from marking every effectively-final local variable as final is not worth the code clutter. (it's rare that this will ever lead to a different post-Proguard final-flag status, and it's rare that such a changed status will be of any help (since it'd require the JIT's optimiser to find itself in a situation that the finality of *that* variable allows it to do something a little better). ... Or perhaps there's a different reason why you like making all your local variables final?

The current state of the frontend. Still to do: * Make failure toasts not contain a giant tick (Hey yuan, any chance of a big red cross or such to appear in the toast in the event of an error?) * Add UI to handle large amounts of share targets (currently a certain number are shown in the list and more are to be accessible via a popup). (Hey yuan, how best to solve that, do you think?) * Make the buttons for the inlined devices not be a slightly different colour the the other ones :/ * Make the spacer between Sync devices identical to the other spacers (because for some reason it decided to be different) Currently, it looks somewhat like this: https://www.dropbox.com/s/tm0f8chs4dftc0o/DodgyIcons.png?dl=0&m= Where the top two are share devices. (yuan: The existing Send Tab handler selected an icon to display next to devices based on the kind of device it was: here you see two little mobile phones. Do you want to keep this behaviour? (in which case it looks like the icons will need tweaking, these ones seem a little low-resolution and slightly the wrong colour) Or do you want them all to show up with the one icon you already did? ... Or do you want to take some totally different approach for selecting the device to send the tab to?)

(In reply to Chris Kitching [:ckitching] from comment #15) > Created attachment 8478214 [details] [diff] [review] > V3: Add share overlay frontend > > The current state of the frontend. > Still to do: > > * Make failure toasts not contain a giant tick (Hey yuan, any chance of a > big red cross or such to appear in the toast in the event of an error?) > > * Add UI to handle large amounts of share targets (currently a certain > number are shown in the list and more are to be accessible via a popup). > (Hey yuan, how best to solve that, do you think?) > > * Make the buttons for the inlined devices not be a slightly different > colour the the other ones :/ > > * Make the spacer between Sync devices identical to the other spacers > (because for some reason it decided to be different) > > Currently, it looks somewhat like this: > https://www.dropbox.com/s/tm0f8chs4dftc0o/DodgyIcons.png?dl=0&m= > > Where the top two are share devices. > (yuan: The existing Send Tab handler selected an icon to display next to > devices based on the kind of device it was: here you see two little mobile > phones. Do you want to keep this behaviour? (in which case it looks like the > icons will need tweaking, these ones seem a little low-resolution and > slightly the wrong colour) Or do you want them all to show up with the one > icon you already did? > ... Or do you want to take some totally different approach for selecting the > device to send the tab to?) Hi Chris, My answers to your question 1. Error page: Take the same layout as the "Success" page and show strings(tentative) and an action "Try again". Tapping the action will relaunch the Share overlay.http://cl.ly/image/1W3U0u1B0y0x. We might add an icon to indicate errors, will decide with Anthony. 2. Multiple share targets If there is only target, there is no need to show an additional hierarchy. If there are more than two devices, show an arrow indicator and slide (right to left) the device list. The list has a slight color change, and uses device icons(mobile/laptop) to match with the names. http://cl.ly/image/0S2l2M0O1f2Z In terms of which icons to use, I would suggest to keep using our own icons. "Send to devices" icon for the share overlay. And generic device icons for the device list. Also, if there is no favicon available to grab, I suggest we use the generic "Globe" icon, see http://cl.ly/image/0S2l2M0O1f2Z. So it will fill the white space and keep the visual treatment consistent. Lastly, layout measurement and color spec is at: http://cl.ly/image/0R3z1y2e2Q0x/o

Updated assets for Share overlay. Added generic device icons and an arrow indicator for multiple device list

Comment on attachment 8478214 [details] [diff] [review] V3: Add share overlay frontend Review of attachment 8478214 [details] [diff] [review]: ----------------------------------------------------------------- Do we need {m,h,x,xx} variants of each of these icons? Is it enough to include {h,xx}? ::: mobile/android/base/AndroidManifest.xml.in @@ +378,5 @@ > + <intent-filter> > + <action android:name="android.intent.action.SEND" /> > + > + <category android:name="android.intent.category.DEFAULT" /> > + Collapse these empty lines. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +89,5 @@ > Firefox will use the locale currently selected in Android's settings > to display browser chrome. --> > <!ENTITY locale_system_default "System default"> > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> Yuan's mockup says simply "Bookmark". Localization note that this is the verb, instructing Firefox to add to bookmarks, not the noun form. @@ +90,5 @@ > to display browser chrome. --> > <!ENTITY locale_system_default "System default"> > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> Question for Yuan: maybe this should be "Read later"? @@ +91,5 @@ > <!ENTITY locale_system_default "System default"> > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_send_tab_btn_label "Send tab to Firefox Sync"> Add a localization note: this string is only shown if the user has too many devices to show in the list. IMO this should be "Send tab to another device". @@ +92,5 @@ > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_send_tab_btn_label "Send tab to Firefox Sync"> > +<!ENTITY overlay_share_no_url "&brandShortName; can\'t handle a share that doesn\'t include a URL. Sorry!"> Less apologetic, more specific, matches our other similar error strings: "No link found in this share." ::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java @@ +21,5 @@ > + private static final String LOGTAG = "GeckoSendTabAdapter"; > + > + private String defaultMessage; > + > + private SendTabListItemClickListener listener; final. @@ +32,5 @@ > + * devices. > + */ > + private boolean inListState() { > + return currentState == State.LIST > + || currentState == State.OVERFLOWING_LIST; Nit: || at end of line, not start. @@ +35,5 @@ > + return currentState == State.LIST > + || currentState == State.OVERFLOWING_LIST; > + } > + > + Collection<ParcelableClientRecord> records; Move this up with the other members. @@ +39,5 @@ > + Collection<ParcelableClientRecord> records; > + > + // The maximum number of target devices to show in the main list. Further devices are available > + // from a secondary menu. > + public static final int MAXIMUM_INLINE_ELEMENTS = 2; Move all constants to the top of the class. @@ +45,5 @@ > + public SendTabDeviceListArrayAdapter(Context context, SendTabListItemClickListener aListener, int textViewResourceId) { > + super(context, textViewResourceId); > + > + // Cache the default label. > + defaultMessage = context.getResources().getText(R.string.overlay_share_send_tab_btn_label).toString(); How long is the lifecycle of this array adapter? If it's short, then what's the point? If it's long, then if the user switches locale, you'll be wrong. If we never need to show that string (e.g., if we know our client records), then caching it is also pointless. Don't cache strings. @@ +58,5 @@ > + > + clear(); > + switch (newState) { > + case NONE: > + // Create a single "Send to Firefox Sync" element. Keep this comment in sync with whatever the final string ends up as. @@ +131,5 @@ > + } > + > + private int getImage(ParcelableClientRecord record) { > + // TODO: Refactor Sync device types as an enum. Storing strings: expensive. Hardcoding > + // them: error prone. "expensive" is a little hyperbolic, don't you think? The string has to arrive somehow. ::: mobile/android/base/overlays/ui/SendTabList.java @@ +21,5 @@ > + * Initially, the view resembles a disabled ShareMethodTextButton. > + * Once state is loaded from Sync's database, we know how many devices the user may send their tab > + * to. > + * > + * If the number of targets does not MAX_INLINE_SYNC_TARGETS, we present a ShareMethodTextButton does not exceed? @@ +23,5 @@ > + * to. > + * > + * If the number of targets does not MAX_INLINE_SYNC_TARGETS, we present a ShareMethodTextButton > + * for each of them. > + * Otherwise, … @@ +89,5 @@ > + } > + } > + > + public void setSyncClients(ParcelableClientRecord[] clients) { > + adapter.setClientRecordList(Arrays.asList(clients)); null check input. @@ +98,5 @@ > + switchState(NONE); > + return; > + } > + > + if (size > 0 && size <= SendTabDeviceListArrayAdapter.MAXIMUM_INLINE_ELEMENTS) { size will always be at least 0 here, so drop the first half of the conditional. ::: mobile/android/base/overlays/ui/SendTabListItemClickListener.java @@ +1,1 @@ > +package org.mozilla.gecko.overlays.ui; License. ::: mobile/android/base/overlays/ui/ShareDialog.java @@ +33,5 @@ > + > +/** > + * A transparent activity that displays the share overlay. > + */ > +public class ShareDialog extends Activity implements SendTabListItemClickListener { Activities must handle locale switching. Read https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/fennec/localeswitching.html

(In reply to Chris Kitching [:ckitching] from comment #14) > ... Or perhaps there's a different reason why you like making all your local > variables final? Simple: it catches bugs and improves micro-architecture. Assigning to variables more than once, or in non-trivial control flows, is a source of bugs. (Miss an else, get nesting wrong, mis-handle an exception, ..., and you end up assigning something once too often or not enough.) Immutability is good for more than just thread-safety. And if we can't use Haskell, at least we can use `final`. My position is that `final` should be the default behavior for both members and locals, and there should be an explicit `var` keyword to ask the compiler to let you write bug-ridden, confusing, unsafe code.

FYI, Anthony will take a final pass on the visual details of the overlay. So please expect some tweaks on colors, icons, and spacing. Thanks!

Attaching the preview of the spec, just unified the color codes, type, and icons, nothing major :) Will attach icons after. The 1 dp dividers between the items in the list are #D7D9DB (I think I forgot to label that in the spec).

Don't block this on pixel perfect spec fixes. We can/will do those in follow-ups.

^ I also probably ran out of space... (looks at his own crazy spec) Sure Wesj!

Not yet addressed most of Richard's comments, but performed the changes here that the backend review indicated were useful. (Notably the toast helper class is here) Might prove useful to reviewers over there. Will get the frontend-specific problems addressed shortly... Thanks for the input, UX people!

Comment on attachment 8480301 [details] [diff] [review] V4: Add share overlay frontend Review of attachment 8480301 [details] [diff] [review]: ----------------------------------------------------------------- Quick drive-by, even though you didn't flag me yet. ::: mobile/android/base/AndroidManifest.xml.in @@ +370,5 @@ > > +#ifdef MOZ_ANDROID_SHARE_OVERLAY > + <!-- Share overlay activity --> > + <activity android:name="org.mozilla.gecko.overlays.ui.ShareDialog" > + android:label="Send to Firefox" This needs to be a string reference. E.g., android:label="@string/moz_app_displayname" @@ +372,5 @@ > + <!-- Share overlay activity --> > + <activity android:name="org.mozilla.gecko.overlays.ui.ShareDialog" > + android:label="Send to Firefox" > + android:windowSoftInputMode="stateAlwaysHidden|adjustResize" > + android:theme="@style/ShareOverlayActivity"> Consider whether you need to handle configuration changes. E.g., android:configChanges="keyboard|keyboardHidden|mcc|mnc|orientation|screenSize|locale|layoutDirection" @@ +385,5 @@ > + </activity> > + > + <!-- Service to handle requests from overlays. --> > + <service android:name="org.mozilla.gecko.overlays.service.OverlayIntentService" > + android:label="Firefox Sharing Service" /> For the sake of completeness, this label should be a string reference, too. And both labels here should be phrased in terms of brandShortName. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +89,5 @@ > Firefox will use the locale currently selected in Android's settings > to display browser chrome. --> > <!ENTITY locale_system_default "System default"> > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> Prior review comments still apply, as you're aware. Let's get the strings done ASAP.

In general (as usual, but just reminding), a review comment I ignore is a review comment I apply as given. (so you need not check to make sure I did all the things I'm not replying to below, and I need not say "Yes, you're right, I'm an idiot" 200 times a week :P). (In reply to Richard Newman [:rnewman] from comment #18) > Do we need {m,h,x,xx} variants of each of these icons? Is it enough to > include {h,xx}? I'm not certain how to determine when this is permissible. Presumably Android will fetch the bigger one and downscale. That'll cost a bit of CPU time (so have a small effect on startup speed), but lets us make the APK smaller. Maybe there exist devices or old versions of Android that don't exhibit sensible behaviour in this context and fail to find resources? The memory spike that will occur on low-end devices while we load a high resolution resource and downscale it might cause an OOM (and heap fragmentation?) On the other hand, pngs account for a pretty small fraction of apk size. ¯\_(ツ)_/¯ I think this is a question we should answer for absolutely all of our resources at once over in Bug 959203. If we then decide that using fewer sizes is to our advantage, we should let UX know not to waste their time making them, and we can have a party deleting ~150kb of crap. > @@ +90,5 @@ > > to display browser chrome. --> > > <!ENTITY locale_system_default "System default"> > > > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > > Question for Yuan: maybe this should be "Read later"? Ignoring... > @@ +91,5 @@ > > <!ENTITY locale_system_default "System default"> > > > > +<!ENTITY overlay_share_bookmark_btn_label "Add to Bookmarks"> > > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > > +<!ENTITY overlay_share_send_tab_btn_label "Send tab to Firefox Sync"> > > Add a localization note: this string is only shown if the user has too many > devices to show in the list. > > IMO this should be "Send tab to another device". I agree, though it's not *really* a tab. So how about "Send to another device"? After all, we have "Bookmark", "Add to reading list" - both things with no explicit subject. Having a "tab" as an explicit subject, especially when there's no "tab" as such present, seems confusing. (I guess we should be careful to avoid letting our internal nomenclature for things leak into application strings when the user might not share our vocab). > @@ +32,5 @@ > > + * devices. > > + */ > > + private boolean inListState() { > > + return currentState == State.LIST > > + || currentState == State.OVERFLOWING_LIST; > > Nit: || at end of line, not start. Really?! Is that our code style? Whoops. I thought it was this. There may or may not be quite a lot of favicon code lying around that does this. I'll stop doing this now... > @@ +39,5 @@ > > + Collection<ParcelableClientRecord> records; > > + > > + // The maximum number of target devices to show in the main list. Further devices are available > > + // from a secondary menu. > > + public static final int MAXIMUM_INLINE_ELEMENTS = 2; > > Move all constants to the top of the class. Yuan's new design violates an assumption I made about how this would work. > @@ +45,5 @@ > > + public SendTabDeviceListArrayAdapter(Context context, SendTabListItemClickListener aListener, int textViewResourceId) { > > + super(context, textViewResourceId); > > + > > + // Cache the default label. > > + defaultMessage = context.getResources().getText(R.string.overlay_share_send_tab_btn_label).toString(); > > How long is the lifecycle of this array adapter? > > If it's short, then what's the point? > > If it's long, then if the user switches locale, you'll be wrong. > > If we never need to show that string (e.g., if we know our client records), > then caching it is also pointless. > > Don't cache strings. I found the manual you linked before about localisation most instructive, thank you. THERE EXISTS DOCUMENTATION \o/ > @@ +58,5 @@ > > + > > + clear(); > > + switch (newState) { > > + case NONE: > > + // Create a single "Send to Firefox Sync" element. > > Keep this comment in sync with whatever the final string ends up as. > > @@ +131,5 @@ > > + } > > + > > + private int getImage(ParcelableClientRecord record) { > > + // TODO: Refactor Sync device types as an enum. Storing strings: expensive. Hardcoding > > + // them: error prone. > > "expensive" is a little hyperbolic, don't you think? The string has to > arrive somehow. Slightly tongue-in-cheek comment :P. It struck me as strange that Sync uses string literals for this. Assuming you're serialising these types, I would've imagined an enum with an explicit ordinal (that is, you provide you own field to do the job of "ordinal" so you're not going to break when some twerp comes along and rewrites your enum), so you can just stick an integer in your communications. Also, this means nobody will ever typo and compare to "mobil" or whatever. Just a thought. The general theme is "nobody cares", but it seemed odd to me that it wasn't done that way. My cleanup patch sense was tingling. > ::: mobile/android/base/overlays/ui/SendTabList.java > @@ +21,5 @@ > > + * Initially, the view resembles a disabled ShareMethodTextButton. > > + * Once state is loaded from Sync's database, we know how many devices the user may send their tab > > + * to. > > + * > > + * If the number of targets does not MAX_INLINE_SYNC_TARGETS, we present a ShareMethodTextButton > > does not exceed? Has anyone really been far even as decided to use want to go even do look more like? > > + * to. > > + * > > + * If the number of targets does not MAX_INLINE_SYNC_TARGETS, we present a ShareMethodTextButton > > + * for each of them. > > + * Otherwise, > > … Tune in next week... > @@ +98,5 @@ > > + switchState(NONE); > > + return; > > + } > > + > > + if (size > 0 && size <= SendTabDeviceListArrayAdapter.MAXIMUM_INLINE_ELEMENTS) { > > size will always be at least 0 here, so drop the first half of the > conditional. Um. So it turns out IDEA updated and turned off nearly all of its shiny new toys by default. It also added a load more... This is slightly cool. I'll file the results later. (Things like "Field used from both synchronised and unsynchronised context" seem like they might uncover some bugs...) Seriously, look at the list here: http://www.jetbrains.com/idea/documentation/inspections.jsp Under "Threading issues". Drool. Note that this list is outdated and incomplete. Drool more. (In reply to Richard Newman [:rnewman] from comment #25) > @@ +372,5 @@ > > + <!-- Share overlay activity --> > > + <activity android:name="org.mozilla.gecko.overlays.ui.ShareDialog" > > + android:label="Send to Firefox" > > + android:windowSoftInputMode="stateAlwaysHidden|adjustResize" > > + android:theme="@style/ShareOverlayActivity"> > > Consider whether you need to handle configuration changes. E.g., > > > android: > configChanges="keyboard|keyboardHidden|mcc|mnc|orientation|screenSize|locale| > layoutDirection" Presumably I should also list here configuration changes that mean nothing to me, such as MNC, as well as ones I actually handle. Orientation is handled quite nicely by Android by itself, so I'm kinda okay keeping the default behaviour there. Locale needs to be handled by all activities of ours that wish to be LocaleAwareActivities, methinks? Keyboards don't matter to me... > @@ +385,5 @@ > > + </activity> > > + > > + <!-- Service to handle requests from overlays. --> > > + <service android:name="org.mozilla.gecko.overlays.service.OverlayIntentService" > > + android:label="Firefox Sharing Service" /> > > For the sake of completeness, this label should be a string reference, too. Actually, this needs to stop existing. Thanks for pointing it out. There will be a brief interlude while I rejig the send-tab-to-device frontend to properly handle large amounts of devices, then you'll have a landable patch (hopefully). As a followup, I'll be adding Yuan's fancy "slide to the left to show devices" thingy. Sorry. That's too problematic to do immediately.

Addressed review comments, added UI for selecting devices. This is now "finished". It requires some UI polish. The most glaring deviation from Yuan's vision at the moment is how we handle large numbers of Sync devices. Instead of a cool slide-sideways thing, we just do a boring AlertDialog. On the other hand, it seems sort of unusual that people who aren't developers experiencing Bug 977161 will have enough connected devices (you'll need a total of three devices on FF Sync for this to occur). For that reason, I'm not hugely worried about this (though am irritated I didn't get it done in time. Android's animation libraries are apparently not my friend). Hopefully we can land this and uplift UI polish as required. That, or stew on Nightly for six weeks and ship in 35...

Comment on attachment 8481914 [details] [diff] [review] V5: Add share overlay frontend Review of attachment 8481914 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. ::: mobile/android/base/AndroidManifest.xml.in @@ +376,5 @@ > + <!-- Share overlay activity --> > + <activity android:name="org.mozilla.gecko.overlays.ui.ShareDialog" > + android:label="@string/overlay_share_header" > + android:theme="@style/ShareOverlayActivity" > + android:configChanges="keyboard|keyboardHidden|mcc|mnc|locale" Never specify `locale` without `layoutDirection`. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +97,5 @@ > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_header "Send to &brandShortName;"> > + > +<!ENTITY overlay_share_send_other "Send to other devices"> > + <!-- Localization note (overlay_share_send_tab_btn_label) : Used on the Correct indenting. @@ +99,5 @@ > + > +<!ENTITY overlay_share_send_other "Send to other devices"> > + <!-- Localization note (overlay_share_send_tab_btn_label) : Used on the > + share overlay menu to represent the "Send Tab" action when the user > + either has not set up sync, or has no other devices to send a tab s/sync/Sync @@ +100,5 @@ > +<!ENTITY overlay_share_send_other "Send to other devices"> > + <!-- Localization note (overlay_share_send_tab_btn_label) : Used on the > + share overlay menu to represent the "Send Tab" action when the user > + either has not set up sync, or has no other devices to send a tab > + to.--> Space after period (here and line 95). ::: mobile/android/base/overlays/ui/OverlayToastHelper.java @@ +52,5 @@ > + > + TextView text = (TextView) layout.findViewById(R.id.overlay_toast_message); > + text.setText(message); > + > + if (!withRetry) { Can we avoid adding the param, and just use retryListener == null? ::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java @@ +21,5 @@ > + > +public class SendTabDeviceListArrayAdapter extends ArrayAdapter<ParcelableClientRecord> { > + private static final String LOGTAG = "GeckoSendTabAdapter"; > + > + protected State currentState; Comment about synchronization. Move to be with its friends. @@ +25,5 @@ > + protected State currentState; > + > + private final SendTabTargetSelectedListener listener; > + > + Collection<ParcelableClientRecord> records; Comment about synchronization. @@ +61,5 @@ > + } > + > + clear(); > + > + // Alas, addAll isn't available in all API versions we support... if (Versions.feature11Plus) { addAll(records); // TODO: find out if `addAll` notifies. `add` does. } else { setNotifyOnChange(false); // So we don't notify for each add. for (ParcelableClientRecord record : records) { add(record); } notifyDataSetChanged(); } @@ +108,5 @@ > + return R.drawable.sync_mobile; > + } > + > + if (record.type == null) { > + return R.drawable.overlay_send_tab_icon; The fallback should be sync_desktop. @@ +140,5 @@ > + /** > + * Get a dummy ParcelableClientRecord for use in one of the "button states". > + */ > + private void showDummyRecord(String name) { > + Log.e("Fennec", "Showing dumm record: " + name); Typo, and should this just die? ::: mobile/android/base/overlays/ui/SendTabList.java @@ +24,5 @@ > +import static org.mozilla.gecko.overlays.ui.SendTabList.State.NONE; > +import static org.mozilla.gecko.overlays.ui.SendTabList.State.SHOW_DEVICES; > + > +/** > + * The SendTab button has a few different states depending on the available devices (and if we've s/if/whether @@ +33,5 @@ > + * to. > + * > + * If the number of targets does not MAX_INLINE_SYNC_TARGETS, we present a ShareMethodTextButton > + * for each of them. > + * Otherwise, Otherwise, fix this comment.

All comments addressed. Ready to land sans the closed tree and my having no idea what your first review comment is trying to say. I will not be available until Tuesday. (In reply to Richard Newman [:rnewman] from comment #28) > ::: mobile/android/base/AndroidManifest.xml.in > @@ +376,5 @@ > > + <!-- Share overlay activity --> > > + <activity android:name="org.mozilla.gecko.overlays.ui.ShareDialog" > > + android:label="@string/overlay_share_header" > > + android:theme="@style/ShareOverlayActivity" > > + android:configChanges="keyboard|keyboardHidden|mcc|mnc|locale" > > Never specify `locale` without `layoutDirection`. Some explanation would be helpful... For Added the attribute, but unclear why this is necessary. > ::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java > @@ +21,5 @@ > > + > > +public class SendTabDeviceListArrayAdapter extends ArrayAdapter<ParcelableClientRecord> { > > + private static final String LOGTAG = "GeckoSendTabAdapter"; > > + > > + protected State currentState; > > Comment about synchronization. Move to be with its friends. Actually, this synchronisation appears to be redundant, since this object is only accessed from one thread. Cargo-culted from the adapter used in the original Sync Send Tab adapter. Unless there's some secret reason I don't know about for that to be synchronised, I'm fixing this by just removing the synchronisation. > @@ +61,5 @@ > > + } > > + > > + clear(); > > + > > + // Alas, addAll isn't available in all API versions we support... > > if (Versions.feature11Plus) { > addAll(records); > // TODO: find out if `addAll` notifies. `add` does. > } else { > setNotifyOnChange(false); // So we don't notify for each add. > for (ParcelableClientRecord record : records) { > add(record); > } > notifyDataSetChanged(); Ahh, I had no idea it did that. Whoops. My first ArrayAdapter, ;) Also, you led me astray with the code I was plagerising: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/activities/ClientRecordArrayAdapter.java#40 :P > @@ +108,5 @@ > > + return R.drawable.sync_mobile; > > + } > > + > > + if (record.type == null) { > > + return R.drawable.overlay_send_tab_icon; > > The fallback should be sync_desktop. This was a really dumb hack to handle the special state when we need to do something other than display a list of devices. It has been replaced by a less stupid hack that doesn't involve this very confusing check. > @@ +140,5 @@ > > + /** > > + * Get a dummy ParcelableClientRecord for use in one of the "button states". > > + */ > > + private void showDummyRecord(String name) { > > + Log.e("Fennec", "Showing dumm record: " + name); > > Typo, and should this just die? Yup, forgot to run my "Delete debug print statements" script.

(In reply to Chris Kitching [:ckitching] from comment #29) > > > + android:configChanges="keyboard|keyboardHidden|mcc|mnc|locale" > > > > Never specify `locale` without `layoutDirection`. > > Some explanation would be helpful... > For Added the attribute, but unclear why this is necessary. Save me typing: http://stackoverflow.com/questions/13856229/onconfigurationchanged-is-not-called-over-jellybean4-2-1

Comment on attachment 8482232 [details] [diff] [review] V6: Add share overlay frontend Review of attachment 8482232 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +96,5 @@ > +<!ENTITY overlay_share_bookmark_btn_label "Bookmark"> > +<!ENTITY overlay_share_reading_list_btn_label "Add to Reading List"> > +<!ENTITY overlay_share_header "Send to &brandShortName;"> > + > +<!ENTITY overlay_share_send_other "Send to other devices"> Newline after. ::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java @@ +71,5 @@ > + if (AppConstants.Versions.feature11Plus) { > + addAll(records); > + } else { > + for (ParcelableClientRecord record : records) { > + add(record); You missed a line from my review. setNotifyOnChange(false); // So we don't notify for each add. notifyDataSetChanged will re-enable automatic notification, so you just need to turn it off before you call all of your adds. ::: mobile/android/base/overlays/ui/SendTabList.java @@ +1,5 @@ > +/* > + * 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/. > + */ Newline after comment, and why are you using two different license templates? Compare to SendTabDeviceListArrayAdapter.java. ::: mobile/android/base/overlays/ui/ShareDialog.java @@ +143,5 @@ > + findViewById(R.id.sharedialog).startAnimation(anim); > + > + // Add button event listeners. > + > + // Bookmarks (These two completely boring) Remove. @@ +151,5 @@ > + addBookmark(); > + } > + }); > + > + // Reading list Remove. @@ +176,5 @@ > + Intent serviceIntent = new Intent(this, OverlayActionService.class); > + serviceIntent.setAction(OverlayConstants.ACTION_SHARE); > + > + serviceIntent.putExtra(OverlayConstants.EXTRA_SHARE_METHOD, (Parcelable) method); > + Remove this newline. @@ +213,5 @@ > + > + // Currently, only one extra parameter is necessary (the GUID of the target device). > + Bundle extraParameters = new Bundle(); > + // TODO: Handle multiple-selection subject to UX deciding what it should look like (and if it > + // should exist). File, add bug number here. ::: mobile/android/base/resources/anim/overlay_slide_up.xml @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?><!-- Split line on each of these anim block comments. ?xml line should be on its own. @@ +1,2 @@ > +<?xml version="1.0" encoding="utf-8"?><!-- > + ~ This Source Code Form is subject to the terms of the Mozilla Public '-', not '~' (both files). ::: mobile/android/base/resources/layout/overlay_share_dialog.xml @@ +66,5 @@ > + android:background="@color/overlay_share_background_colour" > + android:orientation="vertical"> > + > + <!-- TODO: Once API 11 is available, stick "showDividers=middle" into the parent and get rid > + of these evil separator views. --> Make two separate XML files. File a follow-up bug explaining the whys and hows.

I'm doing a build to verify locally, then I'll land these.