noties / Markwon

Android markdown library (no WebView)
https://noties.io/Markwon/
Apache License 2.0
2.76k stars 313 forks source link

No image size resolving in visitor #33

Closed Kaned1as closed 6 years ago

Kaned1as commented 6 years ago

Hi. Thanks for wonderful Markdown view, was a life-saver to me.

Here: SpannableMarkdownVisitor#visit(org.commonmark.node.Image)

The AsyncDrawable is created without imageSizeResolver and it's rendered veeeery big

I understand that there are no size constraints in native Markdown syntax but at least some level of customization should be possible for images to fit/scale into the textview.

noties commented 6 years ago

Hey @Adonai !

Yeah, I think that the library should respect TextView canvas size to not exceed it (keeping original ratio of cause). Meanwhile, if it's an option, you could use HTML img tags. Markwon takes into account width and height attributes and allows then reference % and em (font size), so as pixels (actually everything else except % and em is considered to be in pixels). For example:

<!-- takes 33% of canvas width keeping the original ratio -->
<img src="my-super-image.png" width="33%">
Kaned1as commented 6 years ago

@noties, yep, thanks, I recognized the difference in htmlParser/native markdown.

The problem is that I can't control what users write in the app so for now will come up with small postprocessing hack.

Thanks for quick reply!

noties commented 6 years ago

Hey @Adonai !

I have created a new branch for the next release and pushed there discussed changes, here. I have tested it and it works for me. How it's for you? 😉

Kaned1as commented 6 years ago

@noties , I'll test it this week and report, thanks!

Kaned1as commented 6 years ago

Ugh, compiling aar without uploading it is such a pain. You should have told me about jitpack earlier, bro :)

https://jitpack.io/com/github/noties/Markwon/v1.0.5-SNAPSHOT/

noties commented 6 years ago

Dafuq is this? 😶 It's not created by me.

While we are on this I can recommend using local maven repo: https://noties.github.io/blog/2018/03/24/local-maven-repo no need to upload anything

Kaned1as commented 6 years ago

This thing is auto-generating build artifacts for any github repo you request it to do. See this, very handy

noties commented 6 years ago

Yeah, I don't like it. Maybe I should create a snapshot myself as development for the next release is mainly in one branch.

Kaned1as commented 6 years ago

Ok, tested this, works as expected. You may close this once it's released. Thanks!

Kaned1as commented 6 years ago

The other problem, though, is that bounds in AsyncDrawable are resolved in setResult() but canvas width detection occurs in draw(). When TextView is just inflated its width and height are zero, thus AsyncDrawable may fail to properly resolve dimensions.

In particular, this happens when you have RecyclerView with TextView items and scroll too fast or reload adapter.

noties commented 6 years ago

I think if you do something like this it should solve the RecyclerView issue:

public class MyRecyclerViewAdapter extends RecyclerView.Adapter<MyHolder> {

    @Override
    public void onBindViewHolder(@NonNull final MyHolder holder, final int position) {
        whenReady(holder.text, new Runnable() {
            @Override
            public void run() {
                Markwon.setMarkdown(holder.text, getMarkdown(position));
            }
        });
    }

    private static void whenReady(@NonNull final View view, @NonNull final Runnable action) {
        if (view.getWidth() == 0) {
            view.getViewTreeObserver().addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() {
                @Override
                public boolean onPreDraw() {
                    if (view.getWidth() > 0) {
                        view.getViewTreeObserver().removeOnPreDrawListener(this);
                        action.run();
                    }
                    return true;
                }
            });
        } else {
            action.run();
        }
    }
}
Kaned1as commented 6 years ago

Thanks, I solved it the other but fairly similar way. Anyway, this issue is resolved.