network #28
No reviewers
Labels
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
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Louvorg/ReaderForSelfoss-multiplatform#28
Loading…
Reference in New Issue
Block a user
No description provided.
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: ApiDetails
set(value) {field = if (value < 0) { 0 } else { value } }
init {
networkStatus.start()
networkStatus.stop
should 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
else
block 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 {
null
This should be a todo
@davidoskky can you please rebase your branch ? I just merged the last changed that I wanted on yesterday's PR.
b7b4ff1485
tof7a29d66ca
@ -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 parameter
if (isConnectionAvailable.value && !offlineOverride) {
isConnectionAvailable.value && !offlineOverride
should be refactored inside a method namedisNetworkAvailable()
@ -59,0 +61,4 @@
null
)
if (fetchedItems != null) {
This should be moved after the
else
block@ -76,0 +84,4 @@
null
)
if (fetchedItems != null) {
This should be moved after the
else
block@ -137,3 +186,4 @@
}
suspend fun unmarkAsRead(item: SelfossModel.Item): Boolean {
// TODO: Check internet connection
This should be removed.
@ -259,2 +324,2 @@
val response = api.update()
return response?.isSuccess ?: false
var response = false
offlineOverride = false
It's weird to have this here.
offlineOverride
shouldn't be overridden from inside the repository.If
offlineOverride
istrue
then returnfalse
.There is no need for an exception for this method.
@ -180,6 +179,12 @@ class HomeActivity : AppCompatActivity(), SearchView.OnQueryTextListener, DIAwar
dataBase = 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.IO
CoroutineScope(Dispatchers.Main).launch {
repository.offlineOverride = false
repository.offlineOverride
should 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.VISIBLE
val parser = MercuryApi()
binding.progressBar.visibility = View.VISIBLE
This 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
17f7d58aed
to49abc081b6
@ -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
repository.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 connection
return api.markAsRead(id.toString())?.isSuccess == true
var success = false
Every block that does something like
can be refactored to something like
@ -149,2 +199,2 @@
// TODO: Check internet connection
return api.unmarkAsRead(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Please refactor this
@ -163,2 +216,2 @@
// TODO: Check success, store in DB
return api.starr(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Please refactor this
@ -177,2 +233,2 @@
// TODO: Check internet connection
return api.unstarr(id.toString())?.isSuccess == true
var success = false
if (isNetworkAvailable()) {
Please refactor this
@ -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()) {
Please refactor this
@ -259,2 +323,2 @@
val response = api.update()
return response?.isSuccess ?: false
var response = false
if (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 && !offlineOverride
fun 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
MyApp
class ?(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 ?
f670e222c6
to1d5ab3205e
There 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.