#8: Separate user-defined properties into dedicated storage #18

shubertm · 2023-09-28T12:33:19Z

Separate user-defined properties into dedicated storage #8

app/build.gradle

kirillt · 2023-10-06T02:23:21Z

We have small bug here.

Steps to reproduce:

Create several notes or open a folder with existing notes.
Create new note, e.g. "abc". Press save.
The new note is displayed in the list.
Tap it to open.
Go back.
The list lacks new "abc" note.
Open any note. Go back.
The note is there again.

kirillt · 2023-10-21T05:37:28Z

The bug with disappearing notes is fixed now. I've updated ark-filepicker dependency to take it from GitHub Packages.

kirillt · 2023-10-21T06:03:19Z

Feel free to merge as soon as @hieuwu puts a green tick.

shubertm · 2023-10-21T08:29:46Z

Feel free to merge as soon as @hieuwu puts a green tick.

Alright

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

app/build.gradle

hieuwu · 2023-10-21T08:48:10Z

Please also update screenshot before and after change to make sure feature still works well as we don't have test in this repo yet

shubertm · 2023-10-21T09:45:08Z

Please also update screenshot before and after change to make sure feature still works well as we don't have test in this repo yet

This feature has no UI, is in the data layer. Or ma you explain?

hieuwu · 2023-10-21T14:25:55Z

Please also update screenshot before and after change to make sure feature still works well as we don't have test in this repo yet

This feature has no UI, is in the data layer. Or ma you explain?

No worries. All good

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

kirillt · 2023-10-23T09:26:20Z

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

+            } catch (e: Exception) {
+                e.printStackTrace()
+            }


Can any exceptions really occur here? If try-catch is necessary, then better to log at warning level, not just print.

@hieuwu Please correct me if I'm wrong.

The path.deleteIfExists() inside delete method will throw IOException. I think that's why he wrap it in try catch block.

kirillt · 2023-10-23T09:26:52Z

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

+                } catch (e: Exception) {
+                    e.printStackTrace()
+                }


What exceptions can occur here?

Initial implementation was throwing IOException, since we changed to new kotlin extension function forEachLine I think we can remove the block

hieuwu · 2023-10-23T12:20:18Z

app/src/main/java/space/taran/arkmemo/data/viewmodels/EditTextNotesViewModel.kt

 import javax.inject.Inject

 @HiltViewModel
 class EditTextNotesViewModel @Inject constructor(): ViewModel() {

    private val iODispatcher = Dispatchers.IO

-    @Inject lateinit var repo: TextNotesRepository
+    @Inject lateinit var repo: TextNotesRepo


Please inject dependencies in constructor

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

hieuwu · 2023-10-23T12:37:04Z

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

+            } catch (e: Exception) {
+                e.printStackTrace()
+            }


The path.deleteIfExists() inside delete method will throw IOException. I think that's why he wrap it in try catch block.

hieuwu · 2023-10-23T12:37:21Z

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

+                e.printStackTrace()
+            }
+        }
+        this.coroutineContext.job


Why do we return an job object when we don't do anything with it? Using an Unit function is good enough in this case

I was using it locally, just forgot to remove it :)

hieuwu · 2023-10-23T12:47:27Z

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

+    private val _textNotes = MutableStateFlow(listOf<TextNote>())
+    val textNotes: StateFlow<List<TextNote>> = _textNotes


Please do not keep state in repository layer. Repsitory's responsibility is containing logic to access to which data source. Please move the state to ViewModel and perform update the state itself there

hieuwu · 2023-10-23T13:05:26Z

And also when I launch app I see the text displays null? Could you please also take a look?

hieuwu · 2023-10-23T13:08:33Z

app/src/main/java/space/taran/arkmemo/data/viewmodels/EditTextNotesViewModel.kt

 import space.taran.arkmemo.models.TextNote
+import space.taran.arkmemo.preferences.MemoPreferences
 import javax.inject.Inject

 @HiltViewModel


Please do me a favor. If you have time, it would be nice if you can move all view models to ui package. ViewModel is a part of the presentation layer and it controls the state to be displayed in the screen. It does not belong to data layer

…tesRepo.kt Co-authored-by: Hieu Vu <[email protected]>

shubertm · 2023-10-24T06:18:38Z

And also when I launch app I see the text displays null? Could you please also take a look?

I think that's because we read time from file metadata. So when its newly saved the metadata is null, metadata is not yet read from file and we can't read it either.

hieuwu · 2023-10-25T07:05:52Z

Looks fine for now. Assuming that the new code works properly. I will work with you to refactor this project to make the architecture aligned with latest best practices

shubertm · 2023-10-25T08:03:07Z

Looks fine for now. Assuming that the new code works properly. I will work with you to refactor this project to make the architecture aligned with latest best practices

That will be amazing

shubertm added 3 commits September 27, 2023 10:01

Separating user meta-data

d886729

Fix duplicate class

c79892a

Fix duplicate class

c8da6dc

shubertm requested a review from kirillt September 28, 2023 12:33

kirillt reviewed Sep 28, 2023

View reviewed changes

app/build.gradle Outdated Show resolved Hide resolved

kirillt requested a review from mdrlzy September 28, 2023 14:17

Fix duplicate class

184e9bd

shubertm temporarily deployed to Development September 29, 2023 10:52 — with GitHub Actions Inactive

shubertm requested a review from kirillt September 29, 2023 13:19

kirillt changed the title ~~#8: Separate user metadata into dedicated storage~~ #8: Separate user-defined properties into dedicated storage Sep 30, 2023

Light theme

4673959

shubertm temporarily deployed to Development October 2, 2023 06:40 — with GitHub Actions Inactive

kirillt mentioned this pull request Oct 6, 2023

#11: Re-implementation of version tracking #13

Open

Fix saving note

c514149

shubertm temporarily deployed to Development October 8, 2023 07:52 — with GitHub Actions Inactive

kirillt assigned shubertm Oct 21, 2023

Update filepicker dependency

03acd4d

kirillt had a problem deploying to Development October 21, 2023 05:36 — with GitHub Actions Failure

Package prefixes are fixed

f74f9bc

kirillt temporarily deployed to Development October 21, 2023 05:46 — with GitHub Actions Inactive

hieuwu reviewed Oct 21, 2023

View reviewed changes

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

app/build.gradle Outdated Show resolved Hide resolved

Move constants

8013a8b

shubertm temporarily deployed to Development October 22, 2023 10:12 — with GitHub Actions Inactive

Update arklib-android

f7e0861

shubertm temporarily deployed to Development October 22, 2023 10:14 — with GitHub Actions Inactive

kirillt reviewed Oct 23, 2023

View reviewed changes

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

kirillt reviewed Oct 23, 2023

View reviewed changes

hieuwu reviewed Oct 23, 2023

View reviewed changes

Update app/src/main/java/space/taran/arkmemo/data/repositories/TextNo…

be5852b

…tesRepo.kt Co-authored-by: Hieu Vu <[email protected]>

shubertm temporarily deployed to Development October 23, 2023 13:20 — with GitHub Actions Inactive

Update app/src/main/java/space/taran/arkmemo/data/repositories/TextNo…

6053864

…tesRepo.kt Co-authored-by: Hieu Vu <[email protected]>

shubertm had a problem deploying to Development October 23, 2023 17:27 — with GitHub Actions Failure

Minor fixes

b374c36

shubertm had a problem deploying to Development October 24, 2023 06:25 — with GitHub Actions Failure

Fix NotesViewModel

27ee2ab

shubertm temporarily deployed to Development October 25, 2023 06:57 — with GitHub Actions Inactive

shubertm requested review from kirillt and hieuwu October 25, 2023 07:04

hieuwu approved these changes Oct 25, 2023

View reviewed changes

shubertm merged commit 1c94c60 into main Oct 25, 2023
1 check passed

kirillt deleted the user-metadata branch December 16, 2023 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#8: Separate user-defined properties into dedicated storage #18

#8: Separate user-defined properties into dedicated storage #18

shubertm commented Sep 28, 2023 •

edited by kirillt

Loading

kirillt commented Oct 6, 2023

kirillt commented Oct 21, 2023

kirillt commented Oct 21, 2023

shubertm commented Oct 21, 2023

hieuwu commented Oct 21, 2023

shubertm commented Oct 21, 2023

hieuwu commented Oct 21, 2023

kirillt Oct 23, 2023

hieuwu Oct 23, 2023

kirillt Oct 23, 2023

shubertm Oct 23, 2023

hieuwu Oct 23, 2023

hieuwu Oct 23, 2023

hieuwu Oct 23, 2023

shubertm Oct 23, 2023

hieuwu Oct 23, 2023

hieuwu commented Oct 23, 2023

hieuwu Oct 23, 2023

shubertm commented Oct 24, 2023

hieuwu commented Oct 25, 2023

shubertm commented Oct 25, 2023

		private val _textNotes = MutableStateFlow(listOf<TextNote>())
		val textNotes: StateFlow<List<TextNote>> = _textNotes

#8: Separate user-defined properties into dedicated storage #18

#8: Separate user-defined properties into dedicated storage #18

Conversation

shubertm commented Sep 28, 2023 • edited by kirillt Loading

kirillt commented Oct 6, 2023

kirillt commented Oct 21, 2023

kirillt commented Oct 21, 2023

shubertm commented Oct 21, 2023

hieuwu commented Oct 21, 2023

shubertm commented Oct 21, 2023

hieuwu commented Oct 21, 2023

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

hieuwu commented Oct 23, 2023

Choose a reason for hiding this comment

shubertm commented Oct 24, 2023

hieuwu commented Oct 25, 2023

shubertm commented Oct 25, 2023

shubertm commented Sep 28, 2023 •

edited by kirillt

Loading