[WIP] feat: added v1 for kmm. needs cleanup #164
Conversation
anuragdalia
changed the title
|
@PatilShreyas what do you suggest? |
|
@anuragdalia that's nice! Let me review the changes and will get back. Meanwhile, I assume you might have utilized the latest API from compose: https://developer.android.com/develop/ui/compose/graphics/draw/modifiers#composable-to-bitmap |
|
Sure. The new api is only available in 1.7.0-alpha. |
AFAIK, that new API is Multi-platform compatible. That's why looks more promising and reliable too. Will check it and will get back to you |
|
@anuragdalia have you tested this implementation with any example on iOS simulator? |
| val width = this.size.width | ||
| val height = this.size.height | ||
|
|
||
| onDrawWithContent { |
There was a problem hiding this comment.
I would strongly recommend to use this API which is KMP-compatible and comes from androix.compose package which will be reliable: https://developer.android.com/develop/ui/compose/graphics/draw/modifiers#composable-to-bitmap
There was a problem hiding this comment.
I tried this. It says cmp but shows up in android only at the moment. Maybe some temporary doc/release bug.
- it's in alpha. So we could move to it when it's stable
What do say?
| canvas.drawPicture(this) | ||
| bitmap.setImmutable() | ||
| return bitmap | ||
| } |
There was a problem hiding this comment.
Also, instead of creating duplicate Capturable implementation across multiple platforms, can't we just create a platform-specific implementation for CacheDrawModifierNode and internal implementation like Bitmap transformation, etc?
There was a problem hiding this comment.
I was leaving room for more stronger changes that might come. But I think we can simplify it for now if you think that'd be better.
I'm pro the current setup so we don't have to do alot of expect/actuals
There was a problem hiding this comment.
Given this is the core code.
There was a problem hiding this comment.
Anyway when 1.7.0 is gonna release, we'll have to cleanup this again. But currently this has lot of duplicate code and I only want a code here which has platform level differences.
There was a problem hiding this comment.
It would be nice if we keep all common things in commonMain and only modifier implementation in the platform specific source.
There was a problem hiding this comment.
Can you please make a change with this?
| @@ -79,7 +59,7 @@ class CaptureController { | |||
| * @param config Bitmap config of the desired bitmap. Defaults to [Bitmap.Config.ARGB_8888] | |||
| */ | |||
| @ExperimentalComposeApi | |||
| fun captureAsync(config: Bitmap.Config = Bitmap.Config.ARGB_8888): Deferred<ImageBitmap> { | |||
| fun captureAsync(config: ColorType = ColorType.ARGB_4444): Deferred<ImageBitmap> { | |||
There was a problem hiding this comment.
This will be a API breaking change but I think we can't get a rid of it 🤔, thoughts?
There was a problem hiding this comment.
No we can't.
Or maybe We can have a custom class with that same name. And typealias in android. I think it won't break for old lib users then I guess. Need to dig
There was a problem hiding this comment.
Okay, we'll plan backward compatibility for this, no worries
|
Hey yes! I did test it on an ios device. Infact I'm using it in an app. |
|
@anuragdalia Any update on the changes which we discussed? |
|
Thank you @PatilShreyas for the amazing library and for adding the kmm support @anuragdalia. |
@minmax1993 Would be helpful if you share your feedback |
|
@anuragdalia Well I found the iOS implementation didn't use any iOS API so it can also runs on desktop and web. I added desktop and wasmJs target and created a PR: anuragdalia#1 |
Hi @minmax1993 |
Summary
Checklist