search

kiwix / java-libkiwix

Libkiwix binding for Java & Kotlin
https://central.sonatype.com/artifact/org.kiwix/libkiwix
GNU General Public License v3.0
3 stars 4 forks source link

Use a common class Wrapper and IterWrapper to wrap native resource. #25

Open mgautierfr opened 1 year ago

mgautierfr commented 1 year ago

Native resource (pointer to shared_ptr) is stored as a long in the Wrapper.Resource class.

As Wrapper implements AutoCloseable and we register the (java) resource to be cleaned at object destruction, we now properly delete the native resource.

This also adding new macro to avoid writting the class name and so reduce potential typos.

mgautierfr commented 1 year ago

This is ready for review. This mainly rewrite the wrapping thing with a base class to correctly handle wrapped cpp instance deletion.

There is still a open question on the change of min sdk version (https://github.com/kiwix/java-libkiwix/pull/25/files#diff-655a69127303f6948c0b150902436756156ec7f82640e994c1f552cbdec5bbceL29-L33). This is needed to have the Cleaner feature but I don't know what is the impact on the android side (especially supported phones)

MohitMaliFtechiz commented 1 year ago

There is still a open question on the change of min sdk version

@mgautierfr , If we change minimum SDK version to 33 then it's only support the android 13 or above devices, we can not use this library below android 13.

rgaudin commented 1 year ago

FYI Android 13 is latest stable and has only 12% market share (January 2023)

kelson42 commented 1 year ago

Sounds like a nogo to make that move as such. What do we have as alternative?

mgautierfr commented 1 year ago

What do we have as alternative?

Keep the previous version using deprecated (but probably never removed) functions.

kelson42 commented 1 year ago

OK, to me it sounds we should keep this PR for a moment and reassess the whole thing after next major release of kiwix-android. For the moment continue with the "old" approach.

kelson42 commented 1 year ago

@mgautierfr Would that be wise to keep this PR open longer without merging/finishing it?