splitwise / TokenAutoComplete

Gmail style MultiAutoCompleteTextView for Android
Apache License 2.0
1.3k stars 384 forks source link

Introducing token selected and double selected events #369

Closed emitchel closed 5 years ago

emitchel commented 5 years ago

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

emitchel commented 5 years ago

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

mgod commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

emitchel commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Fair enough