Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing token selected and double selected events #369

Closed
wants to merge 3 commits into from

Conversation

emitchel
Copy link

@emitchel emitchel commented Nov 25, 2018

AddedTokenClickStyle.DoubleSelect to allow double select functionality and introduced token selected listener functionality

@emitchel
Copy link
Author

@mgod, would love to get this in the next release

@mgod
Copy link
Contributor

mgod commented Nov 27, 2018

This seems like a pretty unintuitive interaction and it's adding another layer of complexity on the selection code, which I'd much prefer to avoid. Can you help me understand your use case?

@emitchel
Copy link
Author

Listening to when a token is selected has many use cases in itself, which this solves passively and simply.

The majority of the logic is the double tap listener, which I'd argue that the complexity is minimal since it's using the View's tag and keeping the library as stateless as possible. On the other hand, moving the logic out of the switch statement could greatly increase the readability in all scenarios.

I understand that there should be a line drawn in needless flexibility, but I believe this functionality is valuable to consumers.

Maybe a separate listener is warranted, for selecting and double selecting. Let me know.

@mgod
Copy link
Contributor

mgod commented Nov 28, 2018

In your code, what are you using the double tap behavior for?

@emitchel
Copy link
Author

The double tap functionality, in this specific client-driven scenario, is used for editing a recipient while filling out the "To: " field. Works out nicely since DoubleSelect has no effect on other types.

@mgod
Copy link
Contributor

mgod commented Nov 28, 2018

Got it, thanks for the clarification. I do see how this is useful in your specific case.

Unfortunately, I don't think supporting this behavior is something the core library should do. Double clicking in a mobile app is an unusual behavior and I don't think it makes a lot of sense to add these behaviors to an already complex set of view interactions.

This is also something that's possible to implement without changing the library. I saw you looked at #350, and the implementation note there is still valid for this. In order to get this behavior without changing the library:

  1. Configure your view to use SelectDeselect
  2. Override buildSpanForObject to return a custom subclass of TokenImageSpan
  3. Set the listener for your custom subclass
  4. In your subclass, override the onClick behavior with something like:
public void onClick() {
    Editable text = getText();
    if (text == null) return;

    boolean wasDoubleClicked = false;
    if (view.getTag()!= null) {//if reselecting, detect if it was a double click
        Long timeOfLastClick = (Long) view.getTag();
        if (timeOfLastClick != null && timeOfLastClick > 0) {
            long now = Calendar.getInstance().getTimeInMillis();
            wasDoubleClicked =  (now - timeOfLastClick) <= MAX_TIME_TO_BE_CONSIDERED_DOUBLE_CLICK
        }
    }

    if (wasDoubleClicked) {
        listener.onTokenDoubleSelected(token);
        view.setTag(null);
    } else {
        listener.onTokenSelected(token);
        //set the time it was selected
        view.setTag(Calendar.getInstance().getTimeInMillis());
        super.onClick();
    }
}                        

I agree that the switch statement makes things less readable, but my takeaway from the code is that I should only have implemented the behavior for Select and provided better tools for implementing more detailed behaviors instead of baking a small set into the library. If I do ever move away from the current set of click behaviors to something more generic, I'll try to provide a migration guide for anyone using less standard behaviors.

@emitchel
Copy link
Author

Fair enough

@emitchel emitchel closed this Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants