network #28

Merged
AmineB merged 28 commits from davidoskky/ReaderForSelfoss-multiplatform:network into master 2022-08-22 19:01:16 +00:00
Contributor

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. (See here)

This PR aims at moving all connectivity checks inside the repository.

## 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. (See [here](https://github.com/aminecmi/ReaderforSelfoss/pull/170#issuecomment-355715654)) This PR aims at moving all connectivity checks inside the repository.
Author
Contributor

Long pressing on the icon allows you to start the application in a temporary offline state.
This only applies to the home activity and it is not very clear to me what the purpose of this would be.

Should I maintain this behaviour or can we remove it?
Removing it would make things simpler and I cannot really imagine a situation in which you would wish to start the application without internet connection besides debugging, and that can simply be achieved by disabling internet access on your phone.

Long pressing on the icon allows you to start the application in a temporary offline state. This only applies to the home activity and it is not very clear to me what the purpose of this would be. Should I maintain this behaviour or can we remove it? Removing it would make things simpler and I cannot really imagine a situation in which you would wish to start the application without internet connection besides debugging, and that can simply be achieved by disabling internet access on your phone.
Owner

@davidoskky I added the option because sometimes, there would be a really slow connection, yet, the android network status would considere it as available network and nothing would work.

It would be great to keep this and have some kind of override to handle this, and start the app in offline mode without disabling the phone internet access.

