900744 - [fig] CursorIndexOutOfBoundsException when long tapping on search suggestion row

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

See bug 900148 comment 5.

Comment 1

[:Margaret Leibovic]

Attachment: WIP

This fixes the problem, but I'm a bit confused about how we're ending up in here when long tapping on a SearchEngineRow, since they're created in BrowserSearch, and that's not a HomeListView.

I actually just realized, though, that we'll need to offset the cursor position by 1 when search suggestions are enabled, for similar reason to bug 900746.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 784694 [details] [diff] [review]
WIP

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

::: mobile/android/base/home/HomeListView.java
@@ +59,5 @@
>      public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +        // Don't show a context menu for search engine rows.
> +        if (view instanceof SearchEngineRow) {
> +            return false;
> +        }

Drive-by: this doesn't really belong to HomeListView imo. SearchEngineRow is an implementation detail of BrowserSearch and, ideally, should not leak into cross-fragment code.

I'd probably add an OnItemLongClick listener to the HomeListView in BrowserSearch (or a subclass of HomeListView) and handle SearchEngineRow where appropriate there.

Comment 3

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> Comment on attachment 784694 [details] [diff] [review]
> WIP
> 
> Review of attachment 784694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeListView.java
> @@ +59,5 @@
> >      public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> > +        // Don't show a context menu for search engine rows.
> > +        if (view instanceof SearchEngineRow) {
> > +            return false;
> > +        }
> 
> Drive-by: this doesn't really belong to HomeListView imo. SearchEngineRow is
> an implementation detail of BrowserSearch and, ideally, should not leak into
> cross-fragment code.
> 
> I'd probably add an OnItemLongClick listener to the HomeListView in
> BrowserSearch (or a subclass of HomeListView) and handle SearchEngineRow
> where appropriate there.

Yeah, that sounds better. I was just confused about how this was called at all because I didn't see HomeListView being used in BrowserSearch, but now I see it's declared in the XML, so that makes sense.

All this new about:home work is definitely an adventure in Java programming patterns!

Comment 4

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Lucas Rocha (:lucasr) from comment #2)
> > Comment on attachment 784694 [details] [diff] [review]
> > WIP
> > 
> > Review of attachment 784694 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/HomeListView.java
> > @@ +59,5 @@
> > >      public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> > > +        // Don't show a context menu for search engine rows.
> > > +        if (view instanceof SearchEngineRow) {
> > > +            return false;
> > > +        }
> > 
> > Drive-by: this doesn't really belong to HomeListView imo. SearchEngineRow is
> > an implementation detail of BrowserSearch and, ideally, should not leak into
> > cross-fragment code.
> > 
> > I'd probably add an OnItemLongClick listener to the HomeListView in
> > BrowserSearch (or a subclass of HomeListView) and handle SearchEngineRow
> > where appropriate there.
> 
> Yeah, that sounds better. I was just confused about how this was called at
> all because I didn't see HomeListView being used in BrowserSearch, but now I
> see it's declared in the XML, so that makes sense.
> 
> All this new about:home work is definitely an adventure in Java programming
> patterns!

I agree. The long click should be handled by BrowserSearch. One tricky part here would be to re-use HomeListView's onItemLongClick() if the view is not a SeacrchEngineRow.

Comment 5

[:Margaret Leibovic]

Attachment: patch

New patch, inspired by my new patch for bug 900148.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 785218 [details] [diff] [review]
patch

Nice.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 785218 [details] [diff] [review]
patch

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

Cool.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/projects/fig/rev/e5b2a760d5d0

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e5b2a760d5d0