Closed Bug 1041026 Opened 7 years ago Closed 7 years ago

Use a single click listener for activating search mode

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: eedens, Assigned: Margaret)

References

Details

Attachments

(1 file)

Followup from bug 1022102. 

I see a couple of options

1) Have the parent group intercept touches and decide whether to deliver to the child editText. If it's RUNNING, deliver. If it's WAITING, don't deliver. Ref: https://developer.android.com/training/gestures/viewgroup.html

2) Increase the touchable area with a TouchDelegate

Ref: https://developer.android.com/training/gestures/viewgroup.html
Summary: Use a single click listener for activating → Use a single click listener for activating search mode
Feedback for https://github.com/leibovic/FirefoxSearch/commit/5bcde84c3241f808471bae3eb40e80f6319ec10e :

Overall, I think this is a good direction. :)

I feel like there are some overlapping concerns between SearchFragment and SearchBar that might make debugging tricky. At a superficial level, I see calls to 'onSearch' in both classes, and it takes awhile to unravel the code path. More importantly, there could be subtle side effects that would be a surprise for the uninitiated.  For example, calling searchBar.setText will cause a couple of events:
  - A change listener for the editText will fire
  - The editText will call filter on the SearchListener
  - The filter spawns a network request 
  -  etc ..

I'd recommend focusing the SearchBar into something less search-y. Suppose it's called ClearableEditText, and has this interface:

```
ClearableEditText
Methods:
  setText  // Probably include a boolean param to decide whether onChange should be fired
  clearText // Maybe redundant. Again, if included, include the boolean param
  setActive
  
  
Events:
  onChange  // Text changed
  onClear  // Text has been cleared; could be consolidated with onChange
  onCancel  // User wants to exit the search input (eg: clicking down/back on their keyboard, or clicking some cancel button)
  onSubmit // User wants to complete the search (eg: clicking OK button on keyboard, or clicking some submit button)
```

Given this generalized interface, SearchFragment binds each event to the proper service/thread/listener/activity/whatever. For example, in onSubmit:
  - Transition from RUNNING to WAITING
  - Send the search query to the parent activity.
I want to base my polish changes on top of this, so let's work towards landing it.

I added a comment about the onInterceptTouchEvent implementation. Let me know if there are other spots that you think need clarification.
Attachment #8460536 - Flags: review?(eedens)
Comment on attachment 8460536 [details] [diff] [review]
Move search bar logic into custom view

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

Looks good to me!
Attachment #8460536 - Flags: review?(eedens) → review+
https://hg.mozilla.org/mozilla-central/rev/4ebd3df15b4c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.