1030896 - Add ability to dismiss search history items on launch screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_search_toolbar_mock1_spec.png

Attaching a design spec sheet to clean up the visuals for the "Search History" that users will see when they launch the app (having used it before, blank state design not attached in this bug).

General idea is to only display additional information for relatively recent searches and progressively less/ more general information for older searches. 

The 'x' would dismiss said items so this list could be somewhat curated and controlled actively by the user.

Relevant to bug 1022100.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_searchhistory_mock1.png

attaching non-spec version

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

These mocks are very provider specific, which we don't intend to support right away. We kinda need a spec for a generic search history, without ties to the provider or a potential native app.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_termshistory_mock1.png

Attaching a simple mock to cover our initial versions as we talked about. This covers primarily just the searched "terms".

Pretty straight forward, 3-dot menu is also included in here if we need a place to put that for the time being.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_termshistory_mock1_spec.png

Basic specs attached.

Comment 5

[Ian Barlow (:ibarlow)]

Hm, want to try simplifying it to individual rows instead of little tiles like that?

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah - that's a good point. I was struggling with that but ultimately decided that I wanted to post something before the weekend. 

Will give this more thought over the weekend. I was also thinking this "more clumped together" view could be for searches further back in history, or some sort of two tiered organization.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8446690 [details]
prev_search_toolbar_mock1_spec.png

Marking as obsolete because I messed up the unit conversion, and also since this won't be initial run anyways.. back to the drawing board!

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_searchhistory_mock4_terms2.png

Here's a basic list of searched terms with a dismiss button. The grey would be the hit state and the hit area. 

Just as a heads up, I'd like to try to load these in with a little bit more intent and dismiss them with animation as well. Maybe to the side? Will play with that in QC and get a .MOV mock up to show at a later date :)

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_suggestions_dismiss.png

Attaching newer mocks

Comment 10

[:Margaret Leibovic]

I'm going to fix most of the styles here as part of bug 1041738, but let's keep this bug open to deal with the dismiss button.

Comment 11

[:Margaret Leibovic]

See also: bug 1038232.

Comment 12

[:Margaret Leibovic]

antlam and I talked about this a bit yesterday. I think it would be nice to add gesture support to dismiss these items with a swipe. That would also fix the issue of figuring out when/how to expose this "x" button in the item.

Comment 13

[Eric Edens [:eedens]]

> I think it would be nice to add gesture support to dismiss these items
> with a swipe.

If we're going to make the history items swipable, then we need to rethink having suggestions look like history items.

By making history items swipable, we're training users that anything that looks like a history items is swipable. This is great if we're consistent. Otherwise it's frustrating.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Why can't we dismiss history items too? :)

Comment 15

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #14)
> Why can't we dismiss history items too? :)

I think you meant to say suggestions here. While we *could* let a user swipe a suggestion off to the side, as soon as they start typing more it would probably re-appear, since we're just getting these suggestions from whatever search service API we're using.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just talked to Margaret about this, I think we've cleared up the terminology a bit.

For the time being, let's make this swiping happen. I will follow up with a bug to look at the design of the "typing" stage screen.

Comment 18

[:Margaret Leibovic]

I have a WIP for swipe-to-dismiss kinda working here:
https://github.com/mozilla/fennec-search/pull/64

I'm running into a problem where an empty space in the list appears after the animation finishes.

wesj, maybe you want to take a look at this? You can use the magic of git to check out my branch if you want to play with it yourself :)

Comment 19

[:Margaret Leibovic]

I can pick this up again if nobody else takes it, but I'm not actively working on this at the moment.

Comment 20

[Brian Nicholson (:bnicholson)]

I can take a look at this one.

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 8446716 [details]
prev_searchhistory_mock1.png

According to Margaret on IRC, these mockups are obsolete and we aren't going to show an X button at all.

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8460379 [details]
prev_suggestions_dismiss.png

