Louvorg/ReaderForSelfoss-multiplatform
Louvorg/ReaderForSelfoss-multiplatform
3
2
Fork 2
Code Issues 16 Pull Requests Projects 1 Releases 65 Wiki Activity

Simplify sources and tags handling #70

Merged
AmineL merged 7 commits from davidoskky/ReaderForSelfoss-multiplatform:drawer_data into master 2022-10-04 18:52:39 +00:00
Conversation 2 Commits 7 Files Changed 21 +34 -35
davidoskky commented 2022-09-30 13:11:33 +00:00
Contributor
Copy Link

The objective is to remove direct database access from the home activity and delegate that logic to the repository.

The objective is to remove direct database access from the home activity and delegate that logic to the repository.
davidoskky added 4 commits 2022-09-30 13:11:33 +00:00
Remove unnecessary data class
Some checks failed
continuous-integration/drone/pr Build is failing
Details
713ceb05bf
davidoskky changed title from Simplify sources and tags handling to WIP: Simplify sources and tags handling 2022-09-30 13:11:46 +00:00
AmineL reviewed 2022-09-30 13:16:17 +00:00
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/MyApp.kt Outdated
@ -75,6 +75,7 @@ class MyApp : MultiDexApplication(), DIAware {
).show()
}
}
connectivityStatus.start()
AmineL commented 2022-09-30 13:16:17 +00:00
Owner
Copy Link

Why was this added ?

Why was this added ?
davidoskky commented 2022-10-01 20:27:41 +00:00
Author
Contributor
Copy Link

This starts tracking the connectivity status before the repository is initiated; it fixes a problem with the repository not correctly executing actions in the init method.

This starts tracking the connectivity status before the repository is initiated; it fixes a problem with the repository not correctly executing actions in the init method.
AmineL marked this conversation as resolved
AmineL requested changes 2022-09-30 13:19:42 +00:00
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt Outdated
@ -352,28 +347,15 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
)
CoroutineScope(Dispatchers.IO).launch {
val drawerData = DrawerData(repository.getDBTags().map { it.toView() },
AmineL commented 2022-09-30 13:16:28 +00:00
Owner
Copy Link

Removing this forces the app to make 2 api calls (to the tags and sources routes) even if we have data in the database.

Not sure if we want to do this.

Removing this forces the app to make 2 api calls (to the tags and sources routes) even if we have data in the database. Not sure if we want to do this.
davidoskky commented 2022-10-01 20:36:59 +00:00
Author
Contributor
Copy Link

There is a change in behaviour effectively.
If this is the behaviour we desire, it should be specified in the repository.
I'm not sure this is good, because doing this you will never update sources after you stored them once in the database.

There is a change in behaviour effectively. If this is the behaviour we desire, it should be specified in the repository. I'm not sure this is good, because doing this you will never update sources after you stored them once in the database.
AmineL commented 2022-10-01 20:44:47 +00:00
Owner
Copy Link

The condition drawerData.sources?.isEmpty() == true || appSettingsService.isUpdateSourcesEnabled() was false if there was already data in the DB and isUpdateSourcesEnabled was false, thus not making an api call as I stated in my first comment.

The condition `drawerData.sources?.isEmpty() == true || appSettingsService.isUpdateSourcesEnabled()` was false if there was already data in the DB and `isUpdateSourcesEnabled` was false, thus not making an api call as I stated in my first comment.
davidoskky commented 2022-10-01 20:50:07 +00:00
Author
Contributor
Copy Link

Yes, I made a mistake. You were right in the first comment. However this behavior never update sources once they've been stored in the database.

Yes, I made a mistake. You were right in the first comment. However this behavior never update sources once they've been stored in the database.
AmineL commented 2022-10-01 20:57:58 +00:00
Owner
Copy Link

The pull to refresh was updating them, if I remember correctly

The pull to refresh was updating them, if I remember correctly
davidoskky commented 2022-10-01 21:07:15 +00:00
Author
Contributor
Copy Link

As far as I can see this is not the case; the pull to refresh just calls handleDrawerData().
What I could do, is change the repository implementation of getTags() and getSources() so that they default to fetching from the database rather than the api unless a parameter to force using the api is set; then I could include a call to this method with the force flag in the pull to refresh.

I'm not sure this is the preferred behavior, I guess an user expects the application to have sources and tags up to date once they start it up. Moreover it's a very small amount of data being transferred when compared to the data downloaded for actual articles.

Also, I'm not sure about how this would work with the number of unread items in each tag; it would probably be out of date when you first start the application.

As far as I can see this is not the case; the pull to refresh just calls `handleDrawerData()`. What I could do, is change the repository implementation of `getTags()` and `getSources()` so that they default to fetching from the database rather than the api unless a parameter to force using the api is set; then I could include a call to this method with the force flag in the pull to refresh. I'm not sure this is the preferred behavior, I guess an user expects the application to have sources and tags up to date once they start it up. Moreover it's a very small amount of data being transferred when compared to the data downloaded for actual articles. Also, I'm not sure about how this would work with the number of unread items in each tag; it would probably be out of date when you first start the application.
AmineL commented 2022-10-02 11:30:17 +00:00
Owner
Copy Link

For now, we'll let this like you implemented it. We'll see later how to handle things.

For now, we'll let this like you implemented it. We'll see later how to handle things.
AmineL marked this conversation as resolved
androidApp/src/main/res/values/strings.xml
@ -63,6 +63,7 @@
<string name="card_height_off">Card height will be fixed</string>
<string name="source_code">Source code</string>
<string name="drawer_error_loading_tags">Error loading tags…</string>
<string name="drawer_error_loading_sources">Error loading sources…</string>
AmineL commented 2022-09-30 13:19:29 +00:00
Owner
Copy Link

Adding this will make the build fail.

It should be added to all the translation files with the same value.

Adding this will make the build fail. It should be added to all the translation files with the same value.
👍 1
AmineL marked this conversation as resolved
davidoskky added 1 commit 2022-10-01 20:44:01 +00:00
Start monitoring connectivity status when the repository is initiated.
Some checks are pending
continuous-integration/drone/pr Build is running
Details
ebef0b3511
davidoskky added 1 commit 2022-10-01 20:51:14 +00:00
Add translated strings
Some checks failed
continuous-integration/drone/pr Build is failing
Details
0bcd55bd4e
davidoskky added 1 commit 2022-10-01 23:01:44 +00:00
Adjust repository tests
All checks were successful
continuous-integration/drone/pr Build is passing
Details
c15bf44032
davidoskky reviewed 2022-10-01 23:29:32 +00:00
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
@ -352,28 +347,15 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
)
davidoskky commented 2022-10-01 23:29:32 +00:00
Author
Contributor
Copy Link

This doesn't worked as expected since the itemAdapter is cleared immediately after.

This doesn't worked as expected since the itemAdapter is cleared immediately after.
davidoskky changed title from WIP: Simplify sources and tags handling to Simplify sources and tags handling 2022-10-03 06:44:22 +00:00
davidoskky commented 2022-10-03 06:46:24 +00:00
Author
Contributor
Copy Link

I guess this can be merged for now and some further optimization can be done in the future.
I can start working on #71 to make sure that everything works correctly with the new api for when selfoss 2.19 will be out.
I will work on the points that are not already addressed by #53.

I guess this can be merged for now and some further optimization can be done in the future. I can start working on #71 to make sure that everything works correctly with the new api for when selfoss 2.19 will be out. I will work on the points that are not already addressed by #53.
AmineL commented 2022-10-04 18:49:31 +00:00
Owner
Copy Link

I can start working on #71 to make sure that everything works correctly with the new api for when selfoss 2.19 will be out.
I will work on the points that are not already addressed by #53.

@davidoskky #71 is not a priority. We should focus on the next milestone first.

> I can start working on #71 to make sure that everything works correctly with the new api for when selfoss 2.19 will be out. I will work on the points that are not already addressed by #53. @davidoskky #71 is not a priority. We should focus on [the next milestone](https://gitea.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/milestone/5) first.
👍 1
AmineL merged commit 023a30c008 into master 2022-10-04 18:52:39 +00:00
davidoskky deleted branch drawer_data 2022-10-04 18:57:31 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
AmineL
Labels
Clear labels   
Difficulty - Beginner friendly

   
Difficulty = Easy

   
Difficulty = Hard

   
Difficulty = Medium

   
Priority = CRITICAL

   
Priority = High

   
Priority = Low

   
Priority = Normal

   
Priority = Some day

   
Status - Fixed somewhere else

   
Status - No details provided

   
Status = Can't fix

   
Status = Duplicate

   
Status = Help wanted

   
Status = Invalid

   
Status = Need more details

   
Status = Taken

   
Status = Wontfix

   
Type - Selfoss works like this

   
Type = Bug

   
Type = Chore

   
Type = Enhancement

   
Type = Feature

   
Type = Question

   
Type = SELFOSS API ISSUE

   
Type = Tools

   
Type = UX/Design

   
Up For Grabs
No Label
Difficulty - Beginner friendly
Difficulty = Easy
Difficulty = Hard
Difficulty = Medium
Priority = CRITICAL
Priority = High
Priority = Low
Priority = Normal
Priority = Some day
Status - Fixed somewhere else
Status - No details provided
Status = Can't fix
Status = Duplicate
Status = Help wanted
Status = Invalid
Status = Need more details
Status = Taken
Status = Wontfix
Type - Selfoss works like this
Type = Bug
Type = Chore
Type = Enhancement
Type = Feature
Type = Question
Type = SELFOSS API ISSUE
Type = Tools
Type = UX/Design
Up For Grabs
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Louvorg/ReaderForSelfoss-multiplatform#70
No description provided.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:drawer_data"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?