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
AmineB 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 +10 -11
1 changed files with 10 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 713ceb05bf - Show all commits

21
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
View File

@ -95,8 +95,6 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
private val repository : Repository by instance()
private val appSettingsService : AppSettingsService by instance()
data class DrawerData(val tags: List<SelfossModel.Tag>, val sources: List<SelfossModel.Source>)
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
binding = ActivityHomeBinding.inflate(layoutInflater)
@ -349,14 +347,15 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
)
davidoskky commented 2022-10-01 23:29:32 +00:00
Review
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.
CoroutineScope(Dispatchers.IO).launch {
val drawerData = DrawerData(repository.getTags(), repository.getSources())
val tags = repository.getTags()
val sources = repository.getSources()
runOnUiThread {
handleDrawerData(drawerData)
handleDrawerData(tags, sources)
}
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-09-30 13:16:28 +00:00
Outdated
Review
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
Outdated
Review
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.
AmineB commented 2022-10-01 20:44:47 +00:00
Outdated
Review
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
Outdated
Review
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.
AmineB commented 2022-10-01 20:57:58 +00:00
Outdated
Review
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
Outdated
Review
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.
AmineB commented 2022-10-02 11:30:17 +00:00
Review
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.
}
}
private fun handleDrawerData(drawerData: DrawerData) {
private fun handleDrawerData(tags: List<SelfossModel.Tag>, sources: List<SelfossModel.Source>) {
binding.mainDrawer.itemAdapter.clear()
// Filters title with clear action
@ -370,24 +369,24 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
}
// Hidden tags
if (drawerData.tags.isNotEmpty() && appSettingsService.getHiddenTags().isNotEmpty()) {
if (tags.isNotEmpty() && appSettingsService.getHiddenTags().isNotEmpty()) {
secondaryItem(
withDivider = true,
R.string.drawer_item_hidden_tags,
DRAWER_ID_HIDDEN_TAGS
)
handleHiddenTags(drawerData.tags)
handleHiddenTags(tags)
}
// Tags
secondaryItem(withDivider = true, R.string.drawer_item_tags, DRAWER_ID_TAGS)
if (drawerData.tags.isEmpty()) {
if (tags.isEmpty()) {
binding.mainDrawer.itemAdapter.add(
SecondaryDrawerItem()
.apply { nameRes = R.string.drawer_error_loading_tags; isSelectable = false }
)
} else {
handleTags(drawerData.tags)
handleTags(tags)
}
// Sources
@ -395,7 +394,7 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
startActivity(Intent(v!!.context, SourcesActivity::class.java))
false
}
if (drawerData.sources.isEmpty()) {
if (sources.isEmpty()) {
binding.mainDrawer.itemAdapter.add(
SecondaryDrawerItem().apply {
nameRes = R.string.drawer_error_loading_sources
@ -403,7 +402,7 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
}
)
} else {
handleSources(drawerData.sources)
handleSources(sources)
}
// About action