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

The best way to implement forms with MvRx & Epoxy #115

Open
qbait opened this issue Oct 4, 2018 · 16 comments
Open

The best way to implement forms with MvRx & Epoxy #115

qbait opened this issue Oct 4, 2018 · 16 comments

Comments

@qbait
Copy link

qbait commented Oct 4, 2018

Could you tell me how I could improve my pattern for creating forms with MvRx?
What's the best way to fix EditText recycling ? Keeping all EditTexts in single view like in your ToDo sample solves the problem, but it's less scalable solution, isn't it?

<?xml version="1.0" encoding="utf-8"?>
<merge xmlns:android="http://schemas.android.com/apk/res/android">
    <android.support.design.widget.TextInputLayout
        android:id="@+id/inputLayout"
        android:layout_width="match_parent"
        android:layout_height="wrap_content">
        <android.support.design.widget.TextInputEditText
            android:id="@+id/editText"
            android:layout_width="match_parent"
            android:layout_height="wrap_content" />
    </android.support.design.widget.TextInputLayout>
</merge>

My Model is below.
setTextAndCursor is Extension fuction inspired by @elihart code airbnb/epoxy#426)
onChange is Extension function wrapping TextWatcher

@ModelView(autoLayout = ModelView.Size.MATCH_WIDTH_WRAP_HEIGHT)
class Input @JvmOverloads constructor(
        context: Context, attrs: AttributeSet? = null, defStyleAttr: Int = 0
) : FrameLayout(context, attrs, defStyleAttr) {

    init {
        inflate(context, R.layout.input, this)
    }

    @TextProp
    fun setText(email: CharSequence) {
        editText.setTextAndCursor(email)
    }

    @CallbackProp
    fun setOnChange(onChange: ((String) -> Unit)?) {
        editText.onChange { onChange?.invoke(it) }
    }

    @ModelProp
    fun setError(error: String?) {
        inputLayout.error = error
    }
}

