Address various concerns #808
Conversation
* fix experimental annotation for new DSL API entrance points * add inline to various methods
…tArray, and I think we don't need to go via a list to an array in this case if we don't really require an array) * breaking change...
| /** | ||
| * internal helper function to execute the block if the item is expandable | ||
| */ | ||
| internal fun <R> IItem<out RecyclerView.ViewHolder>?.ifExpandable(block: (IExpandable<*>) -> R): R? { |
There was a problem hiding this comment.
Is it possible to make this a inline function?
There was a problem hiding this comment.
I had it as an inline function. but AS will inform that the performance impact is rather low and it is suggested to keep it a normal function
There was a problem hiding this comment.
Weird. I thought all inlined higher order functions benefit from inlining (performance wise). I'm not sure if it's because of let, which probably inlines stuff on its own
| /** | ||
| * internal helper function to execute the block if the item is expandable | ||
| */ | ||
| internal fun <R> IItem<out RecyclerView.ViewHolder>?.ifExpandableParent(block: (IExpandable<*>, IParentItem<*>) -> R): R? { |
There was a problem hiding this comment.
Is it possible to make this a inline function?
There was a problem hiding this comment.
I had it as an inline function. but AS will inform that the performance impact is rather low and it is suggested to keep it a normal function
| /** | ||
| * internal helper function to check if an item is expanded. | ||
| */ | ||
| internal fun IItem<out RecyclerView.ViewHolder>?.isExpanded(): Boolean { |
There was a problem hiding this comment.
Not sure if we should use a extension function or property-.
There was a problem hiding this comment.
But if the property would be on the IItem then internal would prevent it from being accessible in the expandable module
There was a problem hiding this comment.
It's possible to create extension properties not just functions.
There was a problem hiding this comment.
I have no strong opinion on this here. Feel free to adjust :)
There was a problem hiding this comment.
ok went ahead and changed it :D
* fix formatting issue
| /** | ||
| * internal helper function to check if an item is expanded. | ||
| */ | ||
| val IItem<out RecyclerView.ViewHolder>?.isExpanded: Boolean |
There was a problem hiding this comment.
@mikepenz wait so did you decide for this to be internal or not? This is currently public
There was a problem hiding this comment.
As per your previous comment, I'm not sure why an internal val would not be accessible in the module from which it is declared
There was a problem hiding this comment.
oops the internal got lost with me changing it to the property. You are absolutely right. will address this.
| @@ -267,15 +286,15 @@ class ExpandableExtension<Item : GenericItem>(private val fastAdapter: FastAdapt | |||
| * @param position the global position of the current item | |||
| * @return a set with the global positions of all expanded items on the root level | |||
| */ | |||
| fun getExpandedItemsRootLevel(position: Int): IntArray { | |||
| val expandedItemsList = ArraySet<Int>() | |||
| fun getExpandedItemsRootLevel(position: Int): List<Int> { | |||
There was a problem hiding this comment.
Yes. That's another reason to go for v4.1.x
I think the usage of this should be == 0, and migration should be super simple. (considered a new function and deprecate the old, but not sure if it is worth it)
This PR fixes various concerns as raised:
#801
#805
#806
It also includes some other smaller changes like proper experimental flag usage, and re-add back the DSL marker.
Also change to use
asSequenceand make those functions return lists instead of an array (no reason why it would require to be an array, also asSequence has no direct asIntArray function)cc @AllanWang