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

Async drawable doesn't show up if it's the only thing in the text view #189

Closed
Kaned1as opened this issue Jan 22, 2020 · 19 comments
Closed
Labels
Milestone

Comments

@Kaned1as
Copy link

  • Markwon version: 4.2.1-SNAPSHOT + GlidePlugin

Async drawable doesn't show up if it's the only thing in the text view.

In the source code of the ReplacementSpan there's this suspicious javadoc

   /**
     * Returns the width of the span. Extending classes can set the height of the span by updating
     * attributes of {@link android.graphics.Paint.FontMetricsInt}. If the span covers the whole
     * text, and the height is not set,
     * {@link #draw(Canvas, CharSequence, int, int, float, int, int, int, Paint)} will not be
     * called for the span.
     */
     public abstract int getSize(...)

And AsyncDrawable.initWithKnownDimensions(...) is only called if AsyncDrawableSpan.draw(...) of the parent span was called.

@noties
Copy link
Owner

noties commented Jan 22, 2020

Hello @Adonai !

Can you please try the same markdown input with 4.2.0 version? And different image loader? I think there is a chance that drawable is not displayed because of missing bounds.

Also, I could not reproduce it. So, let's try to pin point the issue:

  • How your image is represented in the markdown - markdown syntaxt or HTML?
  • Does it have size constraints (in case of HTML)?
  • Do you use custom ImageSizeResolver?
  • What type of image it is - SVG, GIF, static?

As for javadoc... well, AsyncDrawableSpan will set height if it has loaded result. If has none, then height won't be set (and thus draw method not be called). But this is OK, if you have not loaded a result yet

@Kaned1as
Copy link
Author

I'll try to craft minimal reproducible example of this, thanks for quick response!

@Kaned1as
Copy link
Author

Kaned1as commented Jan 22, 2020

Here it is

    val renderer = Markwon.builder(ctx)
        .usePlugin(GlideImagesPlugin.create(ctx))
        .build()

then

    val md = renderer.toMarkdown("![7QOIF2X.jpg](https://i.imgur.com/7QOIF2X.jpg)")
    messageBody.text = md
    AsyncDrawableScheduler.schedule(messageBody)

Note 1: First time it works ok. The second, when images are cached by Glide, it breaks.

Note 2: If paired with any text on the other line, it works again:

    val md = renderer.toMarkdown("No issue!\n\n![7QOIF2X.jpg](https://i.imgur.com/7QOIF2X.jpg)")
    messageBody.text = md
    AsyncDrawableScheduler.schedule(messageBody)

@Kaned1as
Copy link
Author

I think this may be the culprit: b55b1f0

@noties
Copy link
Owner

noties commented Jan 23, 2020

Hello @Adonai !

It might be the culprit but did you test with the previous version? 😄

I did reproduce your report but with a single change it can be fixed:

messageBody.setText(md, TextView.BufferType.SPANNABLE);

But I recommend to use the:

markwon.setMarkdown(messageBody, markdown);

which takes care of not only that, but also other things (like measuring ordered lists width, etc)

@Kaned1as
Copy link
Author

@noties both changes didn't work for me. Still empty message.
Tried on Android Emulator API 27, and real device, API 29.

It might be the culprit but did you test with the previous version?

No, I haven't... not sure it's possible to select specific version with -SNAPSHOT.

@noties
Copy link
Owner

noties commented Jan 26, 2020

Weird, it works for me, I will take a closer look.

No, I haven't... not sure it's possible to select specific version with -SNAPSHOT.

I think it is as simple as changing the library version in your dependencies block:

implementation 'io.noties.markwon:core:4.2.0' // or any other version, really

Checking against latest released version would suffice here, can you try it?

@Kaned1as
Copy link
Author

Kaned1as commented Feb 1, 2020

Hi! I'll be out of town for some time, will surely check once I return. Thanks!

@LaurentTreguier
Copy link

Hello !

