1289242 - Implement (full-screen) Activity Stream pager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P3)

Description

[Andrzej Hunt :ahunt]

With Activity Stream we now want to have a full-screen Activity (or just a fragment?) showing History, Bookmarks, Synced tabs.

See invision mock at:
https://mozilla.invisionapp.com/share/VW7ZBXV8Q#/screens/174476600

Comment 1

[Andrzej Hunt :ahunt]

We can probably start by reusing the existing HomePager, placing that in an Activity, and then changing the panels shown (we also need to add the search/close controls).

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, from the mocks I'd say:
* Use a separate activity
* Maybe just use a ViewPager and some new fragments - just to avoid having to deal with all the homepager legacy (but just try what's easier)

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Also: Let's skip synced tabs for now. They are part of the history panel now and we should discuss this before we go back in time and reverse everything we have done in the last months. ;)

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Implement basic ActivityStream-detail activity

This depends largely on the implementation of a modified Bookmarks
list in Bug 1288127.

Most functionality is not yet implemented, and the panel theming still
needs adjusting. (Anything that is accessed via the item contextmenus
should just work, opening links does not work.)

Review commit: https://reviewboard.mozilla.org/r/67810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67810/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: DO NOT USE - Bug 1289242 - Add button to activate AS detail view

Until we have content in the AS panel we can use this button to launch
the detail view.

Review commit: https://reviewboard.mozilla.org/r/67812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67812/

Comment 6

[Andrzej Hunt :ahunt]

One concern I have here is that the UI doesnt' seem too android'y: is it worth turning the top-bar into a true ActionBar with back/exit button on the left? (That would also be more consistent if we e.g. land the full-screen edit-bookmark dialog.)

Comment 7

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Allow overriding CombinedHistoryPanel adapter

This allows us to plug in a different adapter, which lets us override
the layouts/views used in the adapter, which in turn lets us modify the
styling of the items in the list.

Review commit: https://reviewboard.mozilla.org/r/67860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67860/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Override CombinedHistoryPanel styling for use in AS

Review commit: https://reviewboard.mozilla.org/r/67862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67862/

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8775674 [details]
Bug 1289242 - Implement basic ActivityStream-detail activity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67810/diff/1-2/

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8775675 [details]
DO NOT USE - Bug 1289242 - Add button to activate AS detail view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67812/diff/1-2/

Comment 11

[Andrzej Hunt :ahunt]

Navigation with this also gets tricky:
- opening a url directly is easy: we can return a URL through the Activity Result
- opening a url in background (context-menu -> open in new tab) is trickier. We want to use BrowserApp's onUrlOpenInBackground, but BrowserApp isn't guaranteed to be alive. Storing a list of backgrounded URLs (and opening that list the next time BrowserApp onResumes) might be a more robust way of handling this.

- Navigating back to the relevant history/bookmarks panel (press back button after opening a page) is also tricky. Some possibilities might be:
* write the panel URL into history (e.g. about:home?panel=bookmarks&scroll=... or about:home?panel-bookmarks&search=FOOBAR&scroll=...)
* Boolean flag to return to the panel if a URL was opened from the panel, and no other navigation has occured (simpler, possibly more confusing, possibly less confusing)

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #6)
> One concern I have here is that the UI doesnt' seem too android'y: is it
> worth turning the top-bar into a true ActionBar with back/exit button on the
> left? (That would also be more consistent if we e.g. land the full-screen
> edit-bookmark dialog.)

Yeah, just ping Bryan Bell or Aaron Benson on IRC/Slack/Bugzilla. Right now is the best time to iterate.

(In reply to Andrzej Hunt :ahunt from comment #11)
> - Navigating back to the relevant history/bookmarks panel (press back button
> after opening a page) is also tricky. Some possibilities might be:

I don't think that makes sense here. The screen opens *on top* of the browser (hiding all of the browser chrome) and when opening a link you go *back* to the browser. Pressing back should not make a view appear on top. (Think of a stack of paper and the back button removes the paper on the top).

Comment 13

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Implement ASOpenURLDelegate to handle URL loading from the detail activity

Review commit: https://reviewboard.mozilla.org/r/68002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68002/

Comment 14

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Use delegate to handle URLs from ASDetailActivity

Review commit: https://reviewboard.mozilla.org/r/68004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68004/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8775752 [details]
Bug 1289242 - Override CombinedHistoryPanel styling for use in AS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67862/diff/1-2/

Comment 16

[Andrzej Hunt :ahunt]

Attachment: Bug 1289242 - Avoid unchecked warning and check for correct type when casting EnumSet

Yes this is rather horrible.

Review commit: https://reviewboard.mozilla.org/r/68318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68318/

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8775674 [details]
Bug 1289242 - Implement basic ActivityStream-detail activity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67810/diff/2-3/

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8775675 [details]
DO NOT USE - Bug 1289242 - Add button to activate AS detail view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67812/diff/2-3/

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8775751 [details]
Bug 1289242 - Allow overriding CombinedHistoryPanel adapter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67860/diff/1-2/

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8775752 [details]
Bug 1289242 - Override CombinedHistoryPanel styling for use in AS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67862/diff/2-3/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8776073 [details]
Bug 1289242 - Implement ASOpenURLDelegate to handle URL loading from the detail activity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68002/diff/1-2/

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8776074 [details]
Bug 1289242 - Use delegate to handle URLs from ASDetailActivity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68004/diff/1-2/

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8776073 [details]
Bug 1289242 - Implement ASOpenURLDelegate to handle URL loading from the detail activity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68002/diff/2-3/

Comment 24

[Andrzej Hunt :ahunt]

Comment on attachment 8775674 [details]
Bug 1289242 - Implement basic ActivityStream-detail activity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67810/diff/3-4/

Comment 25

[Andrzej Hunt :ahunt]

Comment on attachment 8775675 [details]
DO NOT USE - Bug 1289242 - Add button to activate AS detail view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67812/diff/3-4/

Comment 26

[Andrzej Hunt :ahunt]

Comment on attachment 8775751 [details]
Bug 1289242 - Allow overriding CombinedHistoryPanel adapter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67860/diff/2-3/

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8775752 [details]
Bug 1289242 - Override CombinedHistoryPanel styling for use in AS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67862/diff/3-4/

Comment 28

[Andrzej Hunt :ahunt]

Comment on attachment 8776605 [details]
Bug 1289242 - Avoid unchecked warning and check for correct type when casting EnumSet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68318/diff/1-2/

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8775674 [details]
Bug 1289242 - Implement basic ActivityStream-detail activity

https://reviewboard.mozilla.org/r/67810/#review68302

::: mobile/android/base/AndroidManifest.xml.in:278
(Diff revision 4)
>          <activity android:name="org.mozilla.gecko.preferences.GeckoPreferences"
>                    android:theme="@style/Gecko.Preferences"
>                    android:configChanges="orientation|screenSize|locale|layoutDirection"
>                    android:excludeFromRecents="true"/>
>  
> +        <activity android:name="org.mozilla.gecko.home.activitystream.ASDetailActivity"

Any reason why we use AS instead of ActivityStream as prefix? :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2721
(Diff revision 4)
>  
>          if (mHomeScreen == null) {
>              if (org.mozilla.gecko.activitystream.ActivityStream.isEnabled(this)) {
>                  final ViewStub asStub = (ViewStub) findViewById(R.id.activity_stream_stub);
> -                mHomeScreen = (HomeScreen) asStub.inflate();
> +                ActivityStream activityStream = (ActivityStream) asStub.inflate();
> +                activityStream.setBrowserApp(this);

The context passed to the view is the Activity (BrowserApp). So you should be able to just cast it to BrowserApp when needed.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ASDetailActivity.java:53
(Diff revision 4)
> +                case HISTORY:
> +                    return new CombinedHistoryPanel();
> +                case BOOKMARKS:
> +                    HomeFragment bookmarksPanel = new CombinedHistoryPanel();

I'd be very careful with re-using the CombinedHistoryPanel:

* It was written for history only. Showing bookmarks and other items here makes this view very complex.
* This panel is used in our release version. If we use it for experimenting with AS features then we risk breaking the release app and might have to chase bugs.
* At one point, after some iterations, we will ship AS and at this point we only need to support what AS needs. By then we might have a super complex, super configurable panel that we don't necessarily need.

Could we move shared functionality to a base class and keep the different panel implementations separate? You decide.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ASDetailActivity.java:99
(Diff revision 4)
> +        ViewPager pager = (ViewPager) findViewById(R.id.as_pager);
> +
> +        pager.setAdapter(new DetailFragmentPager(getSupportFragmentManager(), this));
> +
> +        TabLayout tabLayout = (TabLayout) findViewById(R.id.as_detail_tablayout);
> +        tabLayout.setupWithViewPager(pager);

I'm not really sure but I assume you do not need to call this if TabLayout is already a child of the ViewPager in the XML.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:31
(Diff revision 4)
> +    private WeakReference<BrowserApp> browserAppWeakReference = new WeakReference<BrowserApp>(null);
> +
> +    public void setBrowserApp(final BrowserApp browserApp) {
> +        this.browserAppWeakReference = new WeakReference<>(browserApp);
> +    }

As said above, you can just cast the context object.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:82
(Diff revision 4)
> +        BrowserApp browserApp = browserAppWeakReference.get();
> +
> +        if (browserApp == null) {
> +            return;
> +        }
> +
> +        Intent i = new Intent(getContext(), ASDetailActivity.class);
> +        browserApp.startActivityForResult(i, BrowserApp.ACTIVITY_REQUEST_AS_DETAIL);

In this case you won't need a BrowserApp and can just call startActivity() on the context returned from getContext().

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8775674 [details]
Bug 1289242 - Implement basic ActivityStream-detail activity

https://reviewboard.mozilla.org/r/67810/#review68310

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8776073 [details]
Bug 1289242 - Implement ASOpenURLDelegate to handle URL loading from the detail activity

https://reviewboard.mozilla.org/r/68002/#review68314

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2717
(Diff revision 3)
>          if (mDynamicToolbar.isEnabled()) {
>              mDynamicToolbar.setVisible(true, VisibilityTransition.IMMEDIATE);
>          }
>  
>          if (mHomeScreen == null) {
> -            if (ActivityStream.isEnabled(this)) {
> +            if (org.mozilla.gecko.activitystream.ActivityStream.isEnabled(this)) {

Is this needed?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ASOpenURLDelegate.java:38
(Diff revision 3)
> + * Persisting background URLs is important because it's possible for the app to be killed between
> + * "opening" the URL (e.g. "open in new tab" from the context menu, after which we remain in
> + * the detail activity) and resuming BrowserApp (the app could be killed in between use of the context
> + * menu and resuming BrowserApp).
> + */
> +public class ASOpenURLDelegate extends BrowserAppDelegate {

Quick question (Reflag me for review): Did you try opening the tabs through Tabs.getInstance().loadXX(..)? Is this problematic too?

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8776073 [details]
Bug 1289242 - Implement ASOpenURLDelegate to handle URL loading from the detail activity

https://reviewboard.mozilla.org/r/68002/#review68320

Comment 33

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #29)
> 
> I'd be very careful with re-using the CombinedHistoryPanel:
> 
> * It was written for history only. Showing bookmarks and other items here
> makes this view very complex.
> * This panel is used in our release version. If we use it for experimenting
> with AS features then we risk breaking the release app and might have to
> chase bugs.
> * At one point, after some iterations, we will ship AS and at this point we
> only need to support what AS needs. By then we might have a super complex,
> super configurable panel that we don't necessarily need.
> 
> Could we move shared functionality to a base class and keep the different
> panel implementations separate? You decide.

It looks like this should be possible: I split the CombinedHistoryPanel into an SectionedPanel, and a subclass HistoryPanel. The new SectionedBookmarksPanel can extend from SectionedPanel. (I've kept support for smartfolders in the shared SectionedPanel, which means we could potentially add smartfolders to the Bookmarks Panel - however smartfolders are then provided by the specific panel implementation, i.e. 2 in History, 0 in Bookmarks, and we could have further panels that are AS specific without smartfolders as needed).

I've got the new split architecture working locally, but it still needs some cleanup before it's reviewable.

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #33)
> I've got the new split architecture working locally, but it still needs some
> cleanup before it's reviewable.

Okay! :)

I'll clear the other review requests here until then.

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

Closing: This is not part of the MVP. The current plan for later stages is to have a different kind of stream (no pager). However we might be able to reuse the activity bits from this patch.