1186529 - Replace RecyclerViewItemClickListener.OnClickListener with RecyclerViewClickSupport

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jonathan Almeida [:jonalmeida]]

We don't always need to implement onLongClick for every RecyclerView; sometimes we only want onClick.

For later reference, comments from Sebastian: 
"I wrote RecyclerViewItemClickListener for a specific use case where I needed to listen for normal clicks and long clicks. I thought about later uses cases where you just want to click handler and don't care about long clicks but I didn't want to write code that isn't used.

Maybe it makes sense to refactor RecyclerViewItemClickListener to handle these use cases without implementing empty methods. But that's something we can look at in a follow-up bug."

Comment 1

[Jonathan Almeida [:jonalmeida]]

Sebastian, while typing out this bug I thought of a solution that might work: Make two interfaces in RecyclerViewItemClickListener, OnClickListener and OnLongClickListener. Would only require minor refactoring since the current OnClickListener is used in a lot of places for now.

What do you think?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jonathan Almeida [:jonalmeida] from comment #1)
> Sebastian, while typing out this bug I thought of a solution that might
> work: Make two interfaces in RecyclerViewItemClickListener, OnClickListener
> and OnLongClickListener. Would only require minor refactoring since the
> current OnClickListener is used in a lot of places for now.
> 
> What do you think?

Sounds good! :)

Comment 3

[Jonathan Almeida [:jonalmeida]]

Attachment: recycler_click_listener_split.patch

I have a patch for this already (attached), but I won't submit it for review until bug 1184211 lands since it too needs to be changed.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Interesting article and probably nicer approach then using a GestureDetector like I did:
http://www.littlerobots.nl/blog/Handle-Android-RecyclerView-Clicks/

Comment 5

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc44c526feb6

Comment 6

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=456b5ad5923d

Comment 7

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa9ef5ec1a51

Comment 8

[Jonathan Almeida [:jonalmeida]]

I tried out the click listener in that article is pretty easy to use. I replaced our current one with it and tested it out, our home panels and search bar still work as normal.

Comment 9

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1186529 - Adding RecyclerViewClickSupport for click handling r?sebastian

Bug 1186529 - Remove mandatory onLongClick in RecyclerViewItemClickListener.OnClickListener r?sebastian

Comment 10

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1186529 - Adding ItemClickSupport for RecyclerView click handling r?sebastian

Bug 1186529 - Adding ItemClickSupport for RecyclerView click handling r?sebastian

Comment 11

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1186529 - Replacing OnClickListener with ItemClickSupport in PanelRecyclerView r?sebastian

Bug 1186529 - Replacing OnClickListener with ItemClickSupport in PanelRecyclerView r?sebastian

Comment 12

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15237/#review13643

::: mobile/android/base/home/ItemClickSupport.java:68
(Diff revision 1)
> +    public static ItemClickSupport removeFrom(RecyclerView view) {

Just wanted to point out, I haven't found a reason to use removeFrom anywhere in our usages, but I may be mistaken.

Comment 13

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15237/#review13643

If everything looks good and we decide to stay with this ClickListener, I'll fold all these commits into one.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8644164 [details]
MozReview Request: Bug 1186529 - Adding RecyclerViewClickSupport for click handling r?sebastian

https://reviewboard.mozilla.org/r/15239/#review13661

I'm ignoring this patch as we are probably going for the new approach.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8644165 [details]
MozReview Request: Bug 1186529 - Adding ItemClickSupport for RecyclerView click handling r?sebastian

https://reviewboard.mozilla.org/r/15241/#review13663

There are some small things that I'd change but overall this is already r+.

::: mobile/android/base/home/ItemClickSupport.java:68
(Diff revision 1)
> +    public static ItemClickSupport removeFrom(RecyclerView view) {

I agree that we don't have a usecase for removeFrom() and we probably won't have any in the near future. Feel free to minimize the class to what we actually need.

::: mobile/android/base/home/ItemClickSupport.java:13
(Diff revision 1)
> +public class ItemClickSupport {

We should explain in a comment what this class is good for (like the previous implementation did). 

Maybe we should rename it to make its usecase more clear? e.g. something like RecyclerViewItemClickSupport ... that's pretty long and confusing. Can you come up with something better?

Let's also add a comment mentioning which article inspired this code.

::: mobile/android/base/home/ItemClickSupport.java:92
(Diff revision 1)
> +

Let's get rid of these empty lines.

::: mobile/android/base/home/SearchEngineBar.java:24
(Diff revision 1)
> -        implements RecyclerViewItemClickListener.OnClickListener {
> +//        implements RecyclerViewItemClickListener.OnClickListener {

This comment should be gone in the final patch. :)

::: mobile/android/base/home/SearchEngineBar.java:73
(Diff revision 1)
> +        mItemClickSupport.setOnItemClickListener(this);

We don't need the class member here, do we? Let's use the fancy chaining API:

ItemClickSupport
    .addTo(this)
    .setOnItemClickListener(this);

::: mobile/android/base/resources/values/ids.xml:12
(Diff revision 1)
> +    <item type="id" name="item_click_support" />

If we are going to rename the class then maybe let's rename this one too.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8644165 [details]
MozReview Request: Bug 1186529 - Adding ItemClickSupport for RecyclerView click handling r?sebastian

https://reviewboard.mozilla.org/r/15241/#review13665

::: mobile/android/base/moz.build:329
(Diff revision 1)
> +    'home/ItemClickSupport.java',

This class is pretty much generic. We could move it to the widget package maybe.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8644166 [details]
MozReview Request: Bug 1186529 - Replacing OnClickListener with ItemClickSupport in PanelRecyclerView r?sebastian

https://reviewboard.mozilla.org/r/15243/#review13667

I see this patch already fixes some of the points mentioned when reviewing patch 2. :)

::: mobile/android/base/home/PanelRecyclerView.java:152
(Diff revision 1)
> +

NIT: Additional empty line?

Comment 18

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f503e70eab9b

Comment 19

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15241/#review13663

> We should explain in a comment what this class is good for (like the previous implementation did). 
> 
> Maybe we should rename it to make its usecase more clear? e.g. something like RecyclerViewItemClickSupport ... that's pretty long and confusing. Can you come up with something better?
> 
> Let's also add a comment mentioning which article inspired this code.

I decided to rename it to RecyclerViewClickSupport; a little long but I think it works.

Comment 20

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8644164 [details]
MozReview Request: Bug 1186529 - Adding RecyclerViewClickSupport for click handling r?sebastian

Bug 1186529 - Adding RecyclerViewClickSupport for click handling r?sebastian

Comment 21

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8644164 [details]
MozReview Request: Bug 1186529 - Adding RecyclerViewClickSupport for click handling r?sebastian

This is r+ with nits fixed. I lost sebastian's r+ when folding all my changesets into one.

¯\_(ツ)_/¯

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jonathan Almeida (:jonalmeida) from comment #19)
> I decided to rename it to RecyclerViewClickSupport; a little long but I
> think it works.

Perfect!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c9b41f67d832

Comment 24

[Carsten Book [:Tomcat]]

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