Closed Bug 1025546 Opened 10 years ago Closed 3 years ago

Support various image views in Hub panels

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: rnewman, Unassigned, Mentored)

Details

(Keywords: dev-doc-needed, Whiteboard: [lang=java][bad first bug])

Attachments

(8 files, 9 obsolete files)

(WIP) Add full-screen image panel
13.11 KB, patch
lucasr
: feedback+
Details | Diff | Splinter Review
icon_chev.zip
53.23 KB, application/zip
Details
ltgrey chevrons
187.01 KB, image/png
Details
grey chevrons
187.15 KB, image/png
Details
grey chevrons on image
617.22 KB, image/png
antlam
: feedback+
Details
icon_chev2.zip
54.14 KB, application/zip
Details
Part 1: Add chevron resources
35.85 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Part 2: Add full-screen image panel
31.13 KB, patch
Details | Diff | Splinter Review 
E.g., a full-size image, image grids a la top sites, scrollable sequence of images arranged vertically, an automatic 'carousel'.
This should be relatively straightforward, but it would be a non-trivial amount of work.

I think we should scope this out by starting with a full-size image view.

To add this new view, you would need to declare a new ViewType in HomeConfig.java.

The current view types we have expect to show multiple items, and how the items themselves are displayed is determined by the ItemType. If we follow this pattern, we could add something like a new FRAME ViewType, and then add something like a LARGE_IMAGE ItemType. This logic lives in PanelViewAdapter.java and PanelViewItem.java.

On the JS side, you'll need to add code to the panel validation code in Home.jsm to make sure these new types are recognized as valid.
Whiteboard: [mentor=margaret][lang=java][bad first bug]
I agree that we should scope out the work to focus on a specific image type and not as many as possible. With that in mind, I'd like Ian and UX to give some thought to this as well. I think a full-screen image would be a nice touch and give us a base for some different add-ons. Lets assume UX feels the same, I have some questions:

1. Displaying a single full screen image is fine, but we _do_ support multiple items/rows in Home panel datastorage. Should we consider a way to move between them as a slideshow?
2. What approach should we take for images that don't fit the aspect ratio exactly? Black letter boxing? Stretch to the narrow dimension and crop the larger one?
(In reply to Richard Newman [:rnewman] from comment #0)
> E.g., a full-size image, image grids a la top sites, scrollable sequence of
> images arranged vertically, an automatic 'carousel'.

We already support 'image grids' with the grid+image item combo and vertical list of images with the list+image item combo. Is this meant to be somehow different than what we already have?

The full screen image 'slideshow' should be fairly simple to implement with something like a ImageSwitcher backed by an dataset-backed adapter. But we need to come up with a good interaction for moving between images (keeping in mind that we can't use horizontal gestures because the panels live inside a ViewPager). Maybe arrow buttons that fade away after some time? Tapping on the left/right halves of the screen? Vertical swipes to reveal the next/previous image?

Ian, what do you think?
Flags: needinfo?(ibarlow)
(In reply to Lucas Rocha (:lucasr) from comment #3)
> (In reply to Richard Newman [:rnewman] from comment #0)
> > E.g., a full-size image, image grids a la top sites, scrollable sequence of
> > images arranged vertically, an automatic 'carousel'.
> 
> We already support 'image grids' with the grid+image item combo and vertical
> list of images with the list+image item combo. Is this meant to be somehow
> different than what we already have?

I imagine Richard may be referring to a layout that resembles Top Sites more closely -- images with some padding, and titles underneath? (Compared to our grid view which has almost no padding and close cropped images)

> But we
> need to come up with a good interaction for moving between images (keeping
> in mind that we can't use horizontal gestures because the panels live inside
> a ViewPager). Maybe arrow buttons that fade away after some time? Tapping on
> the left/right halves of the screen? Vertical swipes to reveal the
> next/previous image?
> 
> Ian, what do you think?

Thanks for asking!

I'm not sure I understand what you mean by fullscreen image slideshow. Do you imagine this hiding the entire browser UI? Or does the image simply take up the whole panel? Whatever the case may be, I want to avoid any form of left<-->right navigation to cycle through images, as Lucas said that interaction is already reserved for panel navigation. Using it for in-panel navigation as well feels too meta.

What about using up and down to switch between images? Either a swipe, or a set of button controls? And what kinds of UI transitions are available to us in home panels? The swipe could slide the top image away and reveal another image underneath, it could do a sort of "card-sort" animation where the foreground image shuffles to the back, it could crossfade...
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #4)

> I imagine Richard may be referring to a layout that resembles Top Sites more
> closely -- images with some padding, and titles underneath? (Compared to our
> grid view which has almost no padding and close cropped images)

Yes, Top Sites. Also images only, which I didn't remember us having.



> I'm not sure I understand what you mean by fullscreen image slideshow. Do
> you imagine this hiding the entire browser UI? Or does the image simply take
> up the whole panel?

When I wrote Comment 0, I envisioned:

* "Full-size image": no navigation.
* "Scrollable sequence of images arranged vertically": big pictures, scroll up and down. (Tumblr-style)
* "Automatic carousel": multiple images, rotated on a timer.

I think your transition points make sense for the carousel, too.
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=java][bad first bug] → [lang=java][bad first bug]
(In reply to Ian Barlow (:ibarlow) from comment #4)

> I'm not sure I understand what you mean by fullscreen image slideshow. Do
> you imagine this hiding the entire browser UI? Or does the image simply take
> up the whole panel? Whatever the case may be, I want to avoid any form of
> left<-->right navigation to cycle through images, as Lucas said that
> interaction is already reserved for panel navigation. Using it for in-panel
> navigation as well feels too meta.

Simply taking up the full panel is a good start.

> What about using up and down to switch between images? Either a swipe, or a
> set of button controls? And what kinds of UI transitions are available to us
> in home panels? The swipe could slide the top image away and reveal another
> image underneath, it could do a sort of "card-sort" animation where the
> foreground image shuffles to the back, it could crossfade...

Up/Down is a perfect idea for moving between images.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
I attempted to add a full-screen image panel, using the add-on at:

https://github.com/mcomella/panel-image-view-test/tree/v1

But the panel is never added to the homescreen and I'm not sure why.
Comment on attachment 8452803 [details] [diff] [review]
(WIP) Add full-screen image panel.

re comment 7: any ideas, Lucas?
Attachment #8452803 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8452803 [details] [diff] [review]
(WIP) Add full-screen image panel.

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

Looking generally good. I'd probably implement PanelImageView as a ImageSwitcher or something to easily implement transitions between images in the dataset.
Attachment #8452803 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Michael Comella (:mcomella) from comment #8)
> Comment on attachment 8452803 [details] [diff] [review]
> (WIP) Add full-screen image panel.
> 
> re comment 7: any ideas, Lucas?

My guess is that there's some validation issue (missing element, unrecognized values, etc) in the JSON you're sending to Java. Keep an eye on the "HomePanelsManager" tag in logcat.
This patch will add a full-screen image to the panel, though clicking has no
functionality. It can be tested via: https://github.com/mcomella/panel-image-view-test/tree/v2
(In reply to Lucas Rocha (:lucasr) from comment #10)
> My guess is that there's some validation issue (missing element,
> unrecognized values, etc) in the JSON you're sending to Java. Keep an eye on
> the "HomePanelsManager" tag in logcat.

It was actually a syntax issue with my js - I'm not sure why I didn't catch it before.
Attached patch Add full-screen image panel (obsolete) — — Splinter Review 
This patch creates a full-screen image panel with a single image. You can click
on the image to take you to the url associated with the item. This could be
used to make an addon like Wikipedia's picture of the day. An example addon is
here: https://github.com/mcomella/panel-image-view-test/tree/v2

I wasn't sure what to do with the FilterManager, or the ContextMenuInfoFactory
and long click listener.

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Looking generally good. I'd probably implement PanelImageView as a
> ImageSwitcher or something to easily implement transitions between images in
> the dataset.

I would like to save this for a followup since it seems like it would take
considerably more effort after playing around for a bit, particularly to add
well-functioning "next image" gestures. What do you think, Lucas?
Attachment #8459042 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8459042 [details] [diff] [review]
Add full-screen image panel

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

::: mobile/android/modules/Home.jsm
@@ +341,5 @@
>            // Use IMAGE item type by default in GRID views
>            view.itemType = Item.IMAGE;
> +        } else if (view.type === View.FULLSCREEN_IMAGE) {
> +          // Use IMAGE item type by default in FULLSCREEN_IMAGE views
> +          view.itemType = Item.IMAGE;

What would happen if someone tried to use a different item type here? It looks like we will just ignore it, so perhaps instead we should throw an exception to let the user know that other item types aren't supported for this view type.
Ian, how can I improve the UX of this panel?

Updated the patch to support multiple images with an ImageSwitcher. I wasn't
sure how to back this with an Adapter (as recommended in comment 3). There are
a few rough edges, marked by TODOs in the code.

I went with previous/next buttons for simplicity - the styles are not final.

I did not address Margaret's comment 14 yet.
Attachment #8459991 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8459042 - Attachment is obsolete: true
Attachment #8459042 - Flags: review?(lucasr.at.mozilla)
Quick thoughts:
1. Looking to change fullscreen_image to a more manageable name. We have "list" view and "grid" view. What about calling this one "slide" view?
2. Can we change the "Previous" and "Next" text to simple chevron/arrows?
3. In a followup bug maybe we could fade the chevrons/arrows away until you start panning?
Thanks Mike. Building on Mark's suggestions, I threw a quick sketch together: http://cl.ly/image/0Q3d0W0g1U0D/o

I see this panel working with two kinds of interactions. 
1. Tapping Next/Previous controls
2. Swiping up and down. 

The page should behave slightly differently depending on how people interact with it: 

1. If they tap the chevrons, the image should advance and the chevrons should stay visible. 
2. If they swipe to advance, the chevrons are no longer a necessary visual affordance so they can be hidden. Tapping the top or bottom of the panel can reveal them again. 

I should also note (in case it isn't clear) that the chevrons should be allowed to sit on top of the images, rather than forcing them to sit outside of the image. 

--

needinfoing Anthony for some "Next / Previous" arrow assets
Flags: needinfo?(alam)
Comment on attachment 8459991 [details] [diff] [review]
(WIP) Add full-screen image panel

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

Looks like pretty good start. I'd probably try to find a shorter name for this view. PanelFullscreenImageView is way too long :-P

::: mobile/android/base/home/PanelFullscreenImageView.java
@@ +50,5 @@
> +        PREVIOUS,
> +        NEXT
> +    }
> +
> +    private class ImageItem implements Target {

nit: I'd move this class to the end of PanelFullscreenImageView.

@@ +51,5 @@
> +        NEXT
> +    }
> +
> +    private class ImageItem implements Target {
> +        protected final String url;

Why protected?

@@ +78,5 @@
> +                // the drawable, so wait for just that.
> +                return;
> +            }
> +
> +            // TODO: Placeholder images?

Yeah, we probably want a placeholder in case there's an error (and probably some UI to trigger the image request again).

@@ +102,5 @@
> +
> +        @Override
> +        public void onBitmapFailed(final Drawable errorDrawable) {
> +            Log.e(LOGTAG, "Failed to load the bitmap: " + imageURL);
> +            // TODO: Display "failed" bitmap. What is errorDrawable?

I think it's defined by the error image resource ID you set in the Picasso call i.e. error(image_id)

@@ +111,5 @@
> +            // Do nothing.
> +        }
> +    }
> +
> +    private static final String LOGTAG = "Gecko" + PanelFullscreenImageView.class.getSimpleName();

Isn't this too long for the LOGTAG? I think we have a 23 char limit.

@@ +127,5 @@
> +        // TODO: Panel resizes with keyboard.
> +        setOrientation(LinearLayout.VERTICAL);
> +        final ViewGroup.LayoutParams lp = new ViewGroup.LayoutParams(
> +                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);
> +        setLayoutParams(lp);

Is this actually necessary? I think we use these LayoutParams by default on panel views.

@@ +151,5 @@
> +        imageSwitcher = (ImageSwitcher) findViewById(R.id.image_switcher);
> +        imageSwitcher.setFactory(this);
> +        imageSwitcher.setAnimateFirstView(false);
> +
> +        setOnClickListener(new View.OnClickListener() {

Maybe this should be set on the imageSwitcher instead?

@@ +174,5 @@
> +
> +    @Override
> +    public View makeView() {
> +        final ImageView v = new ImageView(getContext());
> +        v.setScaleType(ScaleType.FIT_CENTER);

Maybe CENTER_CROP to ensure the image fills the whole area? Ask ibarlow.

@@ +176,5 @@
> +    public View makeView() {
> +        final ImageView v = new ImageView(getContext());
> +        v.setScaleType(ScaleType.FIT_CENTER);
> +        v.setLayoutParams(new FrameLayout.LayoutParams(
> +                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT));

This is FrameLayout's default LayoutParams, no need to set them explicitly here.

@@ +224,5 @@
> +            final String url = cursor.getString(urlIndex);
> +            final String title = cursor.getString(titleIndex);
> +            final String imageURL = cursor.getString(imageIndex);
> +
> +            imageItems.add(new ImageItem(url, title, imageURL, currentIndex));

Why creating a separate data structure to hold the list of images when we already have the cursor? Seems unnecessary.
Attachment #8459991 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #19)
> Comment on attachment 8459991 [details] [diff] [review]
> (WIP) Add full-screen image panel
> 
> Review of attachment 8459991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like pretty good start. I'd probably try to find a shorter name for
> this view. PanelFullscreenImageView is way too long :-P

PanelSlideView
Attached file icon_chev.zip — 
(In reply to Ian Barlow (:ibarlow) from comment #18)
> needinfoing Anthony for some "Next / Previous" arrow assets

Here you go! I'm trying to keep it small and out of the way for now with two colors, a grey and an ultra light grey. Will have to play with these on the actual phone before further.
Flags: needinfo?(alam)
(In reply to Lucas Rocha (:lucasr) from comment #19)
> @@ +51,5 @@
> > +        NEXT
> > +    }
> > +
> > +    private class ImageItem implements Target {
> > +        protected final String url;
> 
> Why protected?

I accidentally did the inner-class access stuff backwards in my head.

> Yeah, we probably want a placeholder in case there's an error (and probably
> some UI to trigger the image request again).

A placeholder would only be used when waiting for an image to load as we can use onBitmapFailed to display an error image, or view. Do you still think we should use a placeholder image?

> @@ +127,5 @@
> > +        // TODO: Panel resizes with keyboard.
> > +        setOrientation(LinearLayout.VERTICAL);
> > +        final ViewGroup.LayoutParams lp = new ViewGroup.LayoutParams(
> > +                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);
> > +        setLayoutParams(lp);
> 
> Is this actually necessary? I think we use these LayoutParams by default on
> panel views.

This isn't for the panel view: it's for the inner image views, which by default appear to wrap_content.

> @@ +176,5 @@
> > +    public View makeView() {
> > +        final ImageView v = new ImageView(getContext());
> > +        v.setScaleType(ScaleType.FIT_CENTER);
> > +        v.setLayoutParams(new FrameLayout.LayoutParams(
> > +                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT));
> 
> This is FrameLayout's default LayoutParams, no need to set them explicitly
> here.

I think it's good to be explicit in this case.

> @@ +224,5 @@
> > +            final String url = cursor.getString(urlIndex);
> > +            final String title = cursor.getString(titleIndex);
> > +            final String imageURL = cursor.getString(imageIndex);
> > +
> > +            imageItems.add(new ImageItem(url, title, imageURL, currentIndex));
> 
> Why creating a separate data structure to hold the list of images when we
> already have the cursor? Seems unnecessary.

I suppose we can use the Cursor directly, but the Cursor isn't a good interface to accessing fields, nor for encapsulating image storing/loading: it's better encapsulated with ImageItem. Do you really think it should be changed?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Ian Barlow (:ibarlow) from comment #18)
> I see this panel working with two kinds of interactions. 
> 1. Tapping Next/Previous controls
> 2. Swiping up and down.

The swipe interactions change the implementation details significantly and I'm not quite sure how to approach them - how would you feel if this landed with just tapping controls initially, and the swiping was implemented later in a followup?
Flags: needinfo?(ibarlow)
Attached image ltgrey chevrons — 
The light grey chevrons are basically invisible on the default panel color.
Attached image grey chevrons on image — 
What do you think, Anthony?
Attachment #8460677 - Flags: feedback?(alam)
(In reply to Michael Comella (:mcomella) from comment #23)
> (In reply to Ian Barlow (:ibarlow) from comment #18)
> > I see this panel working with two kinds of interactions. 
> > 1. Tapping Next/Previous controls
> > 2. Swiping up and down.
> 
> The swipe interactions change the implementation details significantly and
> I'm not quite sure how to approach them - how would you feel if this landed
> with just tapping controls initially, and the swiping was implemented later
> in a followup?

Not great, to be honest. How much extra work is it?
Flags: needinfo?(ibarlow)
Potential third option, in case swiping is hard: how about carouseling on a timer, as mentioned briefly in Comment 0?

No interaction handling needed at all, leaves time to build solid swipe handling…
Comment on attachment 8460677 [details]
grey chevrons on image

(In reply to Michael Comella (:mcomella) from comment #26)
> Created attachment 8460677 [details]
> grey chevrons on image
> 
> What do you think, Anthony?

While I would agree that dark looks better here, I think we really have to get the interactions going to see for sure. I can also add some visual affordance to the actual hit area of the chevrons so they don't feel so "hard to touch" as well. 

Is there a build maybe we could try with some of the photos from Margaret's Instagram panel? Also, how far away are the icons from the screen edge?
Attachment #8460677 - Flags: feedback?(alam) → feedback+
Flags: needinfo?(michael.l.comella)
Getting the images from the instagram panel will take a bit of effort, so I settled for some hardcoded images from Wikipedia's picture of the day series. You can install the apk at [1], and visit [2] in the browser, installing the addon there.

Note:
* The hit targets for the up and down buttons are much smaller than they should be and are hard to press
* The panel state resets when a link is clicked or the device is rotated (bug 1041840)

[1]: https://people.mozilla.org/~mcomella/apks/mcomella-1025546_01.apk
[2]: https://people.mozilla.com/~mcomella/addons/mcomella-1025546_01.xpi
Flags: needinfo?(michael.l.comella)
And with light grey chevrons: https://people.mozilla.com/~mcomella/apks/mcomella-1025546_02.apk

Anthony, what do you think?

Some ideas:
* To use the light grey chevrons, change the panel background color (though it may be inconsistent with other home panels)
* Have a semi-transparent overlay of a different color around the chevrons, which also displays the hit area (which can fade over time, though this is slightly different than Ian's design)
Flags: needinfo?(alam)
Ian, how do you feel about Richard's comment 28?
Flags: needinfo?(rnewman)
(In reply to Anthony Lam (:antlam) from comment #29)
Also, how far away are the icons from the screen edge?

The chevrons are currently 12dp away from the edge; the hit target extends 12dp below them and to the left and right edges of the screen.
(In reply to Michael Comella (:mcomella) from comment #32)
> Ian, how do you feel about Richard's comment 28?

I just tried the build, and even though I knew I couldn't scroll, the first thing I instinctively did was try to scroll. The panel misinterpreted that as a click, and took me to the URL of the image. It was a brutally janky and suprising experience. 

Every other panel on about:home responds to a swipe interaction. Making this one behave differently makes me very nervous. 

Also, Anthony I think those chevrons are way too small and difficult to see.
Flags: needinfo?(rnewman)
Attachment #8459992 - Attachment is obsolete: true
Attachment #8459992 - Flags: feedback?(ibarlow)
(In reply to Michael Comella (:mcomella) from comment #33)
> The chevrons are currently 12dp away from the edge; the hit target extends
> 12dp below them and to the left and right edges of the screen.

I think the hit areas should span the width of the screen like we had in the mock ups.

Tested this a bit on my Nexus 5:
 - I found the hit area of the image to be very sensitive (I kept loading the full image), maybe adding scrolling will help that
 - The arrows are on the smaller side - I'll look at increasing the size and including transparency since I want to avoid covering up the beautiful full sized images
 - The hit area of the arrows in the build is hard to hit (can we try a 48 dp height with full screen width?
 - We might also want to consider the background color since a lot of the images don't actually fit perfectly within the bounds of the panel, it's pretty common to see borders.
 - Could we get a quicker sliding animation?
Flags: needinfo?(alam)
Ian, do you think the swiping interaction should be equivalent to the home panels themselves, except in the vertical direction? Or should it not be pageinated?
Flags: needinfo?(ibarlow)
(In reply to Michael Comella (:mcomella) from comment #36)
> Ian, do you think the swiping interaction should be equivalent to the home
> panels themselves, except in the vertical direction? Or should it not be
> pageinated?

Sorry Mike, I'm not sure I follow your question
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #37)
> Sorry Mike, I'm not sure I follow your question

Spoke on IRC - the slides should be pageinated, HomePager style.
Clearing NEEDINFO as this patch is going to be reworked.
Flags: needinfo?(lucasr.at.mozilla)
I just want to note I'll be working in a separate project [1] to simplify the view interactions. Currently, my view, using the ViewDragHelper, has vertical drag/fling behavior similar to the ViewPager's horizontal drag/fling behavior.

[1]: https://github.com/mcomella/AndySandbox/tree/1025546-vert_page
Attached patch Part 1: Add chevron resources (obsolete) — — Splinter Review 
Unfinalized part 1 for resources, just in case someone wants to build this patch series.
This patch adds a WIP fullscreen image panel with a some TODOs that mention
work that still needs to be done. The functionality is approximately the arrow
buttons will scroll the ImageViews and the ImageViews are scrollable by hand
with pageinated behavior (similar to the ViewPager).

Notably, the following still needs to be worked on before it's shippable:
  * Comment 35 (chevrons and hit zones)
  * Constants when scrolling by finger need to be tweaked.
  * (Maybe) Adding an overscroll animation
  * Actions to take onClick/onLongClick
Attachment #8465106 - Flags: feedback?(lucasr.at.mozilla)
Anthony, do you have any updated assets for the chevrons (see comment 35)?
Flags: needinfo?(alam)
Adjusted onViewReleased to act more like the ViewPager.
Attachment #8465910 - Flags: review?(lucasr.at.mozilla)
Attachment #8465106 - Attachment is obsolete: true
Attachment #8465106 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8465910 - Flags: review?(lucasr.at.mozilla) → feedback?(lucasr.at.mozilla)
Comment on attachment 8465910 [details] [diff] [review]
Part 2: Add full-screen image panel

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

Here's an initial batch of comments to get you going. Looking good overall.

::: mobile/android/base/home/PanelSlideView.java
@@ +31,5 @@
> + * A home panel that displays a full-screen image,
> + * without distorting the image proportions.
> + */
> +@SuppressLint("ViewConstructor")
> +class PanelSlideView extends RelativeLayout

Why RelativeLayout? Isn't it just a linear layout in practice?

@@ +46,5 @@
> +        super(context);
> +
> +        // TODO: Panel resizes with keyboard.
> +        final ViewGroup.LayoutParams lp = new ViewGroup.LayoutParams(
> +                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);

getLayoutParams() -> set width/height values -> setLayoutParams() to avoid creating a new (redundant) LayoutParams instance. Why is this necessary?

::: mobile/android/base/widget/HomeItemsImageViewCursorAdapter.java
@@ +20,5 @@
> +/**
> + * A cursor adapter that returns ImageViews from the given
> + * Cursor with the HomeItems data storage pattern.
> + */
> +public class HomeItemsImageViewCursorAdapter extends CursorAdapter {

Wow, let's try to find a more concise name for this class :-P

@@ +63,5 @@
> +    public void bindView(final View view, final Context context, final Cursor cursor) {
> +        // TODO: Store column indices from a method such as swapCursor for efficiency?
> +        final int imageURLIndex = cursor.getColumnIndexOrThrow(HomeItems.IMAGE_URL);
> +        final String path = cursor.getString(imageURLIndex);
> +        // TODO: Can we make this fade in?

Picasso fades in images by default. Isn't it working?

@@ +64,5 @@
> +        // TODO: Store column indices from a method such as swapCursor for efficiency?
> +        final int imageURLIndex = cursor.getColumnIndexOrThrow(HomeItems.IMAGE_URL);
> +        final String path = cursor.getString(imageURLIndex);
> +        // TODO: Can we make this fade in?
> +        // TODO: Cache images.

Picasso caches images for us. No need to do anything here.

@@ +65,5 @@
> +        final int imageURLIndex = cursor.getColumnIndexOrThrow(HomeItems.IMAGE_URL);
> +        final String path = cursor.getString(imageURLIndex);
> +        // TODO: Can we make this fade in?
> +        // TODO: Cache images.
> +        // TODO: Resize images for memory and perf improvements.

You can try using fit() here? It will resize the image according to the target view bounds.

@@ +74,5 @@
> +    }
> +
> +    @Override
> +    public View newView(final Context context, final Cursor cursor, final ViewGroup parent) {
> +        // TODO: Why parent is arg?

The parent is passed in here for the cases where you can inflater here and it needs to figure out what are the default LayoutParams for the host container. See: http://www.doubleencore.com/2013/05/layout-inflation-as-intended/

@@ +76,5 @@
> +    @Override
> +    public View newView(final Context context, final Cursor cursor, final ViewGroup parent) {
> +        // TODO: Why parent is arg?
> +        // Note that the overridden getView depends on this
> +        // method being independent of cursor state.

The cursor state would only be important here if you needed to know the item type to use for the target position.

::: mobile/android/base/widget/VerticalPagedView.java
@@ +21,5 @@
> +public class VerticalPagedView extends AdapterView<HomeItemsImageViewCursorAdapter> {
> +    /* inner-access */ HomeItemsImageViewCursorAdapter adapter;
> +
> +    /* inner-access */ final ViewDragHelper dragHelper;
> +    /* inner-access */ int viewDragRange;

What is this inner-access about?

@@ +34,5 @@
> +
> +    public VerticalPagedView(final Context context, final AttributeSet attrs) {
> +        super(context, attrs);
> +        dragHelper = ViewDragHelper.create(this, new VerticalPagedViewCallback());
> +        layoutParams = new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT);

Reusing LayoutParams across different child views is very bug-prone. This means changing a LayoutParams in one view will affect the other. This might cause breakage if the changes to the other child view is not followed by a requestLayout() call in it (which setLayoutParams() does for you in the target view). Create one instance per child view.

@@ +64,5 @@
> +        if (!previousPage()) {
> +            return false;
> +        }
> +
> +        // TODO: Double-tapping causes a view not to slide in.

Maybe use GestureDetector to tell single from double taps?

@@ +148,5 @@
> +
> +    @Override
> +    public boolean onInterceptTouchEvent(final MotionEvent ev) {
> +        final int action = MotionEventCompat.getActionMasked(ev);
> +        // TODO: Why cancel and false? (it's from a tutorial).

A false turn in onInterceptTouchEvent means that the container will not continue handling events in its own onTouchEvent anymore i.e. motion events will be forwards to its children as usual. In this context, this means it will not continue to intercept touch events to handle dragging gestures in the ViewDragHelper.
Attachment #8465910 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #45)
> Why RelativeLayout? Isn't it just a linear layout in practice?

The chevrons overlap the VerticalPagedView. Is there any way to do that with a LinearLayout?

> Picasso fades in images by default. Isn't it working?
> 
> Picasso caches images for us. No need to do anything here.

No, but there's also a delay when images are loaded back onto the device, which makes me wonder if something isn't configured correctly, or I'm doing something wrong.

> @@ +76,5 @@
> > +    @Override
> > +    public View newView(final Context context, final Cursor cursor, final ViewGroup parent) {
> > +        // TODO: Why parent is arg?
> > +        // Note that the overridden getView depends on this
> > +        // method being independent of cursor state.
> 
> The cursor state would only be important here if you needed to know the item
> type to use for the target position.
> 
> What is this inner-access about?

If an inner class access a private member of the outer class, Java secretly creates a `protected` getter and setter for that member, which is bad for the byte code bloat and runtime hit of having to make a function call. There are efforts to remove these instances from the code in bug 1039898 and "/* inner-access */" is the convention used in that bug.

> @@ +64,5 @@
> > +        // TODO: Double-tapping causes a view not to slide in.
> 
> Maybe use GestureDetector to tell single from double taps?

Tapping twice causes an change of index by 2 but the animation on occurs only on the first item, while the second item will pop in after the first animation completes. Given the animation speed, I think it's better to queue taps and continue the animations as necessary.
(In reply to Lucas Rocha (:lucasr) from comment #45)
> @@ +76,5 @@
> > +        // Note that the overridden getView depends on this
> > +        // method being independent of cursor state.
> 
> The cursor state would only be important here if you needed to know the item
> type to use for the target position.

Right - I added this comment to clarify that the cursor position will not always be accurate to the item of the view (because we accept indices such as -1), but I guess I should have just written that instead - I'll update the comment.
(In reply to Michael Comella (:mcomella) from comment #22)
> > This is FrameLayout's default LayoutParams, no need to set them explicitly
> > here.
> 
> I think it's good to be explicit in this case.

Going through the Android source, if I read it correctly, any LayoutParams set before the views are added are overwritten with the default, so I removed the associated lines in my latest patch (which appear to have been using the default anyway).
Attached file icon_chev2.zip — 
Attaching larger icons :)
Flags: needinfo?(alam)
Attached patch (WIP) OnItemLongClick (obsolete) — — Splinter Review 
I can't get long click working: this is my best attempt. Lucas, any ideas?
Attachment #8468101 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8468101 [details] [diff] [review]
(WIP) OnItemLongClick

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

::: mobile/android/base/home/PanelSlideView.java
@@ +64,5 @@
>                      final int position, final long id) {
>                  itemHandler.openItemAtPosition(imageSwitcher.getAdapter().getCursor(), position);
>              }
>          });
> +        imageSwitcher.setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {

You'll need to trigger it yourself by intercepting long presses on the view. Maybe use GestureDetector? This should give you a good reference on how to do it: https://github.com/lucasr/twoway-view/blob/master/core/src/main/java/org/lucasr/twowayview/TWItemClickListener.java

See also TwoWayView to see how contextmenuinfo is handled: http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/TwoWayView.java#5440
Attachment #8468101 - Flags: feedback?(lucasr.at.mozilla)
Pretty much done except for a few annoying things that I don't think should
prevent this from landing, but should probably be fixed before release:
  * We should hide the appropriate arrow if we're at the edge of the adapter (0
or getCount() - 1).
  * When the panel is refreshed, when an image is clicked, or the device is
rotated, the view index is reset and the panel starts at page 0. I filed
another bug for how often setDataset is called with null.

Other things TODO are left as TODOs in the code, and will be filed as followup
bugs (looks like a good place to open up a lot of mentored bugs!).

One thing that must be fixed:
  * I don't ever reset viewIndex when the Cursor is swapped in the adapter - I
should get this, and other potential resets, working
Attachment #8468960 - Flags: feedback?(lucasr.at.mozilla)
Friendly reminder to update the docs on MDN when this lands :)

We could also mention this in the release notes as a developer feature.
Keywords: dev-doc-needed
I reset the ViewIndex when the dataset is updated now, and hide the the arrows at the edge indices.
Attachment #8469695 - Flags: review?(lucasr.at.mozilla)
Attachment #8468960 - Attachment is obsolete: true
Attachment #8468960 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8469678 [details] [diff] [review]
Part 1: Add chevron resources

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

antlam-approved I assume.
Attachment #8469678 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8469695 [details] [diff] [review]
Part 2: Add full-screen image panel

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

Had a good look at VerticalPagedView but want to spent some more time on it to give more useful feedback. These are my nitty comments and questions for now.

::: mobile/android/base/home/PanelSlideView.java
@@ +36,5 @@
> +class PanelSlideView extends RelativeLayout
> +                     implements DatasetBacked,
> +                                PanelView,
> +                                ViewSwitcher.ViewFactory {
> +    /* inner-access */ static final String LOGTAG = "Gecko" + PanelSlideView.class.getSimpleName();

Not used anywhere.

@@ +57,5 @@
> +
> +        LayoutInflater.from(context).inflate(R.layout.panel_slide_view, this);
> +
> +        imageSwitcher = (VerticalPagedView) findViewById(R.id.image_switcher);
> +        upButton = findViewById(R.id.up);

nit: Group findViewById calls with their respective code chunks? In other words:

imageSwitcher = (VerticalPageview) findViewByid()...
imageSwitcher.setAdapter()
...

upButton = viewViewById(R.id.up);
upbutton.setOnClickListener(...)
...

downButton = viewViewById(R.id.up);
downbutton.setOnClickListener(...)

@@ +124,5 @@
> +    @Override
> +    public void setFilterManager(final FilterManager manager) {
> +        // TODO: Figure this out, though it probably doesn't matter if we only show one image.
> +        // Adapter too?
> +        itemHandler.setFilterManager(manager);

You can probably just do nothing here. IIRC, PanelViewItemHandler handles null filter manager correctly.

::: mobile/android/base/widget/HomeItemsImageAdapter.java
@@ +59,5 @@
> +        } else {
> +            v = convertView;
> +        }
> +
> +        // We can only bind views that have data backing them.

We should just throw if the cursor cannot be moved to the given position here. Just like we do in PanelViewAdapter:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelViewAdapter.java#97

@@ +71,5 @@
> +    }
> +
> +    @Override
> +    public void bindView(final View view, final Context context, final Cursor cursor) {
> +        // TODO: Can we make this fade in?

Is it still not fading in? Does it work for other panel views?

@@ +73,5 @@
> +    @Override
> +    public void bindView(final View view, final Context context, final Cursor cursor) {
> +        // TODO: Can we make this fade in?
> +        // TODO: Sometimes loaded images appear over the current image when scrolling. Cancel the
> +        // request or use a pool of views.

This looks like we're not recycling views properly in VerticalPagedView then. Picasso handles view recycling and cancels requests for us.

@@ +74,5 @@
> +    public void bindView(final View view, final Context context, final Cursor cursor) {
> +        // TODO: Can we make this fade in?
> +        // TODO: Sometimes loaded images appear over the current image when scrolling. Cancel the
> +        // request or use a pool of views.
> +        // TODO: Why doesn't fit work?

What exactly is not working?

@@ +75,5 @@
> +        // TODO: Can we make this fade in?
> +        // TODO: Sometimes loaded images appear over the current image when scrolling. Cancel the
> +        // request or use a pool of views.
> +        // TODO: Why doesn't fit work?
> +        final String path = cursor.getString(imageURLIndex);

path -> imageUrl for clarity.
(In reply to Lucas Rocha (:lucasr) from comment #57)
> nit: Group findViewById calls with their respective code chunks?

The listeners cross-reference the other views and I preferred to keep setting the listeners for a View together over finding the views next to the first listener they set. Do you disagree?

> ::: mobile/android/base/widget/HomeItemsImageAdapter.java
> @@ +59,5 @@
> > +        } else {
> > +            v = convertView;
> > +        }
> > +
> > +        // We can only bind views that have data backing them.
> 
> We should just throw if the cursor cannot be moved to the given position
> here. Just like we do in PanelViewAdapter:

I like the consistency, but it complicates the view handling in VerticalPagedView because we can no longer expect topView, centerView, and bottomView to be initialized. I didn't find it was worth the trade-off - what do you think?

> Is it still not fading in? Does it work for other panel views?

No and 

> @@ +73,5 @@
> > +        // TODO: Sometimes loaded images appear over the current image when scrolling. Cancel the
> > +        // request or use a pool of views.
> 
> This looks like we're not recycling views properly in VerticalPagedView
> then. Picasso handles view recycling and cancels requests for us.

I think the problem is that the top, center, and bottom views translate when scrolled, and then reposition themselves back to the starting location with different image assets. However, if you scroll two pages quickly, a View load may be queued for centerView from the second page page, but won't appear until the third, at which point the second page's image pops in, and is quickly replaced by the third page's image.

I wrote some (commented out) code to shuffle the views around so this is less likely to be an issue (e.g. 3 views scroll down as we go to the previous page, put the bottom-most view at the top), but it doesn't seem to make a noticeable impact. Maybe I am incorrectly recycling these views or making requests.

> @@ +74,5 @@
> > +        // TODO: Why doesn't fit work?
> 
> What exactly is not working?

The images do not appear at all. Maybe it has something to do with the fact that I did not override onMeasure.
Comment on attachment 8469695 [details] [diff] [review]
Part 2: Add full-screen image panel

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

The inner machinery of VerticalPagedView seems directly coupled with the (cursor) adapter and image loading logic, which goes against the general idea of an adapter view. Generally speaking, this view should:
- In onLayout(), detach all views, call adapter.getView for current position and maybe for the previous and next images (if possible).
- When dragging:
  - Detach views that went off screen, probably move it to a small pool of recycled views to be reused in future adapter.getView() calls.
  - Attach newly visible view, create it using adapter.getView() with one of the recycled views as the 'convertView' argument.

::: mobile/android/base/widget/VerticalPagedView.java
@@ +23,5 @@
> + * A vertically-oriented paged-ScrollView populated as a series of ImageViews backed by an adapter.
> + */
> +public class VerticalPagedView extends AdapterView<HomeItemsImageAdapter> {
> +    // This adapter must be able to handle out-of-bounds indices.
> +    /* inner-access */ HomeItemsImageAdapter adapter;

Why do you need to assume this is a HomeItemsImageAdapter? It should be simple to just assume a BaseAdapter here or something.

@@ +27,5 @@
> +    /* inner-access */ HomeItemsImageAdapter adapter;
> +
> +    private final GestureDetector gestureDetector;
> +    /* inner-access */ final ViewDragHelper dragHelper;
> +    /* inner-access */ int viewDragRange;

I find these comments very distracting and I'm not sure the micro-optimization is worth it in most cases to be honest. Not even the stock widgets on Android apply this optimization (and they are a lot more mission-critical than most of our UI code). Can't see why we'd need this.

@@ +36,5 @@
> +
> +    private boolean areViewsInitialized = false;
> +    /* inner-access */ View topView;
> +    /* inner-access */ View centerView;
> +    /* inner-access */ View bottomView;

Why not just lean on the list of children from ViewGroup i.e. getChildAt()/getChildCount()? Why do these fixed references give you?

@@ +55,5 @@
> +
> +    public void reset(final boolean shouldRequestLayout) {
> +        viewIndex = 0;
> +        if (onPageChangeListener != null) {
> +            onPageChangeListener.onPageChange(viewIndex, adapter.getCount());

What if the adapter is empty?

@@ +130,5 @@
> +        this.adapter = adapter;
> +        reset(true);
> +    }
> +
> +    public Cursor swapCursor(final Cursor newCursor) {

The view shouldn't really have APIs depending on Cursors. This is what adapter are for. You can track changes in the adapter with a DataSetObserver. Have a look at how TwoWayView does it.

@@ +137,5 @@
> +        return ret;
> +    }
> +
> +    @Override
> +    public View getSelectedView() {

Why do you need this?

@@ +143,5 @@
> +        return null;
> +    }
> +
> +    @Override
> +    public void setSelection(final int position) {

This should select a specific position without animations. Probably something that sets the current view index/position and calls requestLayout().

@@ +166,5 @@
> +
> +        return handled;
> +    }
> +
> +    // TODO: Override onMeasure?

Implement something more explicit here just to be clear. No need to cover all possible layout combinations though. Have a look at this blog post:

  http://lucasr.org/2014/05/12/custom-layouts-on-android/

The linked source code should give you a good idea of what you can do here.

@@ +174,5 @@
> +            final int bottom) {
> +        super.onLayout(changed, left, top, right, bottom);
> +
> +        // TODO: Is this the correct behavior? I didn't test. x_x
> +        if (adapter.getCursor() == null) {

You should use getCount() instead.

@@ +188,5 @@
> +
> +        if (!areViewsInitialized) {
> +            topView = adapter.getView(viewIndex - 1, null, this);
> +            centerView = adapter.getView(viewIndex, null, this);
> +            bottomView = adapter.getView(viewIndex + 1, null, this);

What happens to these views if the adapter has less than 3 items?

@@ +205,5 @@
> +        }
> +
> +        topView.layout(left, top - height, right, top);
> +        centerView.layout(left, top, right, bottom);
> +        bottomView.layout(left, bottom, right, bottom + height);

I don't see any code to hide (i.e. setVisibility(INVISIBLE)) the non-visible views here. This means the widget is drawing views unnecessarily.

@@ +217,5 @@
> +        // Note that we allocate one LayoutParams per view so that they are not shared.
> +        final int matchParent = LayoutParams.MATCH_PARENT;
> +        final LayoutParams topLayoutParams = new LayoutParams(matchParent, matchParent);
> +        final LayoutParams centerLayoutParams = new LayoutParams(matchParent, matchParent);
> +        final LayoutParams bottomLayoutParams = new LayoutParams(matchParent, matchParent);

Do you actually need to set LayoutParams here? Isn't the default good enough? i.e. just pass null LayoutParams here.

@@ +259,5 @@
> +                // and a click - a bad user experience. Thus we check both.
> +                if (!isDragging &&
> +                        dragHelper.getViewDragState() != ViewDragHelper.STATE_DRAGGING &&
> +                        !wasLongPressHandled) {
> +                    // We don't use row IDs.

Why not?

@@ +284,5 @@
> +
> +    private class VerticalPagedViewCallback extends ViewDragHelper.Callback {
> +        @Override
> +        public boolean tryCaptureView(final View child, final int pointerId) {
> +            return child == centerView;

nit: enclose with parens.

@@ +316,5 @@
> +        }
> +
> +        @Override
> +        public void onViewDragStateChanged(final int state) {
> +            switch (state) {

nit: Replace the switch+if with a single if given that you're only handling STATE_IDLE anyway?

@@ +331,5 @@
> +                        // through Picasso a large amount of time to complete. Also,
> +                        // cancel any pending load requests on the reused View.
> +                        if (oldViewIndex < viewIndex) {
> +                            ImageLoader.with(getContext())
> +                                       .cancelRequest((ImageView) topView);

This should be handled by the adapter's getView() instead. This view shouldn't really need to know anything about the specifics of its children.

@@ +383,5 @@
> +            return viewDragRange;
> +        }
> +    }
> +
> +    public interface OnPageChangeListener {

nit: declare this at the top of the class.
Attachment #8469695 - Flags: review?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #59)
> The inner machinery of VerticalPagedView seems directly coupled with the
> (cursor) adapter and image loading logic, which goes against the general
> idea of an adapter view. Generally speaking, this view should:
> - In onLayout(), detach all views, call adapter.getView for current position
> and maybe for the previous and next images (if possible).

Why should we detach all views in onLayout()? That seems inefficient since we'll just be re-adding views from the pool anyway.

> - When dragging:
>   - Detach views that went off screen, probably move it to a small pool of
> recycled views to be reused in future adapter.getView() calls.
>   - Attach newly visible view, create it using adapter.getView() with one of
> the recycled views as the 'convertView' argument.

AdapterView overrides all of the addView methods, except addViewInLayout, to ensure these views are only added in onLayout. Do you intend for this attaching and detaching to occur in onLayout? This seems like a less efficient version of what I'm doing now (since we're detaching views that don't need to be detached), however, with the addition of using a View Pool to manage recycled Views.

> @@ +27,5 @@
> > +    /* inner-access */ int viewDragRange;
> 
> I find these comments very distracting and I'm not sure the
> micro-optimization is worth it in most cases to be honest. Not even the
> stock widgets on Android apply this optimization (and they are a lot more
> mission-critical than most of our UI code). Can't see why we'd need this.

As mentioned in bug 1039898 comment 0, this saves us ~60k on our apk size. The performance implications are probably negligible in this case.

For Android, they probably don't need it because they don't care as much about compiled code size vs. accidentally opening up private members to people using the framework, shipping different code from the one they write, etc.. Ideally, the access level changes are automated, but I don't think we have that choice at this point.

If you have an issue with the convention, I'd recommend commenting in bug 1039898.

> @@ +36,5 @@
> > +    /* inner-access */ View centerView;
> 
> Why not just lean on the list of children from ViewGroup i.e.
> getChildAt()/getChildCount()? Why do these fixed references give you?

More readable code, the ability to reorder child views, and avoiding fragile assumptions (index 0 is the top view, 1 is the center view, etc.).

I'm not sure it's necessary, especially without the commented out code, but it seemed easier as I was writing the code. 

> @@ +137,5 @@
> > +    public View getSelectedView() {
> 
> Why do you need this?

It's an abstract method of AdapterView.

> @@ +188,5 @@
> > +            bottomView = adapter.getView(viewIndex + 1, null, this);
> 
> What happens to these views if the adapter has less than 3 items?

I overrode the adapter to permit out-of-bounds indices, where it would return a view with an invalid image (whatever it had leftover, or defaulted to). Since we never intend to draw these views, there were no issues with having too few items.

But since we determined the out-of-bounds indices doesn't make sense (ties the View too closely to the adapter), I'm going to change this.

> I don't see any code to hide (i.e. setVisibility(INVISIBLE)) the non-visible
> views here. This means the widget is drawing views unnecessarily.

I take it Android draws offscreen views then? I noticed that when I scrolled views off-screen (before layout), the performance improved substantially so I assumed it didn't (though maybe that's only with modern versions). I can do this.

> @@ +217,5 @@
> Do you actually need to set LayoutParams here? Isn't the default good
> enough? i.e. just pass null LayoutParams here.

You need to provide LayoutParams to addViewInLayout; it appears this is so that the system isn't allocating during layout. If I have a View Pool, I can associate the LayoutParams with each View and pool them, however.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #59)
> @@ +130,5 @@
> > +    public Cursor swapCursor(final Cursor newCursor) {
> 
> The view shouldn't really have APIs depending on Cursors. This is what
> adapter are for. You can track changes in the adapter with a
> DataSetObserver. Have a look at how TwoWayView does it.

I prevented VerticalPagedView from relying on Cursors, however, since we have `PanelSlideView.setDataset(Cursor)` (which calls `VerticalPagedView.getAdapter().swapCursor(Cursor)`) and `VerticalPagedView.setOnItemClickListener` (which calls `PanelViewItemHandler.openItemAtPosition(Cursor, int)`), I currently have VerticalPagedView extending `AdapterView<CursorAdapter>`.
What confused me a bit while reviewing this patch is that VerticalPagedView kinda tried to be a general-purpose AdapterView with support for view recycling but then makes some non-obvious assumptions around how the adapter works.

VerticalPagedView should look very similar to, say, PagerTitleStrip (it already does):
https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/view/PagerTitleStrip.java

We can simplify things a bit here by just changing VerticalPagedView to be backed by a cursor directly, no adapter and view recycling involved. You just create three views to show prev/current/next images and lay them out according to the current position in the cursor. We won't need to make VerticalPagedView a general-purpose AdapterView. Up to you.
Flags: needinfo?(lucasr.at.mozilla)
No longer actively working on this because it's low priority.
Assignee: michael.l.comella → nobody
Mentor: michael.l.comella
Status: ASSIGNED → NEW
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: