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

How do I use Epoxy to create forms? #426

Open
ColtonIdle opened this issue Apr 24, 2018 · 32 comments
Open

How do I use Epoxy to create forms? #426

ColtonIdle opened this issue Apr 24, 2018 · 32 comments

Comments

@ColtonIdle
Copy link

Our team has been using Epoxy now for a few months, but noticed that you can also use it for static content. We first want to address our Login page. It's currently a scrollView with a linear layout, with an imageView, and a few editTexts and a button. How do I use Epoxy for a screen where I would typically be able to directly access all of the views? I didn't see any real docs/wiki surrounding this. Sorry if I missed it.

@elihart
Copy link
Contributor

elihart commented Apr 24, 2018

The quick answer is that you need to register a text watcher on the model and save the text value to a field whenever it changes. Rebuilding models should rely on that field to set the data. Models should also be rebuilt whenever the field is updated, but it is recommend to use requestDelayedModelBuild to avoid rebuilding on every character input.

Since models need to be rebuilt there is a surprisingly tricky issue with the cursor maintaining the correct position. Our app has a lot of pages with forms that we use Epoxy with, and it took us a while to get it right, but I created some utils to help and we have a pretty good pattern now.

I am leaving on a trip today, but I will try to share sample code and more details soon. It's something I've been meaning to do for a while because it isn't too straightforward and has some gotchas to it.

#218 is slightly related and you could read through that for some details

@soyjimmysaenz
Copy link

Hi @elihart .. we really need those utils 😄 We want to build something like Eureka (a library for iOS) but for Android. We would appreciate your help.

@elihart
Copy link
Contributor

elihart commented May 29, 2018

@yesez5 Sorry for the delay, things have been busy. That's great that you want to work on an Android version of Eureka though!

Here is the pattern we use, it's pretty straightforward. This doesn't include the part of the code that handles the TextWatcher and callback to rebuild models, if you are confused about that part let me know and I can provide another snipped.

I still mean to provide an official wiki entry and sample for this, but hopefully I can unblock you in the meantime

@TextProp
    public void setInputText(@Nullable CharSequence text) {
        if (setText(editText, text)) {
            // If the text changed then we move the cursor to the end of the new text. This allows us to fill in text programmatically if needed,
            // like a search suggestion, but if the user is typing and the view is rebound we won't lose their cursor position.
            editText.setSelection(editText.length());
        }
    }

    public static boolean setText(TextView textView, @Nullable CharSequence text) {
        if (!isTextDifferent(text, textView.getText())) {
            // Previous text is the same. No op
            return false;
        }


        textView.setText(text);

        return true;
    }

    /**
     * @return True if str1 is different from str2.
     * <p>
     * This is adapted from how the Android DataBinding library binds its text views.
     */
    public static boolean isTextDifferent(@Nullable CharSequence str1, @Nullable CharSequence str2) {
        if (str1 == str2) {
            return false;
        }
        if ((str1 == null) || (str2 == null)) {
            return true;
        }
        final int length = str1.length();
        if (length != str2.length()) {
            return true;
        }

        if (str1 instanceof Spanned) {
            return !str1.equals(str2);
        }

        for (int i = 0; i < length; i++) {
            if (str1.charAt(i) != str2.charAt(i)) {
                return true;
            }
        }
        return false;
    }

@Dimon94
Copy link

Dimon94 commented Aug 6, 2018

Hi @elihart, What is the best way to handles the TextWatcher and callback to rebuild models?
Now I use setOnFocusChangeListener() and unbind() to rebuild models.And I don't find the more clean way to handles the TextWatcher.I look forward to an official wiki entry and sample. Thx!

@khatv911
Copy link

khatv911 commented Aug 29, 2018

My Approach is:

  • Create a custom Edittext that can manage its Textwatchers list. https://www.dowemo.com/article/57422/how-to-delete-all-listene-added-with-addtextchangedlistener

  • The form item Model should have a textwatcher

  • When bind the text to the edittext :
    +Remove all textwatcher (ideally, at the point, it will be 0 or 1 textwatcher) attached to the edittext.
    +Set the text to the edittext (no textWatcher will be invoked)
    +edittext.addTextWatcher(textwatcher)

  • You may consider to remove the textwacher on OnUnbind event.

  • You may want to add a callback , say OnValueChange(T) in your base FormItem class.

  • aftertextchange will invoke the callback

  • Your ModelController should handle call the callbacks.

  • Use Builder pattern for the object you want to get when hit Summit form

@shakil807g
Copy link

please add an sample for it .

@alouanemed
Copy link

alouanemed commented Oct 18, 2018

Do we have a sample in the Wiki for this as Eli mentioned? Thanks.

@elihart
Copy link
Contributor

elihart commented Oct 18, 2018

No, this hasn't been done yet. I'll update this issue when it is.

If anyone wants to help that would be great

@nealsanche
Copy link

Having just hit this and followed the advice here, I'm left wondering why this is so hard. Using the setInputText advice leads to a few strange edge cases (user can have cursor jump to the end while editing a small typo in the middle of an edit text, for example).

This sort of leads me to wonder if rebuilding the models is actually a good idea for certain state changes? Introducing a delay, via requestDelayedModelBuild just leads to a potential time-bomb where if the user is busy doing something that doesn't fit the expected pattern, something strange could occur.

I'm currently using DataBinding in combination with Epoxy and I guess two-way DataBinding isn't a thing in Epoxy (yet). Much of the advice here seems to apply to something other than DataBinding anyway, so what I've come up with might not be following the advice above completely.

@sandys
Copy link

sandys commented Aug 8, 2019

hi guys,
is there any example now ? im looking for an example that uses both text and a photo field (where a user can attach selfie photos)

@elihart
Copy link
Contributor

elihart commented Aug 13, 2019

@sandys this doesn't apply to photos, it only applies to EditText. Photos should be handled like any other type of data.

@nealsanche Introducing a delay, via requestDelayedModelBuild just leads to a potential time-bomb - this is not accurate. Delaying model building can't hurt if only the EditText content changed. This is because the EditText content is already up to date, and its callback should ensure that your state store is up to date with the latest text. The EpoxyModel will not be up to date with that text, so the only bad thing that can happen is if the view is scrolled off the screen and back on it would bind older text (but the correct text would still be saved in your state store and would be applied when models are rebuilt).

As long as the delay is short enough so it isn't feasible for the model's view to be scrolled off and back on then there won't be issues.

As far as the cursor jumping goes, if you use the sample code above then setInputText only updates the text when it changed, which should only happen if you programmatically change it. Otherwise on normal user input the flow is:

  • User types something
  • text watcher calls back to your screen
  • new text is saved in state store
  • delayed model build is triggered
  • This continues happening while the user types. The model build continues being delayed as long as they type.
  • When the user stops typing for long enough, and the delayed model build finally happens, the differ binds the new text to the model and view, leading to setInputText being called with the text value. This text value should be the same as the existing text, so it should be a no-op and nothing should actually be set on the EditText

If you are seeing the cursor jump, I would debug your code in setInputText and see why it isn't following that expected pattern. Make sure you are not converting the Charsequence to a String, which could cause you to lose information that would make the text comparison fail.

@zirkler
Copy link

zirkler commented Sep 27, 2019

We would really appreciate a dedicated wiki article on the EditText handling.
Or maybe share with us how you are handling it in the Airbnb App.

@sandys
Copy link

sandys commented Sep 28, 2019 via email

@elihart
Copy link
Contributor

elihart commented Sep 28, 2019

I've already posted how to handle edit texts as a previous comment in this issue:
#426 (comment)

There's nothing else to it, it's not complicated.

@zirkler
Copy link

zirkler commented Sep 30, 2019

So what I have is a TextWatcher in my ModelView class, which calls a Callback (which is set via a @CallbackProp annotated setter) whenever the text changes. Works so far, but when the user enters text quickly I'm getting this error from epoxy:

com.airbnb.epoxy.ImmutableModelException: The model was changed between being added to the controller and being bound.
    
    Epoxy attribute fields on a model cannot be changed once the model is added to a controller. Check that these fields are not updated, or that the assigned objects are not mutated, outside of the buildModels method. The only exception is if the change is made inside an Interceptor callback. Consider using an interceptor if you need to change a model after it is added to the controller and before it is set on the adapter. If the model is already set on the adapter then you must call `requestModelBuild` instead to recreate all models.
        at com.airbnb.epoxy.EpoxyModel.validateStateHasNotChangedSinceAdded(EpoxyModel.java:463)

Referring to you:

Here is the pattern we use, it's pretty straightforward. This doesn't include the part of the code that handles the TextWatcher and callback to rebuild models, if you are confused about that part let me know and I can provide another snipped.

I guess exactly the part of handling the TextWatcher is the problem here. So yeah, if could please show us this part would be highly appreciated.

@elihart
Copy link
Contributor

elihart commented Sep 30, 2019

@zirkler your problem is that you are updating the property on the model. As you can see in the error that is not allowed.
You should instead have a callback to the controller that lets you update your state:

class Controller : EpoxyController {
    var inputText: String = ""

    override fun buildModels() {
        editTextRow {
            id("edit text")
            text(inputText)
            onTextChanged {
                inputText = it
               requestDelayedModelBuild(1000)
            }
        }
    }
}

In this simple example the data is stored in a property on the controller. It doesn't matter to epoxy where you store it, it just can't be on the model.

A good app architecture will have a good pattern for storing this state, but that is outside of the realm of Epoxy (See https://github.com/airbnb/MvRx for a good pattern for state)

In general the pattern is straightforward:

  1. Have the view callback to the controller when there is a change
  2. Update your state with the change (ideally use immutable state, so that asynchronous model building can be used)
  3. use requestDelayedModelBuild to debounce frequence state changes like typing

That's all there is to it!

@zirkler
Copy link

zirkler commented Oct 2, 2019

Thanks a lot for your answer, in fact I am using MvRx, great pleasure to work with it. I just found out my issue was caused by the mutability of CharSequence. In my MvRx State I store CharSequence objects and the setInputText accepts a CharSequence.

When logging the CharSequence and its corresponding idendityHashCode in the TextWatcher, I realized the mutation in the model was caused by the CharSequence object.

To fix this I now simply create a new object by calling .toString() in my setState method:
setState { copy(textProp = charSeqFromTextWatcher.toString()) }

Thanks so for your patience, I consider this Issue closed for now.

@elihart
Copy link
Contributor

elihart commented Oct 2, 2019

Thanks for sharing what the problem is, glad it is working now!

@nealsanche
Copy link

Finally had a few hours to look into what has been going wrong with my implementation of forms. So far it's looking like the deferred model building was actually being overridden immediately because invalidate was being called (due to MvRx detecting a change to one of the form fields), which would call recyclerView.buildModels(). So even though the text watchers were requesting a deferred model build with a delay, the requestModelBuild() was coming along right after that with a 0 delay. So it was never actually being delayed.

I'm also investigating your statement that we shouldn't be converting to String and should use CharSequence. As @zirkler found out, above, saving CharSequence in the MvRx state isn't a great idea because it doesn't implement hashCode() according to the documentation. Also, I believe the CharSequence instances are mutable, leading to MvRx debug checks throwing exceptions about the CharSequence changing between setState calls. But that's mostly all MvRx.

So, for now, I've written a bit of code to do the following on Invalidate:

override fun invalidate() {
        modelBuildDelay?.let {
            epoxyController.requestDelayedModelBuild(it)
        } ?: epoxyController.requestModelBuild()
    }

If this is done (and I'm not sure it's the best way to do this, but it's an experiment right now) I notice much more predictable behaviour on the forms, since the model building is actually deferred when desired, instead of firing whenever the MvRx state changes.

@elihart
Copy link
Contributor

elihart commented Oct 8, 2019

Thanks for sharing @nealsanche. MvRx is a bit different - it will always automatically request a model build (like you discovered) so you never need to do it manually in your code.

Do you have your EpoxyController set up to build models and diff them asynchronously? We have found that we don't need to delay our model building with mvrx when the epoxy work happens async, since that seems to delay it enough to not cause cyclical issues with typing.

Otherwise an approach like what you have would be find - although it could maybe be cleaned up into a nicer pattern.

@nealsanche
Copy link

Yes, @elihart, we have been doing async model building with Epoxy. I've been doing a series of experiments.

Symptom: Sometimes keystrokes are missed, or backspaces don't clear a character due to a model build that happens at 'just the right time' so that the user's view is out of sync with the model being built.

Did a series of experiments to see if these issues can be improved:

  1. Put CharSequence into MvRx state class, and Databinding variable, and use setInputText as above. BAD RESULT: MvRx will throw immutable state exceptions because CharSequence isn't immutable.

  2. Make copies of CharSequence before putting them into MvRx state using:

        return this.subSequence(0, this.length)
    }

BAD RESULT: Still skipping characters and backspaces leave the last character in a field.

  1. Add code to allow deferred model building to be turned on during text watcher character handling and validation.

BAD RESULT: Character skipping and backspace issue still present, but will happen much more infrequently such as when the user pauses for just the right interval of time. Have also seen exceptions related to two CharSequences with what looks like the same values (same characters, but perhaps a different cursor location or something) that MvRx throws, and then model building just stops.

  1. Turn off Async model building.

Still working on reproducing the problems with this. I'll get back to you.

@vnwarrior
Copy link

hi guys,
we have just started using mvrx for server rendered ui and forms are a very important part of it.

I just came across this issue and am a bit overwhelmed - especially the last comment with async/deferred model building, etc.

Is there any chance that a standalone example with the best practices (including async model building, etc etc) for those of us getting started ?

sorry if its a bother for you guys.

@zirkler
Copy link

zirkler commented Oct 8, 2019

@vnwarrior I felt the very same, but in the end, it's pretty simple. I will put together an example repository which demonstrates how to build forms with Epoxy and MvRx.

@nealsanche
Copy link

I'm going to spend some time making a minimal example of what we've been doing, see if I can get a reproduction case worked out so we can figure out what the issues are. Clearly having the models rebuilt every time the user inputs text is the root cause of the issues I'm seeing. Specifically with async model building. There's always a chance the async model will be built with out-of-date data.

@nealsanche
Copy link

So, I also broke out a minimal example of what we're currently doing inside our code. It's gotten pretty complicated, as you can see. As usual, I don't seem to be seeing the bad behaviours that I was within our app, so there might be something else contributing to that. But, in case someone else is interested in how someone else has added validation, data binding, and form handling into an app using Epoxy and MvRx. This might be an example of that.

https://github.com/nealsanche/EpoxyFormExample

@sandys
Copy link

sandys commented Oct 9, 2019 via email

@elihart
Copy link
Contributor

elihart commented Oct 9, 2019

@nealsanche thanks for sharing your sample!

Databinding does work a bit different - I don't know that you need the whole binding setup to check isTextDifferent - the databinding framework does that automatically. In fact, I took that approach from databinding.

Also, I wouldn't recommend the FormHelper pattern you are using in onBind/onUnbind. Yes, it prevents allocation of some lambdas when rebuilding models, but in my opinion that is an over optimization that won't have much impact. It also adds extra boiler plate and complexity, and opens up room for bugs because it would be easy to forget the unbind call.

By default with databinding, TextWatcher props will get the DoNotHash behavior, so the lambda for them will be created, but it will not be re-set on the view.

The simplest solution to all of this is that when a new text value is set on the view you can check whether the view is attached to the window. If so, it is a rebind of new data and you could ignore it assuming this is text that was already inputted into the view. The first initial bind only happens when the view is detached from the window. This would probably solve your problems and is easy - the big downside is if you ever wanted to programmatically change the content of the edit text while it is on screen you would not be able too (although you could have a function like InputRow.setText(text: String, shouldForce: Boolean = false) (use a data class to set multiple values in an epoxy prop function). Then you could force the text to update even when it is attached to the window, although that starts to get complicated.

Lastly, small reminder that this is unnecessary

companion object : MvRxViewModelFactory<FormViewModel, FormState> {
        override fun create(viewModelContext: ViewModelContext, state: FormState): FormViewModel? =
            FormViewModel(
                initialState = state
            )

    }

You don't need a factory if you are just using the default state, so this whole object can be removed

@nealsanche
Copy link

@elihart Thanks for the info. I have to admit immediately that I don't understand most of it. Just now I spent some time trying to figure out where the TextWatcher is having it's 'DoNotHash' behaviour set. I still see textWatcher contributing to a generated hashCode method:

 @Override
  public int hashCode() {
    int result = super.hashCode();
    result = 31 * result + (onModelBoundListener_epoxyGeneratedModel != null ? 1 : 0);
    result = 31 * result + (onModelUnboundListener_epoxyGeneratedModel != null ? 1 : 0);
    result = 31 * result + (onModelVisibilityStateChangedListener_epoxyGeneratedModel != null ? 1 : 0);
    result = 31 * result + (onModelVisibilityChangedListener_epoxyGeneratedModel != null ? 1 : 0);
    result = 31 * result + (value != null ? value.hashCode() : 0);
    result = 31 * result + (hint != null ? hint.hashCode() : 0);
    result = 31 * result + (selectAllOnFocus ? 1 : 0);
    result = 31 * result + (textWatcher != null ? textWatcher.hashCode() : 0);
    return result;
  }

So, something I'm doing might be wrong here. Data binding layout looks like this:

<?xml version="1.0" encoding="utf-8"?>
<layout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto">

    <data>

        <variable
            name="value"
            type="String" />

        <variable
            name="hint"
            type="String" />

        <variable
            name="selectAllOnFocus"
            type="boolean" />

        <variable
            name="textWatcher"
            type="android.text.TextWatcher" />
    </data>

    <com.google.android.material.textfield.TextInputLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:paddingTop="4dp"
        android:paddingStart="16dp"
        android:paddingEnd="16dp"
        android:paddingBottom="4dp"
        android:hint="@{hint}"
        app:errorEnabled="true"
        app:helperTextEnabled="true"
        app:helperText=" "

        >

        <com.google.android.material.textfield.TextInputEditText
            android:id="@+id/emailAddressField"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:inputType="textEmailAddress"
            android:selectAllOnFocus="@{selectAllOnFocus}"
            android:text="@{value}"
            app:textWatcher="@{textWatcher}"
            app:validateType='@{"email"}'
            app:validateEmpty="@{true}" />

    </com.google.android.material.textfield.TextInputLayout>
</layout>

I'll see what relaxing the isTextDifferent checks do.

The reason for this FormHelper pattern with onBind and unBind was to keep track of a Validator object but I guess I'll try to find another way to make that happen.

I'd really appreciate a quick example of your statement:

The simplest solution to all of this is that when a new text value is set on the view you can check whether the view is attached to the window. If so, it is a rebind of new data and you could ignore it assuming this is text that was already inputted into the view. The first initial bind only happens when the view is detached from the window.

Is this a bit of code wrapped around the setting of the value in the model? Is the View actually available in the model building DSL somewhere other than in onBind? I think maybe the fact that we're using DataBinding is really not helping us out here.

Thanks in advance.

@elihart
Copy link
Contributor

elihart commented Oct 23, 2019

Yeah, I'm not sure if that is possible with data binding. We do it like this in a custom view

@ModelProp
fun someProp(value) {
 if (isAttachedToWindow() { 
    // View is already in the recyclerview and this is a partial bind for an update
  } else {
    // This is the initial full bind before the view is added to the recyclerview
  }
}
```

@ubuntudroid
Copy link

ubuntudroid commented Apr 13, 2021

FWIW I'm doing the following in my controller when using databinding:

genericMultiLineTextInput {
    id("note")
    text(note) // only used on full binds, never on update (not used at all in the layout!)
    onTextChangedListener { s, _, _, _ -> // data binding variable defined in layout and bound as text watcher
        // store text in ViewModel so that it outlives configuration changes and (by means of additional code) restores
        // make sure that this is a really quick call or move to another thread/coroutine
        viewModel.updateNote()
    }
    onBind { model, view, _ ->
        if (!view.dataBinding.root.isAttachedToWindow) {
            // restore state on full binds (e.g. after restore) and when the item is scrolled back into the viewport
            (view.dataBinding as ItemLayoutGenericMultiLineTextInputBinding).textInput.setText(model.text())
        }
    }
}

Seems to work pretty well even when entering test super fast, copy & pasting, deleting, orientation changes, restores etc.

@ranjeetscience
Copy link

hey Guys, I am also facing the similar issue. I have been using the data binding approach for Epoxy and somewhat stuck with view update with textWatcher. Do we have any similar example that uses data binding approach (like that we have for mvRx)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests