EudyContreras / Skeleton-Bones

Library for dynamically generating skeleton loader drawables for Layouts and Views
MIT License
151 stars 15 forks source link

Can't get the examples working #20

Open mortenholmgaard opened 3 years ago

mortenholmgaard commented 3 years ago

First thanks for what seems like a great library with great technical solution to the problem and good documentation as well.

But I can't get it to work, so I have a few questions/bug findings:

  1. We are not using databinding, but I there seems also to be a great api for that case. But trying to use the examples only gives me a black box, no animation at all. I have tried to set almost every property without any animation as a result. What is the problem do you think?

    SkeletonDrawable.create(this).apply {
        this.getProps().apply {
            this.enabled = true
            this.allowSavedState = true
            this.shimmerRayProperties.apply {
                this.shimmerRayThickness = 10.dp
                this.shimmerRayTilt = 0.3f
            }
        }
    }
    
    SkeletonDrawable.create(this)
        .build()
        .setEnabled(true)
        .setAllowSavedState(true)
        .withShimmerBuilder {
            setThickness(10.dp)
            setTilt(0.3f)
            setAnimationDuration(300L)
        }
        .setCornerRadii(3f)
        .setColor(MutableColor(appCtx.getColor(R.color.green)))
        .withBoneBuilder(generateId()) {
            setColor(MutableColor(appCtx.getColor(R.color.lemon)))
            setMaxThickness(12.dp)
            setMinThickness(10.dp)
            setCornerRadii(CornerRadii(3f))
            setEnabled(true)
            setTransitionDuration(300L)
        }
        .setUseStateTransition(true)
  2. There are quite a few properties from "Example Usages 1" that does not seems to be accessible from the other approaches. Some of them seems important, is I just me that can't find them?

    app:skeletonGenerateBones
    app:skeletonAnimateRestoredBounds
  3. When creating a SkeletonDrawable for a ViewGroup, how then to select which child views that should be skeletonable?

  4. Is it possible to make a View skeletonable without going through it parent, or is the same case as 3. question?

  5. In the readme setCornerRadius() should be changed to setCornerRadii()

EudyContreras commented 3 years ago

Hey @mortenholmgaard does the demo app work for you?

EudyContreras commented 3 years ago

@mortenholmgaard I added a demo-3 branch which contains a demo with no-data binding

I will address your points: 1: Check out the demo-3 branch for a more concrete example 2: At the moment app:skeletonGenerateBones is a property that is set to true by default but can be omitted when using data-binding. app:skeletonAnimateRestoredBounds is not available yet but I will add it soon. I would recommend applying layout animations to your layout instead. 3: When using data-binding is possible to ignore specific views I will soon allow this when using builders. 4: It is possible to apply skeletons to individual children containers within a layout. I will try to provide more examples on how to do that 5: Thank you very much I will fix that

Thank you for your input which is very valuable I will be addressing this and I will ping you when I do so 🙌

mortenholmgaard commented 3 years ago

Thanks for your feedback :)

  1. That example works! But this is a problem with margins. I the demo-3 example when adding start margin to ItemBImage

    <ImageView
        android:id="@+id/ItemBImage"
        android:layout_width="60dp"
        android:layout_height="60dp"
        android:layout_marginStart="30dp"
        ...

    then the skeleton width will only be 30dp and not the expected 60dp.

  2. Okay

  3. and 4. Sounds good - we will be awaiting that 👍 We have cells where multiple things are loading async and there by different parts of the data is appearing one by one. What would be most ideal is also that it is possible in both xml and Builder to se if a view is skeletonable - I don't know if that is possible currently. Regarding withBoneBuilder it might be as good to take the view directly because with view binding we have access to that anyway so if that is possible and possibly also makes it faster, that could also be a option to add.