The shadow and x of the left is what I was thinking for this bug... not sure why it was marked as obsolete...

Comment 23

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #22)
> Comment on attachment 8460379 [details]
> prev_suggestions_dismiss.png
> 
> The shadow and x of the left is what I was thinking for this bug... not sure
> why it was marked as obsolete...

I thought that we decided we just wanted the swipe-to-dismiss behavior. If we were to have an x, how would the user reveal it?

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #23)
> I thought that we decided we just wanted the swipe-to-dismiss behavior. 

Ah, THAT'S why I obsolete'd it. Good call Margaret!

Now I remember, this was an older design that was designed around more of a "swipe to reveal more actions" type of interaction. But, for something that only has one action (and is V1), this isn't as necessary.

So let's just do a "swipe to dismiss" behavior.

Do you have enough to go on here Brian?

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 8460379 [details]
prev_suggestions_dismiss.png

I think so -- if there's no x button, it sounds like it will just be a simple slide out animation followed by a collapse of the empty space.

I found a small ListView library (https://github.com/47deg/android-swipelistview) that sounds like it does exactly what we want, so I'll see if this works instead of trying to reinvent this.

Comment 26

[Brian Nicholson (:bnicholson)]

This is taking a bit longer than I was hoping. I imported SwipeToDismissNOA (https://github.com/JakeWharton/SwipeToDismissNOA) which adds this behavior without much effort, but the problem is that once the slide-in animation happens to replace the removed view, there's a brief flicker. It's likely a layout happening briefly before the dataset gets updated, which Margaret was probably hitting in comment 18. I'll see if I can find a fix.

Comment 27

[Brian Nicholson (:bnicholson)]

Attachment: Import SwipeToDismissNOA

Nick, here's a ripoff of the code sample you sent me. Only change was adding the jar as a search dependency.

Comment 28

[Brian Nicholson (:bnicholson)]

Attachment: Add swipe-to-dismiss to PreSearchFragment list

So the problem here was that notifyDataSetChanged doesn't take immediate effect, so once the animation is done, we briefly see the deleted item pop back and flicker since the ListView still thinks it's in the list. setHasTransientState() forces the view for the entry to be preserved, so even though the ListView still thinks the animated item is part of the list, we can call this method to make sure that removed item stays attached to this out-of-bounds view.

The tricky part is deciding when to release this view so that the ListView can recycle it. As mentioned above, nDSC doesn't happen immediately, so we need to call this some point after the ListView has actually been updated. AFAIK, there's no simple callback we can hook into for a post-nDSC, so I added an arbitrary three second delay to release the transient state. I hate arbitrary timeouts in general, but since this is just for cleanup, I can live with it. Happy to hear any alternatives that could make this better, of course.

Comment 29

[Brian Nicholson (:bnicholson)]

Comment on attachment 8488311 [details] [diff] [review]
Add swipe-to-dismiss to PreSearchFragment list

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

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +198,5 @@
> +                                SearchHistory.CONTENT_URI,
> +                                SearchHistory.QUERY + " = ?",
> +                                new String[] { query });
> +                    }
> +                    return true;

Just realized this ignores the deletion result. Unless we actually care about it, I can change this return type to void.

Comment 30

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #29)
> Comment on attachment 8488311 [details] [diff] [review]
> Add swipe-to-dismiss to PreSearchFragment list
> 
> Review of attachment 8488311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
> @@ +198,5 @@
> > +                                SearchHistory.CONTENT_URI,
> > +                                SearchHistory.QUERY + " = ?",
> > +                                new String[] { query });
> > +                    }
> > +                    return true;
> 
> Just realized this ignores the deletion result. Unless we actually care
> about it, I can change this return type to void.

Yeah, I don't think we care about it, although maybe we should check the return value of delete and log an error if there's a problem actually deleting something.

Comment 31

[:Margaret Leibovic]

Comment on attachment 8488311 [details] [diff] [review]
Add swipe-to-dismiss to PreSearchFragment list

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

