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

onTextChanged implementation #31

Closed
corbt opened this issue Oct 21, 2015 · 8 comments
Closed

onTextChanged implementation #31

corbt opened this issue Oct 21, 2015 · 8 comments

Comments

@corbt
Copy link
Contributor

corbt commented Oct 21, 2015

Previously EditText::onTextChanged (in Kotlin) accepted a lambda that was called with the updated text. So I could do something like:

-onTextChanged { txt -> setVal(txt); Anvil.render() }

Now, onTextChanged requires a full TextWatcher implementation. Of greater concern, it appears to be adding the watcher to the EditText over and over again, with every render. So after a few updates to the field the UI begins lagging noticeably.

The old onTextChanged syntax was sufficient for my needs and more concise. I'm planning on creating my own onTextChanged using the old syntax. Is this something that you would be interested in getting a PR for or not?

@zserge
Copy link
Collaborator

zserge commented Oct 21, 2015

Ah, that's the cool part of the new Anvil actually.

Of course TextWatcher is the ugliest listener in the whole Android SDK: there is no way to tell if it's already added to the view or not, you can not call setText() from inside it etc etc.
It required me to make a lot of kludges to make it work in the old Anvil.

In the new Anvil I still provide a TextWatcher setter, which removes the previous TextWatcher and adds the new one if you use lambdas. Of course, having one TextWatcher instance stored somewhere in class attributes is a solution to avoid extra work.

But there is a cool two-way data binding: text(StringBuilder). It allows you to get user input into the StringBuilder, or to modify the StringBuilder and thus update the text. No infinite recursion happens in this case.

@corbt
Copy link
Contributor Author

corbt commented Oct 21, 2015

Is there a way to subscribe to changes using the text(StringBuilder) binding?

@zserge
Copy link
Collaborator

zserge commented Oct 21, 2015

Oh.. I think the easiest way for now would be to override StringBuilder's replace method. Or cache TextWatcher instance and call Anvil.render() from there only when your text has actually been changed.

@zserge
Copy link
Collaborator

zserge commented Oct 21, 2015

It would be nice to have the TextWatcher DSL as it was in the old Anvil, but:

  • One should be careful about Anvil.render() inside TextWatcher callbacks - it would lead to long recursion if you call it when your input has been changed
  • One should keep the previously known user input somewhere to compare on the next callback run to "debounce" them
  • One should be careful about adding new TextWatcher instances to the views - it seems like TextWatcher's callbacks are fired when it's added to the view (even if there is no user input happening at that time).

@corbt
Copy link
Contributor Author

corbt commented Oct 21, 2015

I ended up slightly tweaking my UX to remove the need to watch for changes. I'm using the text(StringBuilder) binding now and it seems to be working fine for me.

@danhawkes
Copy link

@zserge Having problems getting this to work with RenderableAdapter. A few observations:

  • Watchers set via onTextChanged() don't seem to be removed when calling onTextChanged() again in a subsequent render cycle. This creates issues if the watcher references row-specific data (e.g. setting text for a specific row in the dataset) as obsolete watchers end up being called on mutation.

    The only way I've been able to work around this is to keep record of watchers assigned to views and manually remove them during the render, e.g.

    Map<EditText, TextWatcher> watchers = new HashMap<>();
    
    editText() {
        EditText view = Anvil.currentView();
    
        TextWatcher previousWatcher = watchers.get(view);
        if (previousWatcher != null) {
            view.removeTextChangedListener(previousWatcher);
        }
    
        TextWatcher newWatcher = new TextWatcher(/* … */);
        watchers.put(view, newWatcher);
        view.addTextChangedListener(newWatcher);
    }
    - 
  • Not sure if I've understood correctly, but isn't text(Stringbuilder) limited in that it can only render 'raw' model data? By that, I mean it's not possible to compute values in the render function, e.g. text(Integer.toString(value)) without implementing a separate watcher to update the model.

  • It's not possible to override StringBuilder.replace()(at least in regular java) as the class is final.

@zserge
Copy link
Collaborator

zserge commented Mar 10, 2016

@danhawkes Yeah... TextWatcher is so far the most painful part of Anvil. It relates to a broader topic of two-way data bindings and is on my todo list for a long time already. Still, I don't see a perfect solution yet because Android's TextWatcher is broken too much.

  1. I think BaseDSL.setTextChanged() should replace the textwatcher if the new instance is different from the old one. If it doesn't replace them - it's a bug. If you have a look at BaseDSL.setTextChanged() you may find it behaves very similar to your code.
  2. text(StringBuilder) is a hack I had to add to achieve simple two-way data binding. It a mutable string (which is a StringBuilder in Java) as a parameter. If you pass other (immutable) strings, like the ones received from toString() - you will have to add your own text watcher to get two-way data binding.
  3. Yes, unfortunately it is so, and I admit that StringBuilder is a poor choice for mutable strings and probably our own mutable string class would serve better.

I'm open to a discussion about how two-way data bindings should work in Anvil and how to simplify this process. I would prefer a solution to be more generic (e.g. cover not only EditTexts, but also Sliders, ToggleButtons and other views used for input).

@zserge
Copy link
Collaborator

zserge commented Mar 10, 2016

So far I could only think of two approaches:

  1. Use mutable data classes, so one could use text(mutableString) or progress(mutableInt) or checked(mutableBoolean)`. Each mutable class when changed may emit Anvil.render() and cause UI updates, also at any time one can get the value and receive the current input. To get notified when user does input one would have to override the class. This also forces user to use our own data classes in their view-models, which means view-model code becomes dependent on Anvil.
  2. Use simplified listeners (lambdas) with fromUser parameter. Then one could bind one (immutable) data to the view and register a simple lambda (fortunately lambdas a cheap and new Jack/Jill will bring them by default very soon). Inside a lambda one would write something like if (fromUser) { value = newValue; Anvil.render(); }.

Solution 1 requires less code from user and gives less control if you don't want to subclass your data classes.
Solution 2 makes user to write more code but is more explicit and gives more control.

@zserge zserge added this to the Two-way data binding milestone Mar 15, 2016
Mishkun pushed a commit to sgrekov/anvil that referenced this issue Dec 23, 2019
Add ability to set bidirectional animators in declarative style
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

No branches or pull requests

3 participants