#26: Check for note existence before saving #29
Conversation
kirillt
changed the title
| @@ -88,13 +91,19 @@ class TextNotesRepoImpl @Inject constructor(): TextNotesRepo { | |||
| persistNoteProperties(resourceId = id, noteTitle = note.content.title) | |||
|
|
|||
| val resourcePath = root.resolve("$id.$NOTE_EXT") | |||
| val isDestinationExisting = File(resourcePath.toUri()).exists() | |||
There was a problem hiding this comment.
Do we really have to convert path to Uri and use File instance?
There was a problem hiding this comment.
@tuancoltech is it realistic amount of work for this PR to wire index update flow from arklib-android into Memo? This should be the right way to establish awareness of paths in the folder.
There was a problem hiding this comment.
@ShubertMunthali Oh yes, you're right :)
I updated that.
There was a problem hiding this comment.
@kirillt I haven't got what that index update flow actually does for the Memo app.
Do you have an example (e.g, a commit hash) which does the same to other app?
There was a problem hiding this comment.
@tuancoltech yes, in Navigator: https://github.com/ARK-Builders/ARK-Navigator/blob/main/app/src/main/java/dev/arkbuilders/navigator/presentation/screen/resources/ResourcesPresenter.kt
So the idea is that arklib handles all indexing and we can update the index using updateAll or updateOne which result in updates coming via the flow to the consumer. Ideally, the handling of all resources should be done reactive.
There was a problem hiding this comment.
It should be considered as refactoring at this moment, the PR serves different purpose. But this async handling of updates would remove necessity to manually check exists.
There was a problem hiding this comment.
@kirillt That's cool. Yes, I took note of this and will look into the indexing part.
Thanks for the explanation!
There was a problem hiding this comment.
By the way, I'm just curious how the indexing handles the existing file when creating duplicated file? What is its resolution strategy? Rename?
There was a problem hiding this comment.
Ah, that's tough question at the moment. We need to improve arklib regarding duplicates and hash collisions. Basically, in rare cases when several resources map into same id, we just take only one resource, ignore the others. All metadata linked to the id points to all the collided resources though. We need to solve this: ARK-Builders/arklib#58
Exact duplicates always have same id, but I don't think it's a problem in this case. Because user cares about the content not the location, that's the idea of our project. Tricky situation is when duplicates come from different folders, but this is relevant only to Navigator with its aggregated mode.
| @@ -88,13 +91,19 @@ class TextNotesRepoImpl @Inject constructor(): TextNotesRepo { | |||
| persistNoteProperties(resourceId = id, noteTitle = note.content.title) | |||
|
|
|||
| val resourcePath = root.resolve("$id.$NOTE_EXT") | |||
| val isDestinationExisting = File(resourcePath.toUri()).exists() | |||
There was a problem hiding this comment.
| val isDestinationExisting = File(resourcePath.toUri()).exists() | |
| @@ -88,13 +91,19 @@ class TextNotesRepoImpl @Inject constructor(): TextNotesRepo { | |||
| persistNoteProperties(resourceId = id, noteTitle = note.content.title) | |||
|
|
|||
| val resourcePath = root.resolve("$id.$NOTE_EXT") | |||
| val isDestinationExisting = File(resourcePath.toUri()).exists() | |||
| if (isDestinationExisting) { | |||
There was a problem hiding this comment.
| if (isDestinationExisting) { | |
| if (resourcePath.exists()) { |
tuancoltech
force-pushed
the
bug/crash_save_same_note
branch
from
b846dbf
to
ab4a886
Compare
tuancoltech
temporarily deployed
to
Development
GitHub Actions
Inactive
Check for note existence before saving to avoid crash: #26