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

WIP: HomeActivity cleanup #40

Closed
davidoskky wants to merge 3 commits from davidoskky/ReaderForSelfoss-multiplatform:dispatchers into master
pull from: davidoskky/ReaderForSelfoss-multiplatform:dispatchers
merge into: Louvorg:master
Conversation 6 Commits 3 Files Changed 4 +217 -206
davidoskky commented 2022-08-25 10:51:17 +00:00
Contributor
Copy Link

Types of changes

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This is NOT translation related.

I'm trying to move most of the code in the HomeActivity to the viewmodel and to use Dispatchers.IO for all operations that use the database or the api.

## Types of changes - [x] I have read the **CONTRIBUTING** document. - [x] My code follows the code style of this project. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [x] All new and existing tests passed. - [x] This is **NOT** translation related. I'm trying to move most of the code in the HomeActivity to the viewmodel and to use Dispatchers.IO for all operations that use the database or the api.
davidoskky added 3 commits 2022-08-25 10:51:18 +00:00
Use Dispatchers.IO for network and database operations
All checks were successful
continuous-integration/drone/pr Build is passing
Details
ac0156748b
AmineB commented 2022-08-25 11:04:16 +00:00
Owner
Copy Link

I'm already doing a lot of cleaning in here https://gitea.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/src/branch/feature/api_timeout_and_settings_cleaning

Maybe wait to continue on this PR ?

I'm already doing a lot of cleaning in here https://gitea.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/src/branch/feature/api_timeout_and_settings_cleaning Maybe wait to continue on this PR ?
davidoskky commented 2022-08-25 11:28:47 +00:00
Author
Contributor
Copy Link

Oh, I did not notice that.
I will wait for you to merge that before continuing.

Oh, I did not notice that. I will wait for you to merge that before continuing.
AmineB commented 2022-08-25 11:42:36 +00:00
Owner
Copy Link

Thanks !

Thanks !
AmineB requested changes 2022-08-25 12:07:30 +00:00
AmineB left a comment
Owner
Copy Link

I did a quick review, but as I'll clean some code, don't spend much time on this for now.

I'll try to finish my changes pretty soon.