Code in Fragment

    override fun epoxyController() = simpleController(viewModel) { state ->
       
        with(state.registerForm) {

            input {
                id("email")
                text(email)
                error(emailError)
                onChange { viewModel.onEmailChange(it) }
            }

            input {
                id("firstName")
                text(firstName)
                error(firstNameError)
                onChange { viewModel.onFirstNameChange(it) }
            }

Code in ViewModel

    fun onEmailChange(email: String) {
        setState { copy(registerForm = registerForm.copy(email = email, emailError = emailError(email))) }
    }

    fun onFirstNameChange(firstName: String) {
        setState { copy(registerForm = registerForm.copy(firstName = firstName, firstNameError = firstNameError(firstName))) }
    }
  1. Could you tell me if my pattern is right?
  2. What's the best way to solve the problem with recycling EditText
@elihart
Copy link
Contributor

elihart commented Oct 5, 2018

This pattern looks right to me, what's the problem with recycling exactly? what is it not recycling?

@qbait
Copy link
Author

qbait commented Oct 5, 2018

I had a problem with attaching TextWatcher to EditText and recycling.

I solved it by having WatcherAwareEditText

public class WatcherAwareEditText extends TextInputEditText
{   
    private ArrayList<TextWatcher> mListeners = null;
    public WatcherAwareEditText(Context ctx)
    {
        super(ctx);
    }
    public WatcherAwareEditText(Context ctx, AttributeSet attrs)
    {
        super(ctx, attrs);
    }
    public WatcherAwareEditText(Context ctx, AttributeSet attrs, int defStyle)
    {       
        super(ctx, attrs, defStyle);
    }
    @Override
    public void addTextChangedListener(TextWatcher watcher)
    {       
        if (mListeners == null) 
        {
            mListeners = new ArrayList<TextWatcher>();
        }
        mListeners.add(watcher);
        super.addTextChangedListener(watcher);
    }
    @Override
    public void removeTextChangedListener(TextWatcher watcher)
    {       
        if (mListeners != null) 
        {
            int i = mListeners.indexOf(watcher);
            if (i >= 0) 
            {
                mListeners.remove(i);
            }
        }
        super.removeTextChangedListener(watcher);
    }
    public void clearTextChangedListeners()
    {
        if(mListeners != null)
        {
            for(TextWatcher watcher : mListeners)
            {
                super.removeTextChangedListener(watcher);
            }
            mListeners.clear();
            mListeners = null;
        }
    }
}

and clearing all Watchers before setting new one in my Model

    @CallbackProp
    fun setWatcher(watcher: TextWatcher?) {
        editText.clearTextChangedListeners()
        watcher?.let {
            editText.addTextChangedListener(it)
        }
    }

It works. However I'll be greatful for your feedback @elihart .

It's a pleasure to create forms with MvRx and Epoxy after setting up everything, but coming up with a good pattern isn't trivial. I could give my code as a sample. Let me know if I should create PR.

@shakil807g
Copy link

shakil807g commented Oct 6, 2018

you can put some thing like this in your model

       @OnViewRecycled
       public void clear(){
           editText.removeTextChangedListener(null)
        }

instead of your WatcherAwareEditText

@elihart
Copy link
Contributor

elihart commented Oct 6, 2018

@qbait your solution seems fine. alternatively the textwatcher can be stored as a property in the @ModelView class - whatever you prefer

if you prefer to use a more kotlin like approach and avoid needing to subclass edit text you can use extension functions instead

here's a small sample I just wrote up to illustrate (this is just scratch untested code, but should give you the right idea)

fun EditText.addTextChangedListener(watcher: TextWatcher) {
    addTextChangedListener(watcher)
    getWatchers().add(watcher)
}

fun EditText.clearWatchers() {
    getWatchers().clear()
}

private fun EditText.getWatchers(): MutableList<TextWatcher> {
    return getTag(R.id.text_watchers) ?: run {
        val newList = mutableListOf<TextWatcher>()
        setTag(R.id.text_watchers, newList)
        newList
    }
}

@shakil807g calling removeTextChangedListener(null) doesn't do anything, i wouldn't recommend that.

@shakil807g
Copy link

shakil807g commented Oct 7, 2018

Yes sorry my bad , i am using my model this way is there anything that could go wrong with this approach ?

@EpoxyModelClass(layout = R.layout.item_text_field)
abstract class EditTextModel : EpoxyModelWithHolder<EditTextModel.EditTextHolder>() {

@EpoxyAttribute
var text: String? = null

@EpoxyAttribute
var errorText: String? = null

@EpoxyAttribute
var hint : String? = null

@EpoxyAttribute
@DrawableRes
var prefIcon: String? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
var textWachter: TextWatcher? = null



override fun bind(holder: EditTextModel.EditTextHolder) {


    if(holder.editext.extSetText(text)){
        holder.editext.setSelection(holder.editext.length())
    }

     holder.editext.error = errorText

    holder.editext.addTextChangedListener(textWachter)



}

override fun unbind(holder: EditTextHolder) {
    super.unbind(holder)
    holder.editext.removeTextChangedListener(textWachter)
    holder.editext.error = null
}

  inner class EditTextHolder : EpoxyHolder() {
    lateinit var editext : EditText


    override fun bindView(itemView: View) {
        editext = itemView.editText
    }

  }

@qbait
Copy link
Author

qbait commented Oct 8, 2018

@elihart It was working great when I had 1 big scrolling form. Now I separated sections into separate fragments, but I left single shared ViewModel.
Unfortunately with this approach I have problem with TextWatchers again.
What's the best approach here? should I have ViewModel for every Fragment or I can leave single Shared ViewModel and additionally clear watchers somewhere in my View.

@elihart
Copy link
Contributor

elihart commented Oct 8, 2018

why do you have problems with separate fragments, what is the problem exactly? You've gotta give more details.

sharing a viewmodel should work theoretically, but it may not make the cleanest abstraction if the fragments are unrelated. clearing the watchers seems like it would always be a good idea

@qbait
Copy link
Author

qbait commented Oct 8, 2018

Ok, @elihart, the problem was in clearing Watchers, I was doing it like that:

    @CallbackProp
    fun setWatcher(watcher: TextWatcher?) {
        editText.clearWatchers()
        watcher?.let {
            editText.addWatcher(it)
        }
    }

I changed it to watch only when EditText has focus

        editText.setOnFocusChangeListener { _, hasFocus ->
            if(!hasFocus) {
                editText.clearWatchers()
            } else {
                watcher?.let {
                    editText.addWatcher(it)
                }
            }
        }

I'm curious where you clear you watchers.

@filipwiech
Copy link

@qbait It would be great if you could share your code as a sample, either as a PR, gist, or otherwise.
@elihart This topic seems to come up in both Epoxy and MvRx repos, and proper and clean way of handling this doesn't seem very obvious. Maybe an official sample/wiki page would be a good idea.

@jQrgen
Copy link

jQrgen commented Oct 9, 2018

@qbait Can you please share the setTextAndCursor and onChange extension or a full example? Thanks

@elihart
Copy link
Contributor

elihart commented Oct 9, 2018

@qbait thanks for the update. It seems odd to me that you are needing to clear the watcher when focus changes - it seems like something else is going on here that isn't right. You should only need to clear the watcher when the view is unbound/recycled, which happens automatically with a callback prop

@qbait
Copy link
Author

qbait commented Oct 9, 2018

I'm planning to prepare a sample code and post on Medium when I have more time. Here is the extension

fun EditText.setTextAndCursor(text: CharSequence) {
    if(setText(this,text) ) {
        this.setSelection(this.length())
    }
}

private fun setText(textView: TextView, text: CharSequence): Boolean {
    if (!isTextDifferent(text, textView.text)) {
        return false
    }

    textView.text = text

    return true
}

private fun isTextDifferent(str1: CharSequence?, str2: CharSequence?): Boolean {
    if (str1 === str2) {
        return false
    }
    if (str1 == null || str2 == null) {
        return true
    }
    val length = str1.length
    if (length != str2.length) {
        return true
    }

    if (str1 is Spanned) {
        return str1 != str2
    }

    for (i in 0 until length) {
        if (str1[i] != str2[i]) {
            return true
        }
    }
    return false
}

Instead of onChange, I'm passing normal TextWatcher. Need to polish it when I'm free.

@qbait
Copy link
Author

qbait commented Oct 9, 2018

@elihart my clear function was invoked ~2s after new fragment was created and meantime it was making a mess.

       @OnViewRecycled
       public void clear(){
           editText.clearWatchers()
        }

I can prepare a sample project tomorrow if you would have time to take a look.

@laszlo-galosi
Copy link

@elihart and @qbait I use similar solution with the approach of yours, but I ran into an issue, that for my custom EditField view, that has inputType, imeOptions, @ModelProp annotated properties, and using @AfterPropsSet to bind these properties all together to the EditText. The issue was, because setState on the text change event, and the soft keyboard is displaying, the imeAction, and inputType also changed when the bind is called resulting the changing of the soft keyboard
switching keyboard action icons, and keyboard type (according to imeOptions and inputType). It seems if you use functions instead of properties this issue is solved. Or should I examine the equality of this properties, and only set them if they are different, just like the text in @qbait solution?

@sandys
Copy link

sandys commented Aug 8, 2019

hi guys - does anyone have an example now ? especially with a text field and a photo field (where we click a photo through phone camera).

We are not able to figure this out properly

@zsweigart
Copy link

Yea I'm running into the same issue with multiple fragments. It seems the edittexts are being removed and the clear isn't called immediately so if I cycle between fragments quickly I can have text in an edittext in one fragment changing text in an edittext in a a different fragment

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

8 participants