I'm glad you were able to get this to work. I agree it doesn't feel like the greatest solution, but you're right that this is just for cleanup, so it doesn't feel *too* bad. And if a device takes more than 3 seconds to delete the search history from the db, we'll probably have some more serious issues to deal with.

So, I would definitely review another version if you want to try to find a different solution, but I'd also be fine to land this and deal with problems later if we run into them.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +119,5 @@
>  
> +        // Create a ListView-specific touch listener. ListViews are given special treatment because
> +        // by default they handle touches for their list items... i.e. they're in charge of drawing
> +        // the pressed state (the list selector), handling list item clicks, etc.
> +        SwipeDismissListViewTouchListener touchListener = new SwipeDismissListViewTouchListener(listView, dismissCallback);

This is much simpler than my hacked-together implementation :) I wonder if we should try using this in the tabs tray as well, although I'm not sure what the state of that code is right now.

::: mobile/android/thirdparty/com/example/android/swipedismiss/SwipeDismissListViewTouchListener.java
@@ +355,5 @@
> +                                // updated and this view is no longer shown. Remove the transient
> +                                // state so the view can be recycled.
> +                                ViewCompat.setHasTransientState(pendingDismiss.view, false);
> +                            }
> +                        }, RESET_DELAY);

This seems like a pretty fundamental feature of this library. How do other people cope with this issue? Is the problem that we're using a CursorAdapter and updating the data asynchronously? That seems like it would be a pretty common use-case.

I agree with you that delays like this are a "code smell"... is there any way for us to expand the onDismiss API so that its implementation can tell SwipeDismissListViewTouchListener when to reset the views? It would be nice if we could do that ourselves after calling notifyDatasetChanged.

Comment 32

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #31)
> This seems like a pretty fundamental feature of this library. How do other
> people cope with this issue? Is the problem that we're using a CursorAdapter
> and updating the data asynchronously?

Yeah, exactly. There's a short window between the animation end and the update by the loader to actually get the new set of data for the list. The examples for these libraries always use simple synchronous adapters, such as ArrayAdapter, so using CursorAdapter adds some challenges.

> That seems like it would be a pretty common use-case.

Yeah -- there are a number of posts describing this issue with this library and similar ones:
* http://stackoverflow.com/questions/15468100/cursoradapter-backed-listview-delete-animation-flickers-on-delete
* https://github.com/timroes/EnhancedListView/issues/10
* https://github.com/timroes/SwipeToDismissUndoList/issues/16#issuecomment-24944637

> I agree with you that delays like this are a "code smell"... is there any
> way for us to expand the onDismiss API so that its implementation can tell
> SwipeDismissListViewTouchListener when to reset the views? It would be nice
> if we could do that ourselves after calling notifyDatasetChanged.

Doing it after notifyDataSetChanged is still too soon since the loader has to asynchronously fetch the data once that's called, so what we really need is to reset the views after the loader has actually gotten the data (in onLoadFinished). I started going down this road, but that means there will be more SwipeDismissListViewTouchListener calls scattered throughout the fragment, and using a timeout contained that. That said, explicitly clearing the views is The Right Way to do things, so I'll make another attempt before landing this.

Comment 33

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8488302 [details] [diff] [review]
Import SwipeToDismissNOA

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

I am fine with the actual import logic, but I'd like to get f+ from a more accomplished UI engineer (margaret? lucasr?) to form a consensus that this is a good library to import.

I'd also like a much expanded commit comment, explaining where the code comes from, that the comments are wrong (!?!), and that we're aware of the screaming BETA warning.  This all worries me, and I don't have time or expertise to judge its fitness.  I'd like said commit comment to be duplicated and expanded into mobile/android/thirdparty/com/example/android/swipedismiss/README.txt, giving context into this import.  So r+ on the build stuff, with nits; and a call for more eyes with more ability than my own.