I did a quick review, but as I'll clean some code, don't spend much time on this for now. I'll try to finish my changes pretty soon.
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
@ -453,7 +471,7 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
// TODO: refactor this.
private fun handleDrawerItems() {
tagsBadge = emptyMap()
fun handleDrawerData(maybeDrawerData: DrawerData?, loadedFromCache: Boolean = false) {
AmineB commented 2022-08-25 12:05:09 +00:00
Owner
Copy Link

Why did you remove this ?

I think that this will break some features.

Why did you remove this ? I think that this will break some features.
AmineB commented 2022-08-25 12:05:38 +00:00
Owner
Copy Link

I already started refactoring this, so I'll handle this.

I already started refactoring this, so I'll handle this.
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
@ -144,1 +148,4 @@
setContentView(view)
lifecycleScope.launch {
viewModel.refreshingIndicatorProvider.collect { showRefresh ->
binding.swipeRefreshLayout.isRefreshing = showRefresh
AmineB commented 2022-08-25 12:02:28 +00:00
Owner
Copy Link

The view/activity/fragment should be the one that knows when to display the refresh loading or not.

The view/activity/fragment should be the one that knows when to display the refresh loading or not.
davidoskky commented 2022-08-25 15:56:20 +00:00
Author
Contributor
Copy Link

Alright, I'll find a different way to handle this then.

Alright, I'll find a different way to handle this then.
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
@ -155,1 +164,4 @@
customTabActivityHelper = CustomTabActivityHelper()
handleSettings()
lifecycleScope.launch {
AmineB commented 2022-08-25 12:03:35 +00:00
Owner
Copy Link

The view should call the repository and get the data from there.

This seams too complicated.

The view should call the repository and get the data from there. This seams too complicated.
davidoskky commented 2022-08-25 15:57:55 +00:00
Author
Contributor
Copy Link

Storing the items would allow sharing the same data across all activities without passing it around and it can be used to prevent making an api call every time you close an article and come back to the article list; which is in fact unnecessary.

Storing the items would allow sharing the same data across all activities without passing it around and it can be used to prevent making an api call every time you close an article and come back to the article list; which is in fact unnecessary.
AmineB commented 2022-08-25 16:30:17 +00:00
Owner
Copy Link

it can be used to prevent making an api call every time you close an article and come back to the article list

this is intended to fetch the new articles as soon as possible.

> it can be used to prevent making an api call every time you close an article and come back to the article list this is intended to fetch the new articles as soon as possible.
davidoskky commented 2022-08-25 18:35:33 +00:00
Author
Contributor
Copy Link

Personal use case: selfoss fetches new articles every 15 minutes, in the application I will be reading an article in a couple of minutes and then going back to the articles list to select another one.
Every time I go back to the articles list the fetching starts, which is annoying because of the loading spinner appearing at the top each time.
I have no need for it to be updating so often, and most of those updates are useless anyways because most of the times no new items will be available on the server.
I can manually update the articles list at any time if I wish or if I finished reading all the available ones.

I remember someone else opened an issue some time ago complaining about the frequency of updates as well.

Personal use case: selfoss fetches new articles every 15 minutes, in the application I will be reading an article in a couple of minutes and then going back to the articles list to select another one. Every time I go back to the articles list the fetching starts, which is annoying because of the loading spinner appearing at the top each time. I have no need for it to be updating so often, and most of those updates are useless anyways because most of the times no new items will be available on the server. I can manually update the articles list at any time if I wish or if I finished reading all the available ones. I remember someone else opened an issue some time ago complaining about the frequency of updates as well.
AmineB commented 2022-08-25 18:43:11 +00:00
Owner
Copy Link

Every time I go back to the articles list the fetching starts, which is annoying because of the loading spinner appearing at the top each time.

Why don't you swipe between the articles ?

I remember someone else opened an issue some time ago complaining about the frequency of updates as well.

That user in particular had issues with the tags and sources loading. They had a lot of them. That's why the update_sources setting was added.

> Every time I go back to the articles list the fetching starts, which is annoying because of the loading spinner appearing at the top each time. Why don't you swipe between the articles ? > I remember someone else opened an issue some time ago complaining about the frequency of updates as well. That user in particular had issues with the tags and sources loading. They had a lot of them. That's why the update_sources setting was added.
davidoskky commented 2022-08-25 19:17:02 +00:00
Author
Contributor
Copy Link

Why don't you swipe between the articles ?

I have many articles to read and I do not read them in order, it is easier for me to get back to the list and select from there what I want to read rather than swiping through until I find something interesting.

> Why don't you swipe between the articles ? I have many articles to read and I do not read them in order, it is easier for me to get back to the list and select from there what I want to read rather than swiping through until I find something interesting.
AmineB commented 2022-08-26 07:42:34 +00:00
Owner
Copy Link

That makes sens, but I still thinks the app needs to refresh onReload.

We could try to find a way to differenciate between "going back from another activity of the app" and "relaunching the app after it being in the background". This would solve both our use cases.

That makes sens, but I still thinks the app needs to refresh `onReload`. We could try to find a way to differenciate between "going back from another activity of the app" and "relaunching the app after it being in the background". This would solve both our use cases.
davidoskky commented 2022-08-26 11:23:50 +00:00
Author
Contributor
Copy Link

Yes, I feel this could be a great way to solve this issue.

Yes, I feel this could be a great way to solve this issue.
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/HomeActivity.kt
@ -923,3 +877,3 @@
}
private fun reloadBadgeContent() {
private fun handleBadgesContent() {
AmineB commented 2022-08-25 12:00:26 +00:00
Owner
Copy Link

This way better !

This way better !
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/MyApp.kt
@ -84,6 +84,15 @@ class MyApp : MultiDexApplication(), DIAware {
).show()
}
}
CoroutineScope(Dispatchers.Main).launch {
AmineB commented 2022-08-25 11:59:52 +00:00
Owner
Copy Link

I don't think that this should be here. Each activity/fragment should know what they want to display, and how.

I don't think that this should be here. Each activity/fragment should know what they want to display, and how.
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/viewmodel/AppViewModel.kt
@ -29,2 +42,4 @@
}
}
fun updateRemote() {
AmineB commented 2022-08-25 11:55:57 +00:00
Owner
Copy Link

Why were these moved here ?

Why were these moved here ?
davidoskky commented 2022-08-25 16:02:38 +00:00
Author
Contributor
Copy Link

updateRemote doesn't necessarily have to be there, I have done it because I think it makes it easier to read through the activity code.
getItems in the viewmodel allows storing the items in the viewmodel which might provide some advantages.

updateRemote doesn't necessarily have to be there, I have done it because I think it makes it easier to read through the activity code. getItems in the viewmodel allows storing the items in the viewmodel which might provide some advantages.
AmineB commented 2022-08-25 18:18:22 +00:00
Owner
Copy Link

updateRemote doesn't necessarily have to be there, I have done it because I think it makes it easier to read through the activity code.

I find it more difficult to read. Moving some code that's only available and usable in a specific activity in a shared ViewModel isn't necessary.

getItems in the viewmodel allows storing the items in the viewmodel which might provide some advantages.

There is no need to refactor thing this soon. If there is currently no need to have the items in the viewmodel, there is no need to have them here.

> updateRemote doesn't necessarily have to be there, I have done it because I think it makes it easier to read through the activity code. I find it more difficult to read. Moving some code that's only available and usable in a specific activity in a shared `ViewModel` isn't necessary. > getItems in the viewmodel allows storing the items in the viewmodel which *might* provide some advantages. There is no need to refactor thing this soon. If there is currently no need to have the items in the viewmodel, there is no need to have them here.
👍 1
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -43,4 +45,4 @@
init {
// TODO: Dispatchers.IO not available in KMM, an alternative solution should be found
CoroutineScope(Dispatchers.Main).launch {
updateApiVersion()
AmineB commented 2022-08-25 11:53:34 +00:00
Owner
Copy Link

updateApiVersion() and reloadBadges() both check the network availability. This change isn't needed.

`updateApiVersion()` and `reloadBadges()` both check the network availability. This change isn't needed.
davidoskky commented 2022-08-25 16:00:24 +00:00
Author
Contributor
Copy Link

When init is run the connectivity check always fails.
This allows retrieving the badges every time connectivity gets back.
We could include here all operations that should be run as soon as network connectivity is available, such as fetching sources and tags.

When init is run the connectivity check always fails. This allows retrieving the badges every time connectivity gets back. We could include here all operations that should be run as soon as network connectivity is available, such as fetching sources and tags.
AmineB commented 2022-08-25 18:21:40 +00:00
Owner
Copy Link

When init is run the connectivity check always fails.

So this is a bug that should be fixed instead of having this workaround.

This allows retrieving the badges every time connectivity gets back.

This isn't a good thing, since items arn't refreshed on connectivity change. We would have a difference in the items count and the badge count.

We could include here all operations that should be run as soon as network connectivity is available, such as fetching sources and tags.

Fetching sources and tags can be limited via a update_sources. It's also something that shouldn't be done as much as items fetching.

> When init is run the connectivity check always fails. So this is a bug that should be fixed instead of having this workaround. > This allows retrieving the badges every time connectivity gets back. This isn't a good thing, since items arn't refreshed on connectivity change. We would have a difference in the items count and the badge count. > We could include here all operations that should be run as soon as network connectivity is available, *such as fetching sources and tags.* Fetching sources and tags can be limited via a `update_sources`. It's also something that shouldn't be done as much as items fetching.
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -146,3 +153,4 @@
} else {
getDBTags().map { it.toView() }
}
if (tags != null) {
AmineB commented 2022-08-25 11:52:04 +00:00
Owner
Copy Link

resetDBTagsWithData should only be done if network the api call was done, and if update_sources setting is set to false.

`resetDBTagsWithData` should only be done if network the api call was done, and if `update_sources` setting is set to false.
👍 1
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -163,3 +173,4 @@
} else {
ArrayList(getDBSources().map { it.toView() })
}
if (sources != null) {
AmineB commented 2022-08-25 11:51:49 +00:00
Owner
Copy Link

resetDBSourcesWithData should only be done if network the api call was done, and if update_sources setting is set to false.

`resetDBSourcesWithData` should only be done if network the api call was done, and if `update_sources` setting is set to false.
davidoskky commented 2022-08-26 11:25:21 +00:00
Author
Contributor
Copy Link

I guess that most of the stuff in this PR isn't very useful since what we concluded discussing.
I can close this and make a new one only including the changes which are useful after you merge the one you're working on.

I guess that most of the stuff in this PR isn't very useful since what we concluded discussing. I can close this and make a new one only including the changes which are useful after you merge the one you're working on.
AmineB commented 2022-08-26 11:40:13 +00:00
Owner
Copy Link

Ok, great ! I'll let you know when I finish my refactoring.

Ok, great ! I'll let you know when I finish my refactoring.
AmineB commented 2022-08-26 21:41:26 +00:00
Owner
Copy Link

@davidoskky I did a big cleaning as tou can see in #45, but I still need to #44.

@davidoskky I did a big cleaning as tou can see in #45, but I still need to #44.
davidoskky closed this pull request 2022-08-27 09:32:08 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
AmineB
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#40
No description provided.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:dispatchers"

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?