Since 4.4.0 Markwon instance is no longer immutable

[KirkBushman]

Markwon version: 4.4.0

Hi, I'm back.

I'm using Epoxy to manage lists and specifically collection of comments.

Since I need the markwon instance to set the content with markwon.setParsedMarkdown, I need to pass it to the model.
By rule all params need to be immutable, to be used as a hash.

Since 4.3.1 everything was fine, upgrading to 4.4.0 I'm getting an exception on the Markwon instance, saying it's changing. ImmutableModelException.

Comparing the 2 releases I don't see anything that could be responsible for this (if it's not in the plugins...?).

May I suggest in the future an approach similar to Glide, where it can be configured at startup with a class and an annotation, and can be called anywhere with Glide.with(context), avoiding the need to pass around an instance?

Or having a way not needing a instance to set the content on a TextView?

I can stay on 4.3.1 with no problem for a while, just trying to start a discussion.

[noties]

Hello @KirkBushman ,

It is weird because there are no changes to Markwon's hashCode nor equals (and it is still immutable). But I think it's strange that Markwon instance is being used to create a diff anyway. I'm not familiar with Epoxy but from quick glance it seems that you mark certain properties inside your model as diffable and then Epoxy does its diffing based on that. So, even if you mark Markwon instance as diffable it should not be used in hash calculations as once Markwon instance is created its hashCode won't change. Are you sure you are not recreating Markwon inside your model? Maybe there are additional changes to your project (like epoxy version, build tools, gradle plugin, compile SDK version, etc)?

As for Glide approach - I do not like it. I think it is confusing. And moreover it requires an annotation processor. The actual call Glide.with(Context) might be convenient, but initialization - the annotations and modules description - is very much not. Anyway, there are multiple ways to achieve that - a lifecycle aware storage. So a Context can have some instance associated with it. For example:

class MarkwonCache {
  companion object {
    private val cache = Collections.synchronizedMap(WeakHashMap<Context, Markwon>())

    fun with(context: Context): Markwon {
      return cache[context] ?: {
        Markwon.builder(context)
          .usePlugin(StrikethroughPlugin.create())
          .build()
          .also {
            cache[context] = it
          }
      }.invoke()
    }
  }
}

You could also use LifecycleOwner to clear cached instance (and maybe check initial state of context), but I find its value questionable, so I leave it out

[KirkBushman]

I know,
it does not make any sense, but when changing the version I have that error everywhere.

Looking at the code, the error will trigger when the hash is different between rebuilds.
Something is happening...

As for the Glide approach, I don't like the annotation processor and the weight it generates in general,
but as a developer, implementing code in a testable way, it makes my life easier.

I don't know if I like having a lot of instances of Markwon around, since binding to the view context will create an instance per comment.

Having a static singleton with application context will not work with multiple modules, since you cannot reach it in the sub modules. (without disgusting code).

Can you think of any other approach, that will work in most situations?

[noties]

Would you mind @KirkBushman sharing Epoxy-related code with Markwon that is causing the exception? I think we have a better chance of fixing this issue by looking at the root cause of it.

Meanwhile,

I don't know if I like having a lot of instances of Markwon around, since binding to the view context will create an instance per comment.

The thing is - there will be exactly one instance of Markwon per activity. Individual widgets do not create own Context

Having a static singleton with application context will not work with multiple modules, since you cannot reach it in the sub modules. (without disgusting code).

If you define your singleton or a static cache in your base module (other modules depend on it) you should not have any disgusting code at all. You also can use dependency injection to provide a Markwon instance (or cache for that matter)

[KirkBushman]

The thing is - there will be exactly one instance of Markwon per activity. Individual widgets do not create own Context

Forgot that, you're right.

If you define your singleton or a static cache in your base module (other modules depend on it) you should not have any disgusting code at all. You also can use dependency injection to provide a Markwon instance (or cache for that matter)

You would want initialize in your application class, using app context, to map it in the core module, one should have to use an interface to implement. That would be wank code to me.
At this point I don't see any other way to easily fix this, so I'll go this route.

Codewise, there is not a lot to look at:

@EpoxyModelClass
abstract class CommentModel : EpoxyModelWithHolder<CommentHolder>() {

    @EpoxyAttribute
    lateinit var comment: Comment

    @EpoxyAttribute(DoNotHash)
    lateinit var markwon: Markwon

    override fun getDefaultLayout(): Int {
        return R.layout.item_comment
    }

    override fun bind(holder: CommentHolder) {
        ...

        holder.body.movementMethod = LinkMovementMethod.getInstance()
        markwon.setParsedMarkdown(holder.body, comment.body)
        ...
    }
}

[noties]

Hello @KirkBushman ,

I'm going to close this as I couldn't find Markwon related code. It seems to be some Kotlin/Epoxy specific issue

[KirkBushman]

Ok,
I'll probably use the solution provided by you. Thanks again.