Sample for `ark-core` `FileStorage` JNI bindings usage by oluiscabral · Pull Request #110 · ARK-Builders/ark-android

oluiscabral · 2024-11-01T21:45:51Z

No description provided.

sample/src/main/java/dev/arkbuilders/sample/storage/StorageDemoFragment.kt

kirillt · 2024-11-09T10:53:54Z

sample/src/main/java/dev/arkbuilders/sample/storage/StorageDemoFragment.kt

            binding.tvMapValues.text = getString(R.string.empty_map)
            return
        }
+        storage!!.writeFS()


I should check with the crate implementation, but do we really need flushing? That isn't handled by the crate?

From what I tested, no

storage.set and storage.remove do not write to fs, only storage.erase deletes file

kirillt · 2024-11-14T15:01:22Z

settings.gradle.kts

        maven {
-            name = "GitHubPackages"
+            name = "arklib-android GitHub Packages"
            url = URI("https://maven.pkg.github.com/ARK-Builders/arklib-android")


Let's use ark-android to avoid confusion with the archived repo.

It was just a fat lib when we were prototyping, now we moved to the next level with modular framework 😁

Yes. I don't know if I understood it correctly. This is indeed a reference to arklib-android archive, it is not a name for the current repo. Should we use another repo instead?

Oh, I didn't notice the link leads to the archived repo.

We certainly should not use archived repos.

But what do we use from arklib-android?

@kirillt It's being used in at least 23 places, just search for dev.arkbuilders.arklib and you'll realize it too. Here's an example on the sample package

ark-android/sample/src/main/java/dev/arkbuilders/sample/App.kt

Lines 4 to 6 in 9ab96f3

import dev.arkbuilders.arklib.data.folders.FoldersRepo

import dev.arkbuilders.arklib.initArkLib

import dev.arkbuilders.arklib.initRustLogger

But what do we use from arklib-android?

TagSelector and ScoreWidget dependent on arklib, they need ResourceIndex, TagStorage, ScoreStorage and so on

Thanks guys, I wasn't realizing it.. I'll come up with plan for the upgrade later.

kirillt · 2024-11-14T16:00:12Z

Doesn't really work on my devices... Tried on Android versions 9 and 11.

Steps to test:

Create an empty folder.
Select it in storage demo.
Either:
a. Leave the prefix field empty.
- Adding some values doesn't lead to creation of the storage on disk.
- Deleting non-existing values doesn't lead to crash or error.
b. Fill the prefix field.
- Crash as soon as the field input is complete.

oluiscabral · 2024-11-30T02:09:03Z

Doesn't really work on my devices... Tried on Android versions 8 and 11.

Steps to test:

Create an empty folder.

Select it in storage demo.

Either:
a. Leave the prefix field empty.

Adding some values doesn't lead to creation of the storage on disk.

Deleting non-existing values doesn't lead to crash or error.
b. Fill the prefix field.

Crash as soon as the field input is complete.

Hi! @kirillt Just repeating what we previously discussed: this issue was being caused by the fact that the released APK did not include ark-core JNI binaries.

Now, with the latest updates, it fortunately might be fixed, but let me know if it still persists

kirillt · 2024-11-30T03:46:57Z

@oluiscabral still doesn't work. Nothing is written to disk.

oluiscabral · 2024-12-01T14:48:59Z

@oluiscabral still doesn't work. Nothing is written to disk.

@kirillt That's unfortunate... But I might know what's happening. I'll try another potential solution and let you know again when it's done

tuancoltech · 2024-12-08T09:53:15Z

.github/workflows/build_sample.yml

+    - name: Download and extract `ark-core` JNI libs
+      run: |
+        wget https://github.com/ARK-Builders/ark-core/releases/download/v1.0.0/jniLibs.zip
+        unzip -d sample/src/main jniLibs.zip
+


Sorry for suddenly jumping into this, but I think implementation(libraries.core) is enough. In theory we don't have to manually download the JNI libs like this because it's already included in the synced dependency of arkcore.

Just repeating what we later discussed: implementation(libraries.core) is not currently enough, since ark-core/java is a JAR, that means JNI libs are not available for use from it. However, I'm working on transforming ark-core/java into an AAR instead (to make JNI libs available directly from the package)

oluiscabral · 2024-12-12T19:46:37Z

@kirillt Just converted the JAR into an AAR. Let me know if it's still working for you

mdrlzy

LGTM
btw FileStorage api lacks method to get Kotlin Map of entries, we have only iterator
Binding use own BTreeMap iterator under the hood. But it is not very convenient to building map manually everytime

mdrlzy

readFS, sync, syncStatus, merge, get not implemented

mdrlzy · 2024-12-13T05:09:45Z

syncStatus called on non-existent file causes crash

JNI DETECTED ERROR IN APPLICATION: JNI FindClass called with pending exception java.lang.RuntimeException: IO error: No such file or directory (os error 2)

oluiscabral · 2024-12-13T12:32:26Z

readFS, sync, syncStatus, merge, get not implemented

syncStatus called on non-existent file causes crash

JNI DETECTED ERROR IN APPLICATION: JNI FindClass called with pending exception java.lang.RuntimeException: IO error: No such file or directory (os error 2)

Thanks for contributing, @mdrlzy!

It's nice that you created a foundation to test each one of the FileStorage methods. I wonder what @tuancoltech thinks about it, because initially I'd also updated the UI and formatted the code, but ended reverting it back after @kirillt told me that Tuan was actually working on it (UI)

Anyways, I have a few considerations about the points you brought. syncStatus indeed throws an error if the FileStorage is not yet persisted on disk. This is actually the behavior of the source code on ark-core. The same applies for sync (that depends on syncStatus), readFS and even erase. All of them presume that writeFS was already called at least once, that is the FileStorage is on disk.

So all that said, maybe should we disable the buttons related to those methods if the button for writeFS was not yet clicked?

About merge and get, both of them are already working for me. Did it crash when you tested them out?

mdrlzy · 2024-12-13T16:52:26Z

It's nice that you created a foundation to test each one of the FileStorage methods. I wonder what @tuancoltech thinks about it, because initially I'd also updated the UI and formatted the code, but ended reverting it back after @kirillt told me that Tuan was actually working on it (UI)

I dont know. Kirill asked me to do review/refactor.

So all that said, maybe should we disable the buttons related to those methods if the button for writeFS was not yet clicked?

Good idea. Done. Can you test it?

About merge and get, both of them are already working for me. Did it crash when you tested them out?

Everything works for me. Except for some cases where the storage file is missing but it seems to be justified

Let's wait for Kirill's review

kirillt · 2024-12-15T06:21:13Z

@oluiscabral to create better API suited for app developers' needs, we need one of these two static functions in fs-storage:

fn list(root: Path): Vec<Path> that lists all storages in a root folder, regardless of their type. I believe that would be too difficult without relying on specific subpath, so we could simply list everything under $root/.ark/user. The sample, or an app could filter this list by the storage name.
- Example: list("/a/b/c/") returns ["/a/b/c/.ark/user/tags", "/a/b/c/.ark/user/scores"]
fn load(root: Path, name: String): Option<BaseStorage> loads the storage with name name if it exists. The path for any combination of root and name is determined by fs-storage and can change in future. Especially, when we add atomic versions into fs-storage.
- Example: load("/a/b/c/", "tags") returns Some(FileStorage("/a/b/c/.ark/user/tags"))
- Example: list("/a/b/c/", "properties") returns Some(FolderStorage("/a/b/c/.ark/user/properties"))
- Example: list("/a/b/c/", "garbage") returns None

This way, the app developer will be able to specify the "root" folder and the storage name and simply load or initialize the storage without working with the file path directly.

@mdrlzy initially I had the idea of flexible path selection in the sample, to ease testing, but I don't think it's a good idea to expose the path via the API to the app developer. We should prevent incorrect usage of the underlying files of the storage. Could you modify the sample to only allow selection of root and name for the storage? If the storage exists, fs-storage must inform the app developer about the storage type: FileStorage, or FolderStorage. If it doesn't exist yet, the app developer is free to choose the type to initialize new storage.

empty storage is ok to be not persisted, for the rest we have sync status, it sounds ok to have storage in pending state if we have something like "flush" or "sync" operation

kirillt · 2024-12-15T06:48:40Z

@mdrlzy also, would be great to add display for SyncStatus that is returned by fn sync_status

It seems to me (although I'm not 100% sure) that usages of flush method should be replaceable with fn sync.

@oluiscabral other minor changes to the BaseStorage API could be done:

restrict visibility to write_fs and read_fs, it should be used only by the storage module itself
declare public getter entries returning same type as read_fs, i.e. Result<&BTreeMap<K, V>>
change return type of read_fs to simply Result<()>

mdrlzy · 2024-12-15T11:17:22Z

initially I had the idea of flexible path selection in the sample, to ease testing, but I don't think it's a good idea to expose the path via the API to the app developer. We should prevent incorrect usage of the underlying files of the storage. Could you modify the sample to only allow selection of root and name for the storage? If the storage exists, fs-storage must inform the app developer about the storage type: FileStorage, or FolderStorage. If it doesn't exist yet, the app developer is free to choose the type to initialize new storage.

empty storage is ok to be not persisted, for the rest we have sync status, it sounds ok to have storage in pending state if we have something like "flush" or "sync" operation

The problem is that if storage file does not exist then syncStatus, sync, readFS, erase causes crash.
So when using api in this form we need to know whether the file exists or not.
Current FileStorage api does not require root at all and it can be placed anywhere.
There is also no binding for FolderStorage yet.

also, would be great to add display for SyncStatus that is returned by fn sync_status

We already have button that shows toast with sync status. Is this not enough?

kirillt · 2024-12-15T15:46:27Z

We already have button that shows toast with sync status. Is this not enough?

Should be enough. Sorry, I haven't tested the APK yet.

oluiscabral · 2024-12-16T11:19:40Z

@oluiscabral to create better API suited for app developers' needs, we need one of these two static functions in fs-storage:

fn list(root: Path): Vec<Path> that lists all storages in a root folder, regardless of their type. I believe that would be too difficult without relying on specific subpath, so we could simply list everything under $root/.ark/user. The sample, or an app could filter this list by the storage name.

Example: list("/a/b/c/") returns ["/a/b/c/.ark/user/tags", "/a/b/c/.ark/user/scores"]

fn load(root: Path, name: String): Option<BaseStorage> loads the storage with name name if it exists. The path for any combination of root and name is determined by fs-storage and can change in future. Especially, when we add atomic versions into fs-storage.

Example: load("/a/b/c/", "tags") returns Some(FileStorage("/a/b/c/.ark/user/tags"))

Example: list("/a/b/c/", "properties") returns Some(FolderStorage("/a/b/c/.ark/user/properties"))

Example: list("/a/b/c/", "garbage") returns None

This way, the app developer will be able to specify the "root" folder and the storage name and simply load or initialize the storage without working with the file path directly.

Sounds great! Do you think it should already be addressed on this PR?

kirillt · 2025-01-04T19:00:32Z

@oluiscabral

Sounds great! Do you think it should already be addressed on this PR?

We can do it in separate PR, too. But if we don't create it before merging this PR, then we should track the issue.

oluiscabral temporarily deployed to Development November 1, 2024 21:45 — with GitHub Actions Inactive

oluiscabral mentioned this pull request Nov 1, 2024

Rust JNI FileStorage bindings sample #109

Closed

oluiscabral requested a review from kirillt November 1, 2024 21:49

kirillt reviewed Nov 9, 2024

View reviewed changes

sample/src/main/java/dev/arkbuilders/sample/storage/StorageDemoFragment.kt Show resolved Hide resolved

kirillt reviewed Nov 9, 2024

View reviewed changes

oluiscabral requested a review from kirillt November 13, 2024 21:12

kirillt reviewed Nov 14, 2024

View reviewed changes

oluiscabral temporarily deployed to Development November 30, 2024 01:43 — with GitHub Actions Inactive

Download and include ark-core JNI libs release during build

cc4e896

oluiscabral force-pushed the fs-storage-jni-bindings branch from 3b6c3eb to cc4e896 Compare November 30, 2024 02:01

oluiscabral temporarily deployed to Development November 30, 2024 02:01 — with GitHub Actions Inactive

oluiscabral requested a review from kirillt November 30, 2024 02:02

Avoid user adding entries without a storage set

2ee8c1b

oluiscabral temporarily deployed to Development December 1, 2024 15:32 — with GitHub Actions Inactive

kirillt requested a review from mdrlzy December 3, 2024 03:56

tuancoltech reviewed Dec 8, 2024

View reviewed changes

oluiscabral added 2 commits December 12, 2024 16:26

Use newly published ark-core AAR

7337904

Remove residuals from old sample building approach

eb92dfe

oluiscabral temporarily deployed to Development December 12, 2024 19:30 — with GitHub Actions Inactive

oluiscabral mentioned this pull request Dec 12, 2024

Build fs-storage and publish Java package ARK-Builders/ark-core#94

Merged

mdrlzy added 5 commits December 13, 2024 08:35

minor refactor: remove files tree, write fs separate btn

8d9d641

typo fix

4dd2fa7

typo fix

af5adf8

syncStatus

38380e9

minor

0ea65b5

mdrlzy temporarily deployed to Development December 13, 2024 03:36 — with GitHub Actions Inactive

mdrlzy temporarily deployed to Development December 13, 2024 03:43 — with GitHub Actions Inactive

mdrlzy temporarily deployed to Development December 13, 2024 03:51 — with GitHub Actions Inactive

mdrlzy approved these changes Dec 13, 2024

View reviewed changes

sync, syncStatus, readFs, merge, get implemented

69c2fba

mdrlzy requested changes Dec 13, 2024

View reviewed changes

mdrlzy temporarily deployed to Development December 13, 2024 05:05 — with GitHub Actions Inactive

oluiscabral added 3 commits December 13, 2024 08:42

Show toast for readFS result data

8bc704f

Set initial fileStorage as a .txt

b2b330b

Show "NOT FOUND" if ID (key) does not exist

2d22945

oluiscabral temporarily deployed to Development December 13, 2024 12:34 — with GitHub Actions Inactive

mdrlzy added 2 commits December 13, 2024 21:21

minor

96e8ce8

Disable btns if storage == null or storage file not exists

bf7accc

mdrlzy temporarily deployed to Development December 13, 2024 16:45 — with GitHub Actions Inactive

	import dev.arkbuilders.arklib.data.folders.FoldersRepo
	import dev.arkbuilders.arklib.initArkLib
	import dev.arkbuilders.arklib.initRustLogger

Conversation

oluiscabral commented Nov 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oluiscabral Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirillt commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oluiscabral commented Nov 30, 2024

Uh oh!

kirillt commented Nov 30, 2024

Uh oh!

oluiscabral commented Dec 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oluiscabral Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oluiscabral commented Dec 12, 2024

Uh oh!

mdrlzy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdrlzy left a comment

Choose a reason for hiding this comment

Uh oh!

mdrlzy commented Dec 13, 2024

Uh oh!

oluiscabral commented Dec 13, 2024

Uh oh!

mdrlzy commented Dec 13, 2024

Uh oh!

kirillt commented Dec 15, 2024

Uh oh!

kirillt commented Dec 15, 2024

Uh oh!

mdrlzy commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirillt commented Dec 15, 2024

Uh oh!

oluiscabral commented Dec 16, 2024

Uh oh!

kirillt commented Jan 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oluiscabral Dec 12, 2024 •

edited

Loading

kirillt commented Nov 14, 2024 •

edited

Loading

oluiscabral Dec 11, 2024 •

edited

Loading

mdrlzy left a comment •

edited

Loading

mdrlzy commented Dec 15, 2024 •

edited

Loading