I am seeing what looks like the same issue. For me, downgrading to 4.2.0 fixes it (I've used that version since December without experiencing this), and trying 4.2.1 again brings the problem back.
I see it happening in a RecyclerView with a MarkwonAdapter; using createTextViewIsRoot() to get a simple adapter where every entry is only a TextView. (If images are put in their own entries, then it matches this issue)

I'd be happy to help if there is anything I can do/test.

@noties
Copy link
Owner

noties commented Feb 5, 2020

Hello @LaurentTreguier !

On what API level(s) do you see this behavior?

@LaurentTreguier
Copy link

This is on API 29, on the emulator

@noties
Copy link
Owner

noties commented Feb 5, 2020

@LaurentTreguier I still cannot reproduce this issue even on API 29 emulator. Can you please share your Markwon configuration and a markdown snippet as a reference?

@LaurentTreguier
Copy link

I'll put this here to make sure it's not lost in the middle of the code:
I got curious and tried a few things; and the problem goes away as soon as I comment out lines adding a placeholder. Removing the custom PostPlugin (posted below) didn't change anything. It looks like something to do with replacing placeholders with the actual image.

Here is the function I use to setup The Markwon instance:

fun Fragment.lazyMarkdown() = lazy {
    requireActivity().let {
        Markwon.builder(it)
            .usePlugin(CorePlugin.create())
            .usePlugin(MovementMethodPlugin.create())
            .usePlugin(PostPlugin.create())
            .usePlugin(StrikethroughPlugin.create())
            .usePlugin(ImagesPlugin.create())
            .usePlugin(GlideImagesPlugin.create(PostGlideStore(it)))
            .build()
    }
}

Here is the PostPlugin:

class PostPlugin private constructor() : AbstractMarkwonPlugin() {
    override fun configureVisitor(builder: MarkwonVisitor.Builder) {
        builder.on(SoftLineBreak::class.java) { visitor, _ -> visitor.forceNewLine() }
        builder.on(Text::class.java) { visitor, text ->
            visitor.builder().append(
                SpannableString(text.literal).apply {
                    LinkifyCompat.addLinks(this, Linkify.WEB_URLS)
                }
            )
            visitor.visitChildren(text)
        }
    }

    override fun configureConfiguration(builder: MarkwonConfiguration.Builder) {
        builder.imageSizeResolver(object : ImageSizeResolver() {
            override fun resolveImageSize(drawable: AsyncDrawable): Rect {
                val canvasWidth = drawable.lastKnownCanvasWidth
                val factor = canvasWidth.toFloat() / drawable.intrinsicWidth
                return Rect(0, 0, canvasWidth, (drawable.intrinsicHeight * factor).toInt())
            }
        })
    }

    companion object {
        fun create() = PostPlugin()
    }
}

And here is the PostGlideStore implementation:

class PostGlideStore(private val context: Context) : GlideImagesPlugin.GlideStore {
    override fun load(drawable: AsyncDrawable) = AppGlide.with(context)
        .load(drawable.destination)
        .placeholder( // removing this placeholder fixes the issue
            VectorDrawableCompat.create(
                context.resources,
                R.drawable.ic_image,
                context.theme
            )
        )

    override fun cancel(target: Target<*>) = AppGlide.with(context).clear(target)
}

Here is a raw markdown string with which that I have seen the issue, using MarkwonAdapter:

# Gorillaz - El Mañana
[![undefined](https://img.youtube.com/vi/gs1I8_m4AOM/0.jpg)](https://www.youtube.com/watch?v=gs1I8_m4AOM)

[Verse 1]
Summer don't know me no more
He got mad, that's all
Summer don't know me
He'd just let me low in myself
'Cause I do know love
From you then, just dying

[Hook]
I saw that day
Lost my mind
Lord, I'm fine
Maybe in time, you'll want to be mine

[Verse 2]
Don't stop the bud, when it comes
It's the dawn, you'll see
Money won't get there
Ten years passed tonight, you'll flee
If you do that
I'll be sworn
To find you

[Hook]
I saw that day
Lost my mind
Lord, I'm fine
Maybe in time, you'll want to be mine

[Bridge]
I saw that day
Lost my mind
Lord, I'm fine
Maybe in time, you'll want to be mine

[Outro]
(I saw that day)
(Lost my mind)
(Lord, I’m fine)
Maybe in time, you'll want to be mine
Maybe in time, you'll want to be mine
Maybe in time, you'll want to be mine
Maybe in time, you'll want to be mine

@noties
Copy link
Owner

noties commented Feb 8, 2020

Hello @LaurentTreguier !

Thank you for the info, it was most valuable! I did manage to reproduce the issue and the fix is already in the develop branch (4.2.2-SNAPSHOT must be published soon after github runs the tests).

So, the problem here might indeed be the commit that @Adonai mentioned before. In the commit I tried to reduce the number of invalidations that AyncDrawable goes through. For this to properly to function AsyncDrawable must be initialized with no bounds. This way, when result is downloaded AsyncDrawable will be invalidated exactly once. But the placeholder did slip my mind.

So, when you have a PlaceHolderProvider and it returns a Drawable without bounds - AsyncDrawable was not showing both (due to reduced number of invalidations exactly). This is already fixed in develop.

To have a workaround in current 4.2.1 version you can use a placeholder drawable with bounds set. Which makes sense to do anyway, as without bounds placeholder has no real usage (won't be shown).

Currently placeholder bounds has few rules:

  • if it has exact bounds - they will be used (no matter what bounds AsyncDrawable result might be shown later). This makes sense for regular image loading (in markdown syntax) and in HTML. HTML images can have explicit size (for example <img width=100%>), so when image has explicit size this placeholder will keep its bounds (and won't be scaled)
  • if placeholder has no explicit bounds, then its intrinsic bounds will be used (in case of HTML this size will be used in scaling)
  • if placeholder has no bounds and no intrinsic bounds, then this placeholder won't be shown

Hope this resolves this issue and clears a bit how placeholders are displayed

@Kaned1as
Copy link
Author

Hello, I'm back. Will test this in the following days and close the issue. Thanks for your thorough investigation!

@noties noties mentioned this issue Feb 26, 2020
@noties
Copy link
Owner

noties commented Feb 26, 2020

Let me know if issue persists

@noties noties closed this as completed Feb 26, 2020
@LaurentTreguier
Copy link

I don't know if @Adonai has had time to test the 4.2.2 version, but on my end, it looks like the issue is still here.
I tested again with the example I gave up above, and also with the image part alone ([![undefined](https://img.youtube.com/vi/gs1I8_m4AOM/0.jpg)](https://www.youtube.com/watch?v=gs1I8_m4AOM)); and the image still doesn't appear when using a placeholder in the custom GlideStore.

@noties noties reopened this Mar 8, 2020
@noties
Copy link
Owner

noties commented Mar 8, 2020

I can reproduce it, the fix is going to be in the next 4.3.0 version (and upcoming snapshot). The problem is that Glide is setting placeholder as a regular result. AsyncDrawable receives the result, but as there are no canvas dimensions are available - there are no bounds available, thus span returns 0 for the size. Please note that this is the case when there is exactly one image span in the markdown. If there is at least one other character/span, result would be as OK

@noties noties added the bug label Mar 8, 2020
@noties noties added this to the 4.3.0 milestone Mar 8, 2020
@noties noties mentioned this issue Mar 18, 2020
@noties noties closed this as completed Mar 18, 2020
@LaurentTreguier
Copy link

Good news, from my testing today, it looks like this issue is fixed for me; thanks @noties !

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

No branches or pull requests

3 participants