Closed Bug 900744 Opened 8 years ago Closed 8 years ago

[fig] CursorIndexOutOfBoundsException when long tapping on search suggestion row

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #784694 - Flags: feedback?(sriram)
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.
(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!
(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.
Attached patch patchSplinter Review
New patch, inspired by my new patch for bug 900148.
Attachment #784694 - Attachment is obsolete: true
Attachment #784694 - Flags: feedback?(sriram)
Attachment #785218 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 785218 [details] [diff] [review]
patch

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

Cool.
Attachment #785218 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/e5b2a760d5d0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Depends on: 911828
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.