Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgautierfr
Copy link
Member

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 mgautierfr force-pushed the adapt_wrapper branch 2 times, most recently from 8ce4ed1 to 929ae6d Compare February 9, 2023 13:59
Base automatically changed from adapt_wrapper to main February 9, 2023 16:28
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.

# Conflicts:
#	lib/build.gradle
#	lib/src/main/java/org/kiwix/libkiwix/Library.java
@mgautierfr mgautierfr marked this pull request as ready for review March 1, 2023 14:14
@mgautierfr
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member

rgaudin commented Mar 2, 2023

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

@kelson42
Copy link
Contributor

kelson42 commented Mar 2, 2023

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

@mgautierfr
Copy link
Member Author

What do we have as alternative?

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

@kelson42
Copy link
Contributor

kelson42 commented Mar 2, 2023

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.

@mgautierfr mgautierfr mentioned this pull request Mar 6, 2023
@kelson42 kelson42 added this to the 1.0.0 milestone Jul 18, 2023
@kelson42
Copy link
Contributor

kelson42 commented Aug 17, 2023

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

@kelson42 kelson42 modified the milestones: 1.0.0, 1.1.0 Aug 27, 2023
@kelson42 kelson42 modified the milestones: 2.1.0, 2.2.0 Apr 22, 2024
@kelson42 kelson42 modified the milestones: 2.1.1, 2.2.0 May 3, 2024
@kelson42 kelson42 modified the milestones: 2.2.0, 2.3.0 Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants