network #28
Reference in New Issue
Block a user
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:network"
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?
Types of changes
This PR aims at moving all connectivity checks inside the repository.
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.
@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.
@@ -36,6 +38,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetailsset(value) {field = if (value < 0) { 0 } else { value } }init {networkStatus.start()networkStatus.stopshould be called somewhere.I'm not really sure when it could be stopped, maybe on logout.
This should be done when the app is sent to the background or closed.
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.
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.
@@ -59,0 +60,4 @@null)if (fetchedItems != null) {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.
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.
The
elseblock would have the BD fetching that'll assign the DB items tofetchedItems.@@ -59,0 +64,4 @@items = ArrayList(fetchedItems)}} else {// TODO: Provide an error message if the connection is not available.The error message should be displayed when the network status change, not after every api call.
Same for every comment like this one.
@@ -82,0 +104,4 @@null)} else {nullThis should be a todo
@davidoskky can you please rebase your branch ? I just merged the last changed that I wanted on yesterday's PR.
b7b4ff1485tof7a29d66ca@@ -95,3 +91,3 @@}}if (context.isNetworkAvailable()) {Same here.
@@ -46,65 +45,60 @@ override fun doWork(): Result {val periodicRefresh = settings.getBoolean("periodic_refresh", false)if (periodicRefresh) {if (context.isNetworkAvailable()) {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.
@@ -276,89 +275,87 @@ class ArticleFragment : Fragment(), DIAware {}private fun getContentFromMercury(customTabsIntent: CustomTabsIntent) {if ((context != null && requireContext().isNetworkAvailable(null)) || context == null) {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.
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.
This is mercury. It parses the page and fetch it´s content.
I'll look into why it does not work.
@@ -57,2 +52,2 @@if (fetchedItems != null) {items = ArrayList(fetchedItems)// TODO: Use the updatedSince parameterif (isConnectionAvailable.value && !offlineOverride) {isConnectionAvailable.value && !offlineOverrideshould be refactored inside a method namedisNetworkAvailable()@@ -59,0 +61,4 @@null)if (fetchedItems != null) {This should be moved after the
elseblock@@ -76,0 +84,4 @@null)if (fetchedItems != null) {This should be moved after the
elseblock@@ -137,3 +186,4 @@}suspend fun unmarkAsRead(item: SelfossModel.Item): Boolean {// TODO: Check internet connectionThis should be removed.
@@ -259,2 +324,2 @@val response = api.update()return response?.isSuccess ?: falsevar response = falseofflineOverride = falseIt's weird to have this here.
offlineOverrideshouldn't be overridden from inside the repository.If
offlineOverrideistruethen returnfalse.There is no need for an exception for this method.
@@ -180,6 +179,12 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwardataBase = AndroidDeviceDatabase(applicationContext)lifecycleScope.launch {This shouldn't be only on the home activity.
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.
@@ -1063,0 +1044,4 @@Toast.makeText(this, R.string.refresh_in_progress, Toast.LENGTH_SHORT).show()// TODO: Use Dispatchers.IOCoroutineScope(Dispatchers.Main).launch {repository.offlineOverride = falserepository.offlineOverrideshould only refreshed on app reload or swipe to refresh.It shouldn't be overridden here.
@@ -93,3 +46,1 @@)}}if (periodicRefresh && repository.isNetworkAvailable()) {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.
@@ -279,3 +278,1 @@if ((context != null && requireContext().isNetworkAvailable(null)) || context == null) {binding.progressBar.visibility = View.VISIBLEval parser = MercuryApi()binding.progressBar.visibility = View.VISIBLEThis should only be done if the network is available as it was before.
@@ -43,1 +53,4 @@reloadBadges()isConnectionAvailable.asStateFlow().collect { connectionAvailable ->if (!connectionAvailable) {There should be a message emited on network available too.
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
17f7d58aedto49abc081b6@@ -1043,0 +1017,4 @@Toast.makeText(this, R.string.refresh_in_progress, Toast.LENGTH_SHORT).show()// TODO: Use Dispatchers.IOCoroutineScope(Dispatchers.Main).launch {repository.offlineOverride = falserepository.offlineOverride should only refreshed on app reload or swipe to refresh.
It shouldn't be overridden here.
@@ -134,3 +181,2 @@suspend fun markAsReadById(id: Int): Boolean {// TODO: Check internet connectionreturn api.markAsRead(id.toString())?.isSuccess == truevar success = falseEvery block that does something like
can be refactored to something like
@@ -149,2 +199,2 @@// TODO: Check internet connectionreturn api.unmarkAsRead(id.toString())?.isSuccess == truevar success = falseif (isNetworkAvailable()) {Please refactor this
@@ -163,2 +216,2 @@// TODO: Check success, store in DBreturn api.starr(id.toString())?.isSuccess == truevar success = falseif (isNetworkAvailable()) {Please refactor this
@@ -177,2 +233,2 @@// TODO: Check internet connectionreturn api.unstarr(id.toString())?.isSuccess == truevar success = falseif (isNetworkAvailable()) {Please refactor this
@@ -184,1 +241,3 @@val success = api.markAllAsRead(items.map { it.id.toString() })?.isSuccess == truesuspend fun markAllAsRead(items: ArrayList<SelfossModel.Item>): Boolean {var success = falseif (isNetworkAvailable()) {Please refactor this
@@ -259,2 +323,2 @@val response = api.update()return response?.isSuccess ?: falsevar response = falseif (isConnectionAvailable.value) {This should use the same
isNetworkAvailable()function as the others.And it can be refactored as the ones before.
@@ -299,1 +367,4 @@fun isNetworkAvailable() = isConnectionAvailable.value && !offlineOverridefun startNetwork() {Is these functions needed ?
Can't
connectivityStatus.start()andconnectivityStatus.stop()be called inMyApp?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
Can you add a comment on top of the first function to mention this, please ?
@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.
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
@@ -77,2 +80,4 @@)}lifecycleScope.launch {Instead of doing this in every activity, can't it be done in the
MyAppclass ?(Same for the other activities)
Thank you, I didn't know this was possible...
@@ -0,0 +16,4 @@viewModelScope.launch {repository.isConnectionAvailable.collect { isConnected ->if (isConnected && !wasConnected && repository.connectionMonitored) {_toastMessageProvider.emit("Network connection is now available")It would be better to emit an enum. We'll handle the translation on the android/ios side.
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
I didn´t see that it was in the android side.
Yes, please use localized stringd.
@@ -0,0 +1,65 @@plugins {Instead of adding the library code like this, could you maybe create a jar of the library and add it to the project ?
@davidoskky can you please rebase your branch, please ?
f670e222c6to1d5ab3205eThere is a build issue with the aar file :(
The only remaining issue is the build problem because of the aar file.
The author of the library included the fixes I requested very rapidly, thus the local copy is no longer required.
Great ! I'll test this and merge it.