I also looked at the stop skeletons issue https://github.com/EudyContreras/Skeleton-Bones/issues/19 but didn't find any good solutions for I using the code, except keeping a reference to the Builder. I would love if it was possible just to have an extension metod on views that is could call to enable/disable skeletons for a view og viewGroup.

Also a different input could be the I don't know if it would improve performance(not that I have seen any problems yet) to have a general reusable builder, so that you could easy reused the same skeletons thought out you app - which will be what you want most of the time. So the skeleton could be added by a property in the view xml or code, perphaps you have a few different once like you do with style for a type of elements.

EudyContreras commented 3 years ago

@mortenholmgaard Thanks for the feedback and observations and feedback. I will address adress point one as soon as I can. I will also be making the suggested and improvements to the library and improving the documentation 🙌

@mortenholmgaard I will address your questions below:

it is possible in both xml and Builder to se if a view is skeletonable

It is possible by calling the extension getParentSkeletonDrawable on any view. If it returns null that means it is not skeletonable. I will create a new extension function which will easily allow to check this. The following will be available on the next release 🙌

fun ViewGroup.getParentSkeletonDrawable(): SkeletonDrawable?
fun ViewGroup.hasSkeletonLoaderAncestor(): Boolean
fun ViewGroup.isSkeletonLoader(): Boolean

fun View.getParentSkeletonDrawable(): SkeletonDrawable?
fun View.hasSkeletonLoaderAncestor(): Boolean
fun View.isBoneLoader(): Boolean

Regarding this:

I also looked at the stop skeletons issue #19 but didn't find any good solutions for I using the code, except keeping a reference to the Builder. I would love if it was possible just to have an extension metod on views that is could call to enable/disable skeletons for a view og viewGroup.

That is a great idea. I will add this and it will be available upon the next release:

Regarding this.

Also a different input could be the I don't know if it would improve performance(not that I have seen any problems yet) to have a general reusable builder, so that you could easy reused the same skeletons thought out you app - which will be what you want most of the time. So the skeleton could be added by a property in the view xml or code, perphaps you have a few different once like you do with style for a type of elements.

it will be possible in the next release

mortenholmgaard commented 3 years ago

Great work!! It is pretty close to be working perfectly for me.

shimmerRayProperties is not really possible to use as ShimmerRayProperties only has an internal constructor. I see there is a similar pattern with the other builders. What is the reason and purpose of that way of handling it?

val properties = SkeletonProperties().apply {
    allowSavedState = false
    useStateTransition = true
    stateTransitionDuration = 200L
    shimmerRayProperties = ShimmerRayProperties()
}

return SkeletonDrawable.builder(properties)

class ShimmerRayProperties internal constructor()

In relation to getParentSkeletonDrawable() I can see that it is internal, so I can't use that one currently. internal fun View.getParentSkeletonDrawable(): SkeletonDrawable? { ... } It could also be great to have a variant of it like this, to avoid recreating the SkeletonDrawable when ever a row is recycled, if it has been created before: fun ViewGroup.getSkeletonDrawable(): SkeletonDrawable? { ... } This could be related to the reuse in recyclerView problems I currectly have but can't really figure it out without eliminating this cause.

EudyContreras commented 3 years ago

Great work!! It is pretty close to be working perfectly for me.

Awesome to hear 🙌

shimmerRayProperties is not really possible to use as ShimmerRayProperties only has an internal constructor. I see there is a similar pattern with the other builders. What is the reason and purpose of that way of handling it?

I will be fixing this on the next release 🙏 . It does not make sense for those to be internal...

In relation to getParentSkeletonDrawable() I can see that it is internal, so I can't use that one currently. internal fun View.getParentSkeletonDrawable(): SkeletonDrawable? { ... }

It is public in the last release.

fun ViewGroup.getSkeletonDrawable(): SkeletonDrawable? { ... } This could be related to the reuse in recyclerView problems I currectly have but can't really figure it out without eliminating this cause.

Those will be added on the next release 🙌

In addition to the points above I am thinking of adding a property to the drawables which will make it easier to reuse them. Something like

fun resetForReuse()
EudyContreras commented 3 years ago

Issues and improvements addressed in last release

mortenholmgaard commented 3 years ago

Thanks for the improvements - it makes it possible for me to build a more reuseable aproch to it.

I still have one major problem though with is that it is not possible to disable skeletons on a view almost directly after enabling it. It seems that create or toggle skeleton drawable is dispatched async, which most likely is the reason. Will you look into if it is possible to make sure that when stopping it shortly after enabling it, it actually will stop again with calling view.disableSkeletonLoading() ?

I made a gist of how I am trying to make it simple reuseable across the app: https://gist.github.com/mortenholmgaard/577657052bec5408b8477e4b6ff3b526

EudyContreras commented 3 years ago

@mortenholmgaard You are welcomed, Thanks for helping me make the lib better by raising this issues and suggesting improvements.

I will look further into this functionality. I will get back to you once I have fixed this 🙌

EudyContreras commented 3 years ago

@mortenholmgaard From your example it looks like you want to disable a child (bone) of the skeleton immediately after the skeleton is created. Can you explain this use case ?

Here are some things that can help:

To disable a skeleton drawable manually

skeletonDrawable.getProps().enabled = false

In order to disable a skeleton after it has been created we have to do it on the next frame. This can be done using the main dispatcher or by posting what wish to do to the message queue:

val someView = parent.someView

// Using the main scope dispatcher
MainScope().launch {
   someView.disableSkeletonLoading()
}
// Using the message queue:
somView.post {
   someView.disableSkeletonLoading()
}

If we just want to avoid creating a bone loader for a specific view within a skeleton we can simply ignore it.

val someView = parent.someView
SomeSkeletonBuilder().withIgnoredBones(someView)

Keep in mind that when a skeleton is disabled it is then removed from the view and the view's old drawable is restored.

On the next release I will add some ways to easily re-enable skeletons and disable them using extensions

mortenholmgaard commented 3 years ago

The case is that I am creating the SkeletonDrawable for the ViewHolder root and then it am reusing it when ever reusing the row. That means that I am settings the SkeletonDrawable up for the subviews that should use skeletons in some cases. But the case mentioned is just the case where it then should not be shown for the specific subview.

It think the reason for my trouble is that I am mostly handling skeletons per Viewbut they are setup for the parent ViewGroup (but that also has its benefits). I might need a way to disable the it for the specific bone on configuration, like in my sample gist, for the BoneBuilder (It might be there that I have not looked up currently). Meaning that the bone is configured but not starting unless I just after calls enableSkeletonLoading() (might need dispatching as well).

I will also test tomorrow with you dispatch solution.

And great with some more good extensions. A recommendation from me would be to make the extensions more highlighed in the documentation, because as I see your library it is almost to good, meaning that it can do mostly everything with skeletons - which is great - but then require easy documentation access to the simple examples before people are ready for all the more complex goods that this library contains.

Also as soon as I have got it up and running, I will write a recommendations to a few of the Android newsletters I follow, as to me this is by far the best skeleton library out the for Android! 👍

EudyContreras commented 3 years ago

I hope that the information was helpful and that the new changes suits your needs, Let me know if they do not. I will continue improving the library and making it easier to use. I will also be improving the documentation as soon as I get a chance. Thank you for the kind words and I am happy to hear that you find this library good and useful 🎉

mortenholmgaard commented 3 years ago

@EudyContreras I have just tried post to disable and it works perfectly. (Though it migth mean that I have to do that every where I disable to make sure it will work in all cases, which I am not too fond of)

somView.post {
   someView.disableSkeletonLoading()
}

But after that I could detect the last/next problem which is not fixed with the same method, which is renable skeletons loading when the ViewHolder is reused. When I call like this in onViewRecycled it never shows skeletons:

override fun onViewRecycled() {
    binding.imageView.post {
        binding.imageView.enableSkeletonLoading()
        binding.nameTextView.enableSkeletonLoading()
    }
}

Also if I try the same when binding:

fun bind(viewModel: XViewModel) {
    Skeleton.createIfNeeded(binding.root)
        .forImageView(binding.imageView, binding.root)
        .forTextView(binding.nameTextView, binding.root)

    binding.imageView.post { // not shown even with this added in the reuses case!
        binding.imageView.enableSkeletonLoading()
        binding.nameTextView.enableSkeletonLoading()
    }

    ...
}

With the extension shared in the gist before:

fun createIfNeeded(root: ViewGroup): SkeletonBuilder {
    val skeleton = root.getSkeletonDrawable()
    return if (skeleton != null) { // Hitting this case in resuse as the previous created skeleton vas found
        skeleton.builder().setEnabled(true)
        skeleton.builder()
    } else {
        SkeletonDrawable.create(root, true, builder(root)).builder()
    }
}

So no matter which approch I can't get the skeleton to reappear - what am I doing wrong?

EudyContreras commented 3 years ago

@EudyContreras I have just tried post to disable and it works perfectly. (Though it migth mean that I have to do that every where I disable to make sure it will work in all cases, which I am not too fond of)

@mortenholmgaard Doesn't ignoring those views work for better for you in this case? Im not sure I completely understand the use-case ?

But after that I could detect the last/next problem which is not fixed with the same method, which is re-enable skeletons loading when the ViewHolder is reused. When I call like this in onViewRecycled it never shows skeletons:

Enabling do not require posting to the message queue if it is not done on the same frame 🙌 . Also it looks like you are trying to enable the skeletons loading for individual bones within a skeleton. That will unfortunately not work since only the root knows about the skeleton. What you wanna do do is enable the skeleton. I wanna help you make it work but Im not sure I completely follow the case. Doesn't the demo examples such as example 4 fit your use-case ?

You can also branch out from demo-4 and try this out. Just replace from line 133-151 inside the ItemAdapter class


if (position == 1) {
    MainScope().launch {
          drawable.enable(itemView.ItemBContainer)

          delay(2000)
          itemView.ItemBContainer.disableSkeletonLoading()

          delay(2000)
          itemView.ItemBContainer.enableSkeletonLoading()

          delay(2000)
          itemView.ItemBContainer.disableSkeletonLoading()

          // This will not work
          delay(2000)
          itemView.ItemBOuterText.enableSkeletonLoading()
     }
}

If you explain to me exactly what you are trying to achieve I could try making an example that covers this exact case and if it cover it Ill adjust the library so that it does 🙌

mortenholmgaard commented 3 years ago

Okay first of all conceptually for me a skeleton is bound to a View. That meaning that i idealy just see it as simple as enable og disabling a skeleton for a View. And ideally it is also possible to handle a skeleton for a viewGroup like it was a View (single skeleton filling the ViewGroup) or handling the ViewGroup as an easy entry for enabling/disabling all skeleton enabled views in it. So concepturally for me it is all about the individual View. Then I know that teknically it might have to be another story, but this is my conceptually expectation of skeletons ideally.

I am not using Coroutines and ViewModels yet so it is not that simple for me to just reuse the sample directly. But how come this will not work if it is possible:

itemView.ItemBOuterText.enableSkeletonLoading()

That was exactly what I was trying to do. I have now tried with enabling it for the ViewGroup with the SkeletonDrawable in onViewRecycled but no change - the skeleton does not reappear. I have also tried with the ignored bones as suggested (then with disableSkeletonLoading removed) but then the bones are never removed. Can you see where the problem are?

fun bind(viewModel: xxViewModel) {
    Skeleton.createIfNeeded(binding.root) // see previous gist. I have also attempted with just calling SkeletonDrawable.create() every time - same problem.
        .forImageView(binding.imageView, binding.root)

    ImageService().getImage(viewModel.imageUrl) { image ->
           binding.imageView.setImageBitmap(image)
        // binding.root.getSkeletonDrawable()?.getProps()?.addIgnored(binding.imageView)
        binding.imageView.disableSkeletonLoading()
    }
}

override fun onViewRecycled() {     
    binding.root.enableSkeletonLoading()
    // binding.root.getSkeletonDrawable()?.getProps()?.clearIgnored()
}
EudyContreras commented 3 years ago

Okay first of all conceptually for me a skeleton is bound to a View. That meaning that i ideally just see it as simple as enable og disabling a skeleton for a View. And ideally it is also possible to handle a skeleton for a viewGroup like it was a View (single skeleton filling the ViewGroup) or handling the ViewGroup as an easy entry for enabling/disabling all skeleton enabled views in it. So conceptually for me it is all about the individual View.

Ok so basically when I designed the library I made it view-binding first, meaning that it would be easiest to use out of the box with view binding and controlled by a single boolean value which through data-binding would determine when the skeletons would be disposed/disappear. The other principle was the single use and dispose principle. Usually in the common scenario I designed for was like this:

This pattern was used in order to respect the fact that the layout may already be intended to have some other foreground drawable that should not be completely replaced by the skeleton drawables. I will improve views to have foreground skeleton drawables that can be enable and disabled easily.

Skeleton drawable are built using a view, this makes it less straight forward to use the drawable for other views, this will change in the future. I aim to provide the most flexibility so i will make improvements when i have time 🙏 .

I am not using Coroutines and ViewModels yet so it is not that simple for me to just reuse the sample directly.

I ll make more demos without coroutines and ViewModels

But how come this will not work if it is possible: itemView.ItemBOuterText.enableSkeletonLoading()

When creating a skeleton drawable for a ViewGroup we create only one drawable with virtual bones that overlay the children of the ViewGroup, no drawables are created for the children. This means that if the skeleton drawable is disabled/disposed for a ViewGroup it will not be possible to interact with it anymore, I will try to find a way around this

I will look a little closer into this issues and Ill get back to you 🙌

mortenholmgaard commented 3 years ago

@EudyContreras Sounds good - looking forward to hear from you with a fix for the current problem :)

mortenholmgaard commented 3 years ago

@EudyContreras I have just made an example of the problem in a pull request including a video. It should make it simplere to figure out the problem - I hope you can find time to take a look at it :)

EudyContreras commented 3 years ago

@EudyContreras I have just made an example of the problem in a pull request including a video. It should make it simplere to figure out the problem - I hope you can find time to take a look at it :)

Hey @mortenholmgaard thank you for the example, have not been able to work on the lib lately but as soon as I get a chance I will look at your example and start making the improvements 😄

mortenholmgaard commented 3 years ago

@EudyContreras I really hope you soon can find a bit of time to look at this, as we supposed to go live with our projects in a few weeks, and are quite a bit dependant on it.

EudyContreras commented 3 years ago

Hey @mortenholmgaard sorry I have not addressed this yet, I have been busy with my day job and family and I only have the ability to work on this on the late evenings. I believe I will have a chance work on this library and address the issues before the end of the week 🙏 . Ill ping you as soon as I have an update. Sorry for any inconvenience

mortenholmgaard commented 3 years ago

One addition question @EudyContreras: How to avoid all views in the container adding the skeletons to, shows skeletons? bones:skeletonBoneEnabled="

image I have a view like this where only the right side texts shold be skeletons, but when settings skeletons up like this for the right hand side views, the other views still shows some kind of default skeletons/bones:

Skeleton.createIfNeeded(binding.profileInfoSection)
    .forTextView(binding.firstNameTextView, binding.profileInfoSection)
    .forTextView(binding.lastNameTextView, binding.profileInfoSection)
    .forTextView(binding.phoneNumberTextView, binding.profileInfoSection)
    .forTextView(binding.emailTextView, binding.profileInfoSection)
    .forTextView(binding.passwordTextView, binding.profileInfoSection)

I have tried with bones:skeletonBoneEnabled="false" but that did not seem to work. What is the correct way to handle this case as I have this case all over the app?

And is it possible to add skeleton to a viewgroup on the viewgroup it self, and not each of the subviews in it?

(I know it can be hard to find time to side project with small kids - I my self have 3 kids aged 5 and lower :) )

EudyContreras commented 3 years ago

One addition question @EudyContreras: How to avoid all views in the container adding the skeletons to, shows skeletons?

You have to ignore the views you do not want to show skeletons loading on. I believe one of the examples show this. The property/function for this is called:

 fun withIgnoredBones(vararg ids: Int)
fun withIgnoredBones(vararg views: View)

And is it possible to add skeleton to a viewgroup on the viewgroup it self, and not each of the subviews in it?

yes the viewgroups within a skeleton will also have skeleton loading unless they are ignored.

(I know it can be hard to find time to side project with small kids - I my self have 3 kids aged 5 and lower :) )

Ahh yes tell me about it. It is fun to work on these things but finding time and energy is difficult. Thank you for understanding. I started to look at your example by the way. I will be examining it. I will get back to you with some questions very soon.

mortenholmgaard commented 3 years ago

And is it possible to add skeleton to a viewgroup on the viewgroup it self, and not each of the subviews in it?

yes the viewgroups within a skeleton will also have skeleton loading unless they are ignored.

So when the viewgroup have children is it then just to ignore bones on them, and then add a bone on the viewgroup it self? if I recall correctly when I tried to add a bone the viewgroup it just still showed the bones on the children, but I havent ignored bones on them ofcause. Is it possible to ignore bones on them from xml?

EudyContreras commented 3 years ago

So when the viewgroup have children is it then just to ignore bones on them, and then add a bone on the viewgroup it self? if I recall correctly when I tried to add a bone the viewgroup it just still showed the bones on the children, but I havent ignored bones on them ofcause. Is it possible to ignore bones on them from xml?

Well if you have skeleton you can ignore its child views by specifying their ids. Then no loader will be created for those views. Have you tried this? Let me know if it works as expected.

mortenholmgaard commented 3 years ago

@EudyContreras I have now tested trying to add a skeleton to the viewgroup it self, with the children visibility set to gone and having a explicit width and height on the view - but nothing is shown.

I am setting it up with a bone builder like this:

private val viewGroupBoneBuilder: (view: View) -> BoneBuilder.() -> Unit = { view, shape ->
        {
            textBoneBuilder(view, false, null)(this)
            setColor(MutableColor.fromColor(ContextCompat.getColor(view.context, R.color.bone_color_alt)))
            setShaderMultiplier(1.021f)
            setShapeType(ShapeType.RECTANGULAR)
            withShimmerBuilder(shimmerRayBuilder(view, R.color.bone_ray_color_alt))
        }
    }

Edit: I have now tried multiple viewgroups without getting skeletons on any of them. And ignoring bones on child views does only show those child view. What I am looking for with this is when I have a ViewGroup with a few small views, eg. TextView and ImageView, I want one skeleton showing that fill the ViewGroup and hides all child views. Because some time it will look bad if two small views besides each other possibly with differnet heigts and margins has skeletons.

Can you explain why it is needed to ignore bones, because since bones automatically is added there must be some kind of default bones? And if so, I would like just to adjust that default bone style and then only need to ignore bones not now wanted views and not apply bones.

EudyContreras commented 3 years ago

@mortenholmgaard I will try to find sometime to address, I have unfortunately been very busy and unable to put time into the library. I have a feeling that I will soon get the chance again to work on it more consistently. I am sorry for any inconvenience this may cause. I will also keep you updated with any work or update that have been made