@davidoskky I added the option because sometimes, there would be a really slow connection, yet, the android network status would considere it as available network and nothing would work. It would be great to keep this and have some kind of override to handle this, and start the app in offline mode without disabling the phone internet access.
AmineB requested changes 2022-08-17 14:18:53 +00:00
@ -36,6 +38,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
set(value) {field = if (value < 0) { 0 } else { value } }
init {
networkStatus.start()
Owner

networkStatus.stop should be called somewhere.

`networkStatus.stop` should be called somewhere.
Author
Contributor

I'm not really sure when it could be stopped, maybe on logout.

I'm not really sure when it could be stopped, maybe on logout.
Owner

This should be done when the app is sent to the background or closed.

This should be done when the app is sent to the background or closed.
Author
Contributor

Since the network state is checked by the repository, doing this when the app is sent to background would prevent background jobs.
Calling stop before the app is closed appears redundant to me and I'm not sure how to determine that the app is being closed and not just going in background.

Since the network state is checked by the repository, doing this when the app is sent to background would prevent background jobs. Calling stop before the app is closed appears redundant to me and I'm not sure how to determine that the app is being closed and not just going in background.
Owner

That´s why, in the background service, I asked to keep the network check as it was.

The background service should check, when executed, the network status.

Having things like so, let you stop the network status checker when the app is sent to the background.

That´s why, in the background service, I asked to keep the network check as it was. The background service should check, when executed, the network status. Having things like so, let you stop the network status checker when the app is sent to the background.
AmineB marked this conversation as resolved
@ -59,0 +60,4 @@
null
)
if (fetchedItems != null) {
Owner

This block should be after the else, since the db will work as a fallback and we'll get the items from there.

Same for the other block later.

This block should be after the else, since the db will work as a fallback and we'll get the items from there. Same for the other block later.
Author
Contributor

I'm not sure how the database will handle this.
This is correct as of now, moving this block after the else will generate errors if the else block is encountered since the fetchedItems variables is only defined within the if block.

I'm not sure how the database will handle this. This is correct as of now, moving this block after the else will generate errors if the else block is encountered since the fetchedItems variables is only defined within the if block.
Owner

The else block would have the BD fetching that'll assign the DB items to fetchedItems.

The `else` block would have the BD fetching that'll assign the DB items to `fetchedItems`.
AmineB marked this conversation as resolved
@ -59,0 +64,4 @@
items = ArrayList(fetchedItems)
}
} else {
// TODO: Provide an error message if the connection is not available.
Owner

The error message should be displayed when the network status change, not after every api call.

Same for every comment like this one.

The error message should be displayed when the network status change, not after every api call. Same for every comment like this one.
AmineB marked this conversation as resolved
@ -82,0 +104,4 @@
null
)
} else {
null
Owner

This should be a todo

This should be a todo
AmineB marked this conversation as resolved
Owner

@davidoskky can you please rebase your branch ? I just merged the last changed that I wanted on yesterday's PR.

@davidoskky can you please rebase your branch ? I just merged the last changed that I wanted on yesterday's PR.
davidoskky force-pushed network from b7b4ff1485 to f7a29d66ca 2022-08-17 15:59:07 +00:00 Compare
AmineB requested changes 2022-08-17 18:34:18 +00:00
@ -95,3 +91,3 @@
}
}
if (context.isNetworkAvailable()) {
Owner

Same here.

Same here.
AmineB marked this conversation as resolved
@ -46,65 +45,60 @@ override fun doWork(): Result {
val periodicRefresh = settings.getBoolean("periodic_refresh", false)
if (periodicRefresh) {
if (context.isNetworkAvailable()) {
Owner

I think that this should be the only place where the connectivity check should stay like this.

If there is no network connectivity, you don't do anything in the background.

I think that this should be the only place where the connectivity check should stay like this. If there is no network connectivity, you don't do anything in the background.
AmineB marked this conversation as resolved
@ -276,89 +275,87 @@ class ArticleFragment : Fragment(), DIAware {
}
private fun getContentFromMercury(customTabsIntent: CustomTabsIntent) {
if ((context != null && requireContext().isNetworkAvailable(null)) || context == null) {
Owner

Same here. We are fetching data from an api, and we there is no need to do try anything if there is no network available.

Same here. We are fetching data from an api, and we there is no need to do try anything if there is no network available.
Author
Contributor

Right!
By the way, what should this Mercury be?
I never saw this work and each time I got the article open in the browser instead.

Right! By the way, what should this Mercury be? I never saw this work and each time I got the article open in the browser instead.
Owner

This is mercury. It parses the page and fetch it´s content.

I'll look into why it does not work.

[This is mercury](https://mercury.postlight.com/web-parser/). It parses the page and fetch it´s content. I'll look into why it does not work.
AmineB marked this conversation as resolved
@ -57,2 +52,2 @@
if (fetchedItems != null) {
items = ArrayList(fetchedItems)
// TODO: Use the updatedSince parameter
if (isConnectionAvailable.value && !offlineOverride) {
Owner

isConnectionAvailable.value && !offlineOverride should be refactored inside a method named isNetworkAvailable()

`isConnectionAvailable.value && !offlineOverride` should be refactored inside a method named `isNetworkAvailable()`
AmineB marked this conversation as resolved
@ -59,0 +61,4 @@
null
)
if (fetchedItems != null) {
Owner

This should be moved after the else block

This should be moved after the `else` block
AmineB marked this conversation as resolved
@ -76,0 +84,4 @@
null
)
if (fetchedItems != null) {
Owner

This should be moved after the else block

This should be moved after the `else` block
AmineB marked this conversation as resolved
@ -137,3 +186,4 @@
}
suspend fun unmarkAsRead(item: SelfossModel.Item): Boolean {
// TODO: Check internet connection
Owner

This should be removed.

This should be removed.
AmineB marked this conversation as resolved
@ -259,2 +324,2 @@
val response = api.update()
return response?.isSuccess ?: false
var response = false
offlineOverride = false
Owner

It's weird to have this here. offlineOverride shouldn't be overridden from inside the repository.

If offlineOverride is true then return false.

There is no need for an exception for this method.

It's weird to have this here. `offlineOverride` shouldn't be overridden from inside the repository. If `offlineOverride` is `true` then return `false`. There is no need for an exception for this method.
AmineB marked this conversation as resolved
AmineB requested changes 2022-08-18 19:59:13 +00:00
@ -180,6 +179,12 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
dataBase = AndroidDeviceDatabase(applicationContext)
lifecycleScope.launch {
Owner

This shouldn't be only on the home activity.

This shouldn't be only on the home activity.
Author
Contributor

Yes, however it might actually be better to remove this logic from the repository and implement a viewModel to send these messages.
Localization in the common source set appears to be complicated.

Yes, however it might actually be better to remove this logic from the repository and implement a viewModel to send these messages. Localization in the common source set appears to be complicated.
AmineB marked this conversation as resolved
@ -1063,0 +1044,4 @@
Toast.makeText(this, R.string.refresh_in_progress, Toast.LENGTH_SHORT).show()
// TODO: Use Dispatchers.IO
CoroutineScope(Dispatchers.Main).launch {
repository.offlineOverride = false
Owner

repository.offlineOverride should only refreshed on app reload or swipe to refresh.

It shouldn't be overridden here.

`repository.offlineOverride` should only refreshed on app reload or swipe to refresh. It shouldn't be overridden here.
AmineB marked this conversation as resolved
@ -93,3 +46,1 @@
)
}
}
if (periodicRefresh && repository.isNetworkAvailable()) {
Owner

This should be reverted to the old network status check, because the network availability watcher should be stopped when the app is in the background.

This should be reverted to the old network status check, because the network availability watcher should be stopped when the app is in the background.
AmineB marked this conversation as resolved
@ -279,3 +278,1 @@
if ((context != null && requireContext().isNetworkAvailable(null)) || context == null) {
binding.progressBar.visibility = View.VISIBLE
val parser = MercuryApi()
binding.progressBar.visibility = View.VISIBLE
Owner

This should only be done if the network is available as it was before.

This should only be done if the network is available as it was before.
AmineB marked this conversation as resolved
@ -43,1 +53,4 @@
reloadBadges()
isConnectionAvailable.asStateFlow().collect { connectionAvailable ->
if (!connectionAvailable) {
Owner

There should be a message emited on network available too.

There should be a message emited on network available too.
AmineB marked this conversation as resolved
Author
Contributor

I have set it up to stop monitoring the network once the application goes into background.
Unfortunately this doesn't work at the moment due to a bug in the library I'm using to monitor the network.
I have submitted a bug report: https://github.com/ln-12/multiplatform-connectivity-status/issues/3

I have set it up to stop monitoring the network once the application goes into background. Unfortunately this doesn't work at the moment due to a bug in the library I'm using to monitor the network. I have submitted a bug report: https://github.com/ln-12/multiplatform-connectivity-status/issues/3
davidoskky force-pushed network from 17f7d58aed to 49abc081b6 2022-08-20 10:45:03 +00:00 Compare
davidoskky requested review from AmineB 2022-08-20 10:45:31 +00:00
AmineB requested changes 2022-08-20 18:38:15 +00:00
@ -1043,0 +1017,4 @@
Toast.makeText(this, R.string.refresh_in_progress, Toast.LENGTH_SHORT).show()
// TODO: Use Dispatchers.IO
CoroutineScope(Dispatchers.Main).launch {
repository.offlineOverride = false
Owner

repository.offlineOverride should only refreshed on app reload or swipe to refresh.

It shouldn't be overridden here.

repository.offlineOverride should only refreshed on app reload or swipe to refresh. It shouldn't be overridden here.
AmineB marked this conversation as resolved
@ -134,3 +181,2 @@
suspend fun markAsReadById(id: Int): Boolean {
// TODO: Check internet connection
return api.markAsRead(id.toString())?.isSuccess == true
var success = false
Owner

Every block that does something like

        var success = false
	
        if (isNetworkAvailable()) {
            success = DOSOMETHING()
        }
        return success

can be refactored to something like

return isNetworkAvailable() && DOSOMETHING()

Every block that does something like ``` var success = false if (isNetworkAvailable()) { success = DOSOMETHING() } return success ``` can be refactored to something like ``` return isNetworkAvailable() && DOSOMETHING() ```
AmineB marked this conversation as resolved
@ -149,2 +199,2 @@
// TODO: Check internet connection
return api.unmarkAsRead(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Owner

Please refactor this

Please refactor this
AmineB marked this conversation as resolved
@ -163,2 +216,2 @@
// TODO: Check success, store in DB
return api.starr(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Owner

Please refactor this

Please refactor this
AmineB marked this conversation as resolved
@ -177,2 +233,2 @@
// TODO: Check internet connection
return api.unstarr(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Owner

Please refactor this

Please refactor this
AmineB marked this conversation as resolved
@ -184,1 +241,3 @@
val success = api.markAllAsRead(items.map { it.id.toString() })?.isSuccess == true
suspend fun markAllAsRead(items: ArrayList<SelfossModel.Item>): Boolean {
var success = false
if (isNetworkAvailable()) {
Owner

Please refactor this

Please refactor this
AmineB marked this conversation as resolved
@ -259,2 +323,2 @@
val response = api.update()
return response?.isSuccess ?: false
var response = false
if (isConnectionAvailable.value) {
Owner

This should use the same isNetworkAvailable() function as the others.

This should use the same `isNetworkAvailable()` function as the others.
Owner

And it can be refactored as the ones before.

And it can be refactored as the ones before.
AmineB marked this conversation as resolved
@ -299,1 +367,4 @@
fun isNetworkAvailable() = isConnectionAvailable.value && !offlineOverride
fun startNetwork() {
Owner

Is these functions needed ?

Can't connectivityStatus.start() and connectivityStatus.stop() be called in MyApp ?

Is these functions needed ? Can't `connectivityStatus.start()` and `connectivityStatus.stop()` be called in `MyApp` ?
Author
Contributor

I have tried; unfortunately this is not possible.
Not really a technical problem, it is because the name of the library contains a dash (-) and this generates an error at build time.
This issue has already been reported to the author of the library, though nothing has been done to solve it as far as I can see.
https://github.com/ln-12/multiplatform-connectivity-status/issues/2

I have tried; unfortunately this is not possible. Not really a technical problem, it is because the name of the library contains a dash (-) and this generates an error at build time. This issue has already been reported to the author of the library, though nothing has been done to solve it as far as I can see. https://github.com/ln-12/multiplatform-connectivity-status/issues/2
Owner

Can you add a comment on top of the first function to mention this, please ?

Can you add a comment on top of the first function to mention this, please ?
AmineB marked this conversation as resolved
Owner

@davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ?

@davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ?
Author
Contributor

@davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ?

I have removed it because I figured it would be better to use a viewModel to send toast messages to the UI.
This is because we otherwise would have to split localization among the different source sets.

In the next PR I will create the viewModel which observes the connection status and sends a toast message to all the activities when connection is lost/retrieved.

> @davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ? I have removed it because I figured it would be better to use a viewModel to send toast messages to the UI. This is because we otherwise would have to split localization among the different source sets. In the next PR I will create the viewModel which observes the connection status and sends a toast message to all the activities when connection is lost/retrieved.
davidoskky requested review from AmineB 2022-08-20 19:15:27 +00:00
Owner

@davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ?

I have removed it because I figured it would be better to use a viewModel to send toast messages to the UI.
This is because we otherwise would have to split localization among the different source sets.

In the next PR I will create the viewModel which observes the connection status and sends a toast message to all the activities when connection is lost/retrieved.

This should be added to this PR, since it completely related.

> > @davidoskky it would be great to have the toast message displayed back on status change. Why was it removed ? > > I have removed it because I figured it would be better to use a viewModel to send toast messages to the UI. > This is because we otherwise would have to split localization among the different source sets. > > In the next PR I will create the viewModel which observes the connection status and sends a toast message to all the activities when connection is lost/retrieved. This should be added to this PR, since it completely related.
Author
Contributor

Not being able to stop the connection monitoring was quite annoying and it created a lot of troubles while testing, thus I included a local copy of the library with the fix while we wait that it gets introduced in the official library version. https://github.com/ln-12/multiplatform-connectivity-status/pull/4

Not being able to stop the connection monitoring was quite annoying and it created a lot of troubles while testing, thus I included a local copy of the library with the fix while we wait that it gets introduced in the official library version. https://github.com/ln-12/multiplatform-connectivity-status/pull/4
AmineB requested changes 2022-08-21 18:18:35 +00:00
@ -77,2 +80,4 @@
)
}
lifecycleScope.launch {
Owner

Instead of doing this in every activity, can't it be done in the MyApp class ?

(Same for the other activities)

Instead of doing this in every activity, can't it be done in the `MyApp` class ? (Same for the other activities)
Author
Contributor

Thank you, I didn't know this was possible...

Thank you, I didn't know this was possible...
AmineB marked this conversation as resolved
@ -0,0 +16,4 @@
viewModelScope.launch {
repository.isConnectionAvailable.collect { isConnected ->
if (isConnected && !wasConnected && repository.connectionMonitored) {
_toastMessageProvider.emit("Network connection is now available")
Owner

It would be better to emit an enum. We'll handle the translation on the android/ios side.

It would be better to emit an enum. We'll handle the translation on the android/ios side.
Author
Contributor

What do you mean? This view model is already in the android side; these strings will have to be localized. I can do it now

What do you mean? This view model is already in the android side; these strings will have to be localized. I can do it now
Owner

I didn´t see that it was in the android side.

Yes, please use localized stringd.

I didn´t see that it was in the android side. Yes, please use localized stringd.
AmineB marked this conversation as resolved
@ -0,0 +1,65 @@
plugins {
Owner

Instead of adding the library code like this, could you maybe create a jar of the library and add it to the project ?

Instead of adding the library code like this, could you maybe create a jar of the library and add it to the project ?
AmineB marked this conversation as resolved
Owner

@davidoskky can you please rebase your branch, please ?

@davidoskky can you please rebase your branch, please ?
davidoskky force-pushed network from f670e222c6 to 1d5ab3205e 2022-08-21 21:35:04 +00:00 Compare
davidoskky requested review from AmineB 2022-08-21 21:35:50 +00:00
Owner

Direct local .aar file dependencies are not supported when building an AAR. The resulting AAR would be broken because the classes and Android resources from any local .aar file dependencies would not be packaged in the resulting AAR. Previous versions of the Android Gradle Plugin produce broken AARs in this case too (despite not throwing this error).

There is a build issue with the aar file :(

> Direct local .aar file dependencies are not supported when building an AAR. The resulting AAR would be broken because the classes and Android resources from any local .aar file dependencies would not be packaged in the resulting AAR. Previous versions of the Android Gradle Plugin produce broken AARs in this case too (despite not throwing this error). There is a build issue with the aar file :(
AmineB requested changes 2022-08-22 11:52:58 +00:00
AmineB left a comment
Owner

The only remaining issue is the build problem because of the aar file.

The only remaining issue is the build problem because of the aar file.
davidoskky added 1 commit 2022-08-22 17:34:18 +00:00
Use the connectivity-status library from the repository rather than the local copy
Some checks failed
continuous-integration/drone/pr Build is failing
4c78b22614
Author
Contributor

The author of the library included the fixes I requested very rapidly, thus the local copy is no longer required.

The author of the library included the fixes I requested very rapidly, thus the local copy is no longer required.
Owner

Great ! I'll test this and merge it.

Great ! I'll test this and merge it.
AmineB merged commit a9caaefb4d into master 2022-08-22 19:01:16 +00:00
davidoskky deleted branch network 2022-08-22 19:48:34 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
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#28
No description provided.