#11: Version tracking implementation #12

AgCaliva · 2022-12-09T03:00:39Z

Im still working on the new UI for showing notes older versions. Tomorrow(Friday) or Saturday will try to add the rest.
I wait for feedback. Will try to also make code more clean.
Thank you in advance.

app/src/main/java/space/taran/arkmemo/data/ResourceMeta.kt

kirillt · 2022-12-17T19:56:53Z

Cool UI, looks nice! Couple of minor comments regarding it though:

Would be good to add some text labels like "history of changes" for the list of versions.
When user opens a version, probably better to make it on gray background to show that it is in read-only mode.
Existing notes created with main version are not displayed.

Superficial testing has not revealed any bugs, versions are displayed correctly and the number of created files is correct.

We will review and comment the code itself in next couple of days.

shubertm · 2022-12-17T20:44:30Z

I think it will also be good to have an up button on the version list screen.
And i think making the latest version of a note editable from versions screen may improve UX.

mdrlzy

PR lacks formatting

We also need VersionStorage(val root: Path) as a separate class, think of it like it's a wrapper over .ark/versions folder. For an example you can look at PreviewStorage.

It should contain the following methods:

versions(id: ResourceId): List<ResourceId>
addVersion(id: ResourceId, newVersion: ResourceId)
removeVersion(id: ResourceId, removedVersion: ResourceId)