It sounds like the actual library has a significant limitation when used with CursorAdapters; it would be good to capture that discussion somewhere.  That probably means a second comment-only commit attached to the relevant TouchListner classes.

Comment 34

[Brian Nicholson (:bnicholson)]

(In reply to Nick Alexander :nalexander from comment #33)
> I'd also like a much expanded commit comment, explaining where the code
> comes from, that the comments are wrong (!?!), and that we're aware of the
> screaming BETA warning.  This all worries me, and I don't have time or
> expertise to judge its fitness.

The second patch of this bug will make some modifications to this lib, so maybe we can fix up the warning and comments at the same time. But with these changes, this probably shouldn't still be a third party lib at all. We can import this into m/a/b since this is now based on -- but not a direct import of -- the original library. Does that work for you?

> It sounds like the actual library has a significant limitation when used
> with CursorAdapters; it would be good to capture that discussion somewhere. 
> That probably means a second comment-only commit attached to the relevant
> TouchListner classes.

Again, the second patch of this bug will be modifying the lib directly, so these limitations should be fixed for other clients we have using it.

Comment 35

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Nicholson (:bnicholson) from comment #34)
> (In reply to Nick Alexander :nalexander from comment #33)
> > I'd also like a much expanded commit comment, explaining where the code
> > comes from, that the comments are wrong (!?!), and that we're aware of the
> > screaming BETA warning.  This all worries me, and I don't have time or
> > expertise to judge its fitness.
> 
> The second patch of this bug will make some modifications to this lib, so
> maybe we can fix up the warning and comments at the same time. But with
> these changes, this probably shouldn't still be a third party lib at all. We
> can import this into m/a/b since this is now based on -- but not a direct
> import of -- the original library. Does that work for you?
 
Sure, in our namespace.

Comment 36

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Import SwipeToDismissNOA

Just giving this to Margaret since this is no longer a thirdparty lib import. Updated commit message:

SwipeToDismissNOA (https://github.com/JakeWharton/SwipeToDismissNOA) is
a port of Roman Nurik's swipe-to-dismiss library to use NineOldAndroids.
SwipeToDismissNOA adds swipe behavior to ListView that animates and
removes the item.

As-is, there are a few problems with this library:
* The comments about the v12 API requirements are wrong
* It has problems with async adapters, such as CursorAdapter
* The MotionEvent#obtain calls are not followed by recycle
* There are beta warnings in the comments, likely due to the problems above

As a result, this library is being imported to mobile/android/base
instead of the normal thirdparty lib directory so we can make the
necessary modifications. Subsequent patches in bug 1030896 will aim to
address these issues to make this code production-ready.

Comment 37

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Fixes to SwipeDismissListViewTouchListener

Fixes to the problems mentioned above.

I experimented more with doing cleanup in onLoadFinished instead of a timeout-based approach, and it while it mostly worked, there was still at least one difficult-to-fix issue. The issue was that there's still flicker when swiping multiple items at the same time, which happens because of the following:

1) Item 1 is swiped.
2) Item 2 is swiped (while item 1 is still animating).
3) The first onLoadFinished is fired from item 1's removal, but the data set still contains item 2. This is where the flicker happens.
4) The second onLoadFinished is fired from item 2's removal.

To completely fix this, I think we'd need to somehow associate onLoadFinished callbacks with the specific removals that triggered those callbacks, so only those views would be reset. Alternatively, we could use a proxy CursorWrapper that keeps track of the deleted items so we don't have to wait for the async DB updates.

Given how comparatively simple the timeout-based approach is, I'm starting to come around to it. Let me know if you have any other ideas! I'll also flag lucasr here since he has a lot of experience with ListViews and animations.

Comment 38

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Add swipe-to-dismiss to PreSearchFragment list

Comment 39

[:Margaret Leibovic]

Comment on attachment 8489615 [details] [diff] [review]
Part 1: Import SwipeToDismissNOA

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

I looked through this, but I didn't perform a rigorous review, so this is mostly a rubber stamp.

Comment 40

[:Margaret Leibovic]

Comment on attachment 8489630 [details] [diff] [review]
Part 2: Fixes to SwipeDismissListViewTouchListener

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

Given the issues we discussed, I'm fine with the timeout-based approach. However, we could file a follow-up bug to see if there's a better way to do this. If we can come up with a good solution, we could look into using this for the tabs tray as well.

Comment 41

[:Margaret Leibovic]

Comment on attachment 8489642 [details] [diff] [review]
Part 3: Add swipe-to-dismiss to PreSearchFragment list

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

Looks good. I wonder how this will look with my patches for bug 1051973.

Comment 42

[Lucas Rocha (:lucasr)]

Comment on attachment 8489630 [details] [diff] [review]
Part 2: Fixes to SwipeDismissListViewTouchListener

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

This timeout based approach is looking a bit too hand-wavy to me. As you mentioned before, the robust solution for this would be to mark certain positions as removed immediately after the swipe animation completes and 'commit' the removal once the loader refreshes the adapter with new cursor. From looking at ListView's code, the simplest way to do it would be to just mark the dismissed view with hasTransientState(). This will ensure that it will stay hidden until the next adapter change, when ListView clears all transient state views automatically on the following layout traversal.

The problem with relying on transient state views though is that transient state handling is no-op in pre-16. So, my suggestion is to mimic the approach I described above directly in the adapter. Something like:
1. In your onDismiss() callback code, do something like: adapter.markPositionAsRemoved(position). Store the list of positions marked for removal in a Set<Integer> or something.
2. In the adapter's getView(), force height = 0 on positions that are marked as removed.
3. Override the adapter's swapCursor() and clear the positions marked as removed there.

It seems margaret is ok with the timeout approach. I'm just suggesting what I consider a more future-proof path here. Up to you.

::: mobile/android/base/widget/SwipeDismissListViewTouchListener.java
@@ -1,1 @@
> -// THIS IS A BETA! I DON'T RECOMMEND USING IT IN PRODUCTION CODE JUST YET

Production-ready, yay! :-P

@@ +346,5 @@
> +                                setTranslationX(pendingDismiss.view, 0);
> +                                final ViewGroup.LayoutParams lp = pendingDismiss.view.getLayoutParams();
> +                                lp.height = originalHeight;
> +                                pendingDismiss.view.setLayoutParams(lp);
> +                                pendingDismiss.view.clearAnimation();

Isn't just resetting alpha and translation enough? Just curious about why clearAnimation() is necessary here (on Gingerbread, likely).

Comment 43

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Fixes to SwipeDismissListViewTouchListener, v2

(In reply to Lucas Rocha (:lucasr) from comment #42)
> The problem with relying on transient state views though is that transient
> state handling is no-op in pre-16.

Well, that makes me sad.

> So, my suggestion is to mimic the
> approach I described above directly in the adapter. Something like:
> ...

I implemented this and was *still* getting the flickering view for some reason. I'm not sure why -- it seems like that should work (similar to the alternative CursorWrapper approach I mentioned).

Playing around with other ideas, I went back and tried adding a RecyclerListener again, which had problems showing when swiping multiple items quickly. I decided to just drop support for this whole pending dismiss queue and allow only one dismiss at a time, and it seems to work pretty well. We're having to rewrite more of this library than I was hoping, but at least these changes make the code a bit simpler.

One of the reasons I liked the timeout-based fix is that it keeps the implementation fairly contained to SwipeDismissListViewTouchListener (and doesn't require all clients to make a custom adapter or CursorWrapper, for example). I think I like this version, too, for the same reasons.

> @@ +346,5 @@
> > +                                setTranslationX(pendingDismiss.view, 0);
> > +                                final ViewGroup.LayoutParams lp = pendingDismiss.view.getLayoutParams();
> > +                                lp.height = originalHeight;
> > +                                pendingDismiss.view.setLayoutParams(lp);
> > +                                pendingDismiss.view.clearAnimation();
> 
> Isn't just resetting alpha and translation enough? Just curious about why
> clearAnimation() is necessary here (on Gingerbread, likely).

Good question. I removed it and it still works on 4.4 and 2.3, so don't think this is needed.

(In reply to :Margaret Leibovic from comment #40)
> Given the issues we discussed, I'm fine with the timeout-based approach.
> However, we could file a follow-up bug to see if there's a better way to do
> this. If we can come up with a good solution, we could look into using this
> for the tabs tray as well.

I guess it's good that we're working this out now since there's already a demand for this library in other places: bug 735740, plus maybe the tabs tray like you mentioned :)

Comment 44

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Add swipe-to-dismiss to PreSearchFragment list, v3

Updated for the lib changes. Now have a ReyclerListener to attach, and now we get a single position in onDismiss instead of an array of positions.

Comment 45

[Lucas Rocha (:lucasr)]

Comment on attachment 8491118 [details] [diff] [review]
Part 2: Fixes to SwipeDismissListViewTouchListener, v2

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

I like the fact that this implementation encapsulates things a bit more but is is a bit racy because the dismissed view will re-appear if you scroll it off screen then on screen again before the loader returns. Unlikely to happen, yes, but this should at least be documented somewhere for future reference. I'll let margaret decide whether or not this is a safe compromise.

I'm a bit surprised that my suggested approach caused flickering. The only reason I can think of is that you're still marking views as having transient state somewhere. Or maybe there's something about the way the loader behaves that causes a 'premature' commit on some of the dismissed positions (in which case you'd have to check which of the dismissed position actually got removed in each swapCursor() call).

::: mobile/android/base/widget/SwipeDismissListViewTouchListener.java
@@ +164,5 @@
> +    /**
> +     * Returns a {@link android.widget.AbsListView.RecyclerListener} to be added to the
> +     * {@link ListView} using {@link ListView#setRecyclerListener(RecyclerListener)}.
> +     */
> +    public AbsListView.RecyclerListener makeRecycleListener() {

If your intent is to avoid forcing users of this view to do repeated work, and assuming this recycler is pretty much required for this view to work correctly, maybe it would be nicer if you:
1. Set this RecyclerListener here privately.
2. Override setRecyclerListener() to set a listener that is wrapped by the private one.

@@ +168,5 @@
> +    public AbsListView.RecyclerListener makeRecycleListener() {
> +        return new AbsListView.RecyclerListener() {
> +            @Override
> +            public void onMovedToScrapHeap(View view) {
> +                final Object tag = view.getTag();

This will likely break if the adapter uses the view's standard tag to store a ViewHolder, for example. Use a private tag by using a tag ID here i.e. define a 'original_height' ID in values/ids.xml and call setTag(R.id.original_height, height()) and getTag(R.id.original_height).

Comment 46

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #45)
 
> I like the fact that this implementation encapsulates things a bit more but
> is is a bit racy because the dismissed view will re-appear if you scroll it
> off screen then on screen again before the loader returns. Unlikely to
> happen, yes, but this should at least be documented somewhere for future
> reference. I'll let margaret decide whether or not this is a safe compromise.

I'm okay with this compromise, although I agree it would be good to document it. I would like to land this code so that we can see what it does in practice, and address follow-up issues as necessary.

> @@ +168,5 @@
> > +    public AbsListView.RecyclerListener makeRecycleListener() {
> > +        return new AbsListView.RecyclerListener() {
> > +            @Override
> > +            public void onMovedToScrapHeap(View view) {
> > +                final Object tag = view.getTag();
> 
> This will likely break if the adapter uses the view's standard tag to store
> a ViewHolder, for example. Use a private tag by using a tag ID here i.e.
> define a 'original_height' ID in values/ids.xml and call
> setTag(R.id.original_height, height()) and getTag(R.id.original_height).

This is a good point, considering the goal here is to make this a more generic widget we can use elsewhere.

Comment 47

[:Margaret Leibovic]

Comment on attachment 8491118 [details] [diff] [review]
Part 2: Fixes to SwipeDismissListViewTouchListener, v2

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

::: mobile/android/base/widget/SwipeDismissListViewTouchListener.java
@@ +107,5 @@
>           * @param listView               The originating {@link ListView}.
>           * @param reverseSortedPositions An array of positions to dismiss, sorted in descending
>           *                               order for convenience.
>           */
> +        void onDismiss(ListView listView, int position);

I like this API change. Update the documentation as well, though :)

Comment 48

[:Margaret Leibovic]

Comment on attachment 8491124 [details] [diff] [review]
Part 3: Add swipe-to-dismiss to PreSearchFragment list, v3

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

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +139,5 @@
> +                            Log.w(LOG_TAG, "Search query not deleted: " + query);
> +                        }
> +                        return null;
> +                    }
> +                }.execute();

I do worry a bit about the asynchronous nature of this call, and making sure that the adapter is up to date after the view is recycled, but I don't see a straightforward way to fix this.

Could consumers optionally tell SwipeDismissListViewTouchListener to hold onto the view until they say it's okay to recycle it? To do that, onDismiss could take a third listener parameter that we call when we're done with the dismissal. Or could we augment the API to optionally force the onDimiss call into a background thread in SwipeDismissListViewTouchListener and deal with this there? I'm just trying to brainstorm some approaches that don't involve messing with the adapter...

Maybe we should file a bug to at least acknowledge that there is a potential problem here. Although this may be an edge case in practice, it would make our app look buggy if a user runs into it.

Comment 49

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #45)
> If your intent is to avoid forcing users of this view to do repeated work,
> and assuming this recycler is pretty much required for this view to work
> correctly, maybe it would be nicer if you:
> 1. Set this RecyclerListener here privately.
> 2. Override setRecyclerListener() to set a listener that is wrapped by the
> private one.

For makeRecyclerListener, I was following the makeScrollListener pattern that's already used here. I'm confused about your suggestion -- wouldn't overriding setRecyclerListener require us to also use a custom ListView? Feel free to comment in the follow-up I'm filing.

> This will likely break if the adapter uses the view's standard tag to store
> a ViewHolder, for example. Use a private tag by using a tag ID here i.e.
> define a 'original_height' ID in values/ids.xml and call
> setTag(R.id.original_height, height()) and getTag(R.id.original_height).

Huh, I didn't know this version of setTag even existed. We should use this a lot more to avoid potential collisions!

(In reply to :Margaret Leibovic from comment #48)
> Could consumers optionally tell SwipeDismissListViewTouchListener to hold
> onto the view until they say it's okay to recycle it?

That's kind of what I tried to do in v1 of this patch, which was using setHasTransientState(). "setHasTransientState(true)" basically means "don't allow this view to be recycled until the transient state is reset to false". I'm not sure how else we could force SwipeDismissListViewTouchListener to hold onto the view since recycling is something that happens internally to ListView, and it's not something we explicitly trigger.

That said, maybe we can use both setHasTransientState and this recycler-based solution at the same time. For newer APIs, setHasTransientState could simply hold onto the view, preventing it from recycled, and this recycler listener code will have no effect. For older APIs, setHasTransientState is no-op, so we'd fall back to the recycler implemented here.

I'd also like to take a deeper look at the adapter-related issues I saw, so I agree that we should land this and talk more in a follow-up. Filed bug 1071283.

Comment 50

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/7fb7d61bc5d9
https://hg.mozilla.org/integration/fx-team/rev/70e9091da10d
https://hg.mozilla.org/integration/fx-team/rev/39234d590486

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7fb7d61bc5d9
https://hg.mozilla.org/mozilla-central/rev/70e9091da10d
https://hg.mozilla.org/mozilla-central/rev/39234d590486