mdrlzy · 2022-12-20T16:22:42Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+    fun saveNote(note: TextNote?,rootResourceId: String? = null):Long {
        if (note != null) {
            val path = getPath()
            if (path != null) {
                Files.list(path)


Can note really be null?

Better to avoid nesting too much

if (path == null) return CODES_CREATING_NOTE.NOTE_NOT_CREATED.errCode.toLong()

mdrlzy · 2022-12-20T16:25:48Z

app/src/main/java/space/taran/arkmemo/models/Version.kt

+@Parcelize
+data class Version (
+    val content: Content,
+    @IgnoredOnParcel
+    val meta: VersionMeta? = null
+): Parcelable
+{
+    @Parcelize
+    data class Content(
+        val idList: List<String>
+        ): Parcelable
+}


Could it be just like that?
I think meta.id should be included in this list too.

@Parcelize data class Version ( val idList: List<String> ): Parcelable

Sorry, do you mean that i should remove rootResourceId and have it as first item in idList?

app/src/main/java/space/taran/arkmemo/data/VersionMeta.kt

AgCaliva · 2022-12-20T19:42:11Z

I agree with all said here, im already working on it, will push changes shortly. Thank you.

AgCaliva · 2022-12-20T19:45:27Z

PR lacks formatting

What you exactly mean by lacks formatting? I have noticed that i have used my old habit from java of using function setter/getter instead of default setter/getter, i guess thats what you mean? Thank you in advance for the review.

mdrlzy · 2022-12-21T16:42:56Z

What you exactly mean by lacks formatting?

I mean code formatting. The indentation is wrong, there are not enough spaces in some places.
Just use Ctrl+Alt+L when editing
https://blog.jetbrains.com/idea/2020/06/code-formatting/

kirillt · 2022-12-22T12:18:59Z

app/src/main/java/space/taran/arkmemo/data/repositories/VersionStorage.kt

+import java.nio.file.Path
+import kotlin.io.path.createDirectories
+
+typealias ResourceId = Long


ResourceId from the library should be used to avoid type diverging in future: https://github.com/ARK-Builders/arklib-android/blob/3334850384f84fe8d5f0545a221cc1fbc14e722e/lib/src/main/java/space/taran/arklib/Lib.kt

kirillt · 2022-12-22T12:20:23Z

app/src/main/java/space/taran/arkmemo/data/VersionMeta.kt

+import space.taran.arkmemo.data.repositories.ResourceId
+
+
+data class VersionMeta(


So just ResourceId could be used, but that will be trivial refactoring and can be done later.

kirillt · 2022-12-22T12:23:38Z

app/src/main/java/space/taran/arkmemo/data/repositories/ArkFiles.kt

+import java.nio.file.Path
+
+object ArkFiles {
+    const val ARK_FOLDER = ".ark"


(Future work)

We need to extract this variable into the Android bindings, it is defined in the library: https://github.com/ARK-Builders/arklib/blob/fea125e8330243522183c187a467dc762dec1fb7/src/lib.rs#L22

kirillt · 2022-12-22T12:30:53Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+                        path,
+                        JsonParser.parseNoteContentToJson(note.content),
+                    )!!
+                if (newResourceId < 0) {//Error creating TextNote file:


(Future work)

~~Good point, but ideally id must always come from the lib and this way we could not worry about wrong values.~~

~~In Rust lib it is always unsigned (u64): https://github.com/ARK-Builders/arklib/blob/main/src/id.rs#L51~~

In Android bindings it is plain Long, but we can rely on it to be unsigned: https://github.com/ARK-Builders/arklib-android/blob/3334850384f84fe8d5f0545a221cc1fbc14e722e/lib/src/main/java/space/taran/arklib/Lib.kt#L50

I see, you are using computeId in other places, this is good. I think it can be used here too.

kirillt · 2022-12-22T12:33:58Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+                if (newResourceId < 0) {//Error creating TextNote file:
+                    return newResourceId
+                }
+                versionStorage.addVersion(newResourceId, rootResourceId)


In addVersion definition, order of the arguments is different:

fun addVersion(id: ResourceId, newVersion: ResourceId?) {

kirillt · 2022-12-22T12:43:13Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+            getPath()?.resolve(ARK_VERSIONS_DIR)?.resolve("${version.meta?.rootResourceId}")
+        removeFileFromMemory(notePath)
+        removeFileFromMemory(versionPath)
+        //we should delete all notes listed in version that don't have been forked? by now it just deletes the last note.


Good question. IMO we should delete all versions, if user initiates deletion from the main screen. When we'll have forks, we'll need to find where the fork occurs and delete only chain of versions down to the fork, e.g. if we have versions A -> B -> C ->D and B -> E, and the user deletes D from main screen, then we remove everything except A, B and E.

If the user delete a version from versions screen, then only particular version must be removed and it's parent and children must be attached (grandparent becomes just parent): with versions A -> B -> C, deletion of B must result in versions A -> C. When we'll have forks, all children should be lifted: with versions A -> B -> C and B -> D, deletion of B would result in A -> C and A -> D.

shubertm · 2022-12-22T12:46:55Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

-        private const val DUMMY_FILENAME =  "Note"
+        const val NOTE_EXT = "note"
+        private const val DUMMY_FILENAME = "Note"
+        private const val ARK_VERSIONS_DIR = ".ark/versions"


Better to use arkFolder().arkVersions(), instead of this line. Usages of ARK_VERSIONS_DIR should use arkFolder().arkVersions() like here

ARK-Memo/app/src/main/java/space/taran/arkmemo/data/repositories/VersionStorage.kt

Line 16 in 9ca0459

private val versionsDir = root.arkFolder().arkVersions()

kirillt · 2022-12-22T13:18:24Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

-                            close()
-                        }
+                        val fileName = filePath.fileName.toString()
+                        val rootResourceId: ResourceId = fileName.toLong()


(Future work)

We have a method for creating filenames from ids:
https://github.com/ARK-Builders/arklib/blob/main/src/id.rs#L31

Example: https://github.com/ARK-Builders/arklib-android/blob/3334850384f84fe8d5f0545a221cc1fbc14e722e/arklib/src/lib.rs#L269

kirillt · 2022-12-22T13:29:07Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+                val bufferedReader = BufferedReader(fileReader)
+                val jsonVersion = StringBuilder()
+                with(bufferedReader) {
+                    forEachLine {


But can we have more than 1 line in the version file?
Am I right, the format is like this?

rootId, version1, version2 ... latestVersion

Or if it's JSON, then something like this?

{ root: ResourceId, children: Array<ResourceId> }

Its JSON, right now the format its that the rootId its the filename, and the versions are contained inside the file:
{ idList: [] }
I think we have talked about putting the rootId as the first item in the list, so i will stop using VersionMeta, then the rootId will just be the first item, and the filename also.
We have talked about that we should only write to file in version when there is more than one version, so i will apply this too, so then the notes from main branch will be compatible.

kirillt · 2022-12-22T13:30:44Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+            val noteFile = File(path.toFile(), "$ResourceId.$NOTE_EXT")
+            val notePath = noteFile.toPath()
+            try {
+                val fileReader = FileReader(noteFile)


I think we should extract common code working with files. Would be easier to read.

kirillt · 2022-12-22T13:43:08Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

+                val fileWriter = FileWriter(tempFile)
+                val bufferedWriter = BufferedWriter(fileWriter)
+                writeToFile(bufferedWriter)
+                val id = computeId(Files.size(tempFile.toPath()), tempFile.toPath())


(Future work)

Would be cool to count the size during writing and the just use it instead of querying filesystem for file size. For now it is not really necessary.

kirillt · 2022-12-22T13:46:37Z

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepository.kt

-
-                } catch (e: Exception) {
-                    e.printStackTrace()
+            val tempFile = File(file, "${DUMMY_FILENAME}.${NOTE_EXT}")


(Future work)

Probably something standard should be used for creating temporary file.

kirillt · 2022-12-22T13:48:54Z

app/src/main/java/space/taran/arkmemo/data/repositories/VersionStorage.kt

+            val bufferedReader = BufferedReader(fileReader)
+            val jsonVersion = StringBuilder()
+            with(bufferedReader) {
+                forEachLine {


Seems that we have 4 instances of this code reading a file, we should have a method for this.

kirillt · 2022-12-22T14:03:57Z

app/src/main/java/space/taran/arkmemo/models/Version.kt

+
+@Parcelize
+data class Version(
+    val content: Content,


Actually, title from content is also metadata, it will be moved into MetadataStorage in the future.

kirillt · 2022-12-22T14:05:43Z

app/src/main/java/space/taran/arkmemo/ui/adapters/VersionsListAdapter.kt

+            val selectedNote = notes!![bindingAdapterPosition]
+            val selectedVersion = version!!
+            val isReadOnly =
+                bindingAdapterPosition != notes!!.size - 1//read only unless its last item.


(Minor) I would write it like bindingAdapterPosition < notes!!size

kirillt · 2023-01-16T03:51:24Z

We'll continue this work in other PR.

t90r and others added 2 commits December 8, 2022 23:49

Version tracking implementation

a956027

finish version tracking.

7b52344

kirillt reviewed Dec 17, 2022

View reviewed changes

app/src/main/java/space/taran/arkmemo/data/ResourceMeta.kt Outdated Show resolved Hide resolved

kirillt requested review from mdrlzy and shubertm December 17, 2022 19:59

mdrlzy reviewed Dec 20, 2022

View reviewed changes

review changes

0b5ff2a

code formatting

9ca0459

kirillt temporarily deployed to Development December 22, 2022 01:11 — with GitHub Actions Inactive

kirillt reviewed Dec 22, 2022

View reviewed changes

shubertm reviewed Dec 22, 2022

View reviewed changes

kirillt reviewed Dec 22, 2022

View reviewed changes

kirillt closed this Jan 16, 2023

kirillt mentioned this pull request May 27, 2023

#11: Re-implementation of version tracking #13

Open

kirillt changed the title ~~Version tracking implementation~~ #11: Version tracking implementation May 27, 2023

kirillt mentioned this pull request Sep 14, 2023

Bounty #8 for Shubert #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#11: Version tracking implementation #12

#11: Version tracking implementation #12

AgCaliva commented Dec 9, 2022

kirillt commented Dec 17, 2022

shubertm commented Dec 17, 2022

mdrlzy left a comment

mdrlzy Dec 20, 2022

mdrlzy Dec 20, 2022

AgCaliva Jan 9, 2023

AgCaliva commented Dec 20, 2022

AgCaliva commented Dec 20, 2022

mdrlzy commented Dec 21, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022 •

edited

Loading

kirillt Dec 22, 2022 •

edited

Loading

kirillt Dec 22, 2022

kirillt Dec 22, 2022 •

edited

Loading

shubertm Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

AgCaliva Jan 9, 2023

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt Dec 22, 2022

kirillt commented Jan 16, 2023

		import space.taran.arkmemo.data.repositories.ResourceId


		data class VersionMeta(

#11: Version tracking implementation #12

#11: Version tracking implementation #12

Conversation

AgCaliva commented Dec 9, 2022

kirillt commented Dec 17, 2022

shubertm commented Dec 17, 2022

mdrlzy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgCaliva commented Dec 20, 2022

AgCaliva commented Dec 20, 2022

mdrlzy commented Dec 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillt Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

kirillt Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillt Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillt commented Jan 16, 2023

kirillt Dec 22, 2022 •

edited

Loading

kirillt Dec 22, 2022 •

edited

Loading

kirillt Dec 22, 2022 •

edited

Loading