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

network #28

Merged
AmineB merged 28 commits from davidoskky/ReaderForSelfoss-multiplatform:network into master 2022-08-22 19:01:16 +00:00
Conversation 12 Commits 28 Files Changed 14 +18 -16
1 changed files with 18 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d654b1b0bd - Show all commits

34
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
View File

@ -63,7 +63,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:12:50 +00:00
Outdated
Review
Copy Link

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.
davidoskky commented 2022-08-17 18:29:22 +00:00
Outdated
Review
Copy Link

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.
AmineB commented 2022-08-17 18:35:26 +00:00
Outdated
Review
Copy Link

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`.
suspend fun getNewerItems(): ArrayList<SelfossModel.Item> {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:29:55 +00:00
Outdated
Review
Copy Link

This should be moved after the else block

This should be moved after the `else` block
// TODO: Use the updatedSince parameter
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
val fetchedItems = api.getItems(
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:11:48 +00:00
Outdated
Review
Copy Link

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.
displayedItems.type,
settings.getString("prefer_api_items_number", "200").toInt(),
@ -84,7 +84,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun getOlderItems(): ArrayList<SelfossModel.Item> {
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:30:20 +00:00
Outdated
Review
Copy Link

This should be moved after the else block

This should be moved after the `else` block
val offset = items.size
val fetchedItems = api.getItems(
displayedItems.type,
@ -106,7 +106,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:13:59 +00:00
Outdated
Review
Copy Link

This should be a todo

This should be a todo
suspend fun allItems(itemType: ItemType): List<SelfossModel.Item>? {
return if (isConnectionAvailable.value && !offlineOverride) {
return if (isNetworkAvailable()) {
api.getItems(
itemType.type,
200,
@ -137,7 +137,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun reloadBadges(): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
val response = api.stats()
if (response != null) {
badgeUnread = response.unread
@ -153,7 +153,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun getTags(): List<SelfossModel.Tag>? {
// TODO: Store in DB
return if (isConnectionAvailable.value && !offlineOverride) {
return if (isNetworkAvailable()) {
api.tags()
} else {
// TODO: Compute from database
@ -163,7 +163,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun getSpouts(): Map<String, SelfossModel.Spout>? {
// TODO: Store in DB
return if (isConnectionAvailable.value && !offlineOverride) {
return if (isNetworkAvailable()) {
api.spouts()
} else {
// TODO: Compute from database
@ -173,7 +173,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun getSources(): ArrayList<SelfossModel.Source>? {
// TODO: Store in DB
return if (isConnectionAvailable.value && !offlineOverride) {
return if (isNetworkAvailable()) {
api.sources()
} else {
// TODO: Compute from database
@ -192,7 +192,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun markAsReadById(id: Int): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
success = api.markAsRead(id.toString())?.isSuccess == true
}
return success
@ -209,7 +209,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun unmarkAsReadById(id: Int): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
success = api.unmarkAsRead(id.toString())?.isSuccess == true
}
return success
@ -226,7 +226,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun starrById(id: Int): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
success = api.starr(id.toString())?.isSuccess == true
}
return success
@ -243,7 +243,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:35:04 +00:00
Outdated
Review
Copy Link

Please refactor this

Please refactor this
suspend fun unstarrById(id: Int): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
success = api.unstarr(id.toString())?.isSuccess == true
}
return success
@ -252,7 +252,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun markAllAsRead(items: ArrayList<SelfossModel.Item>): Boolean {
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
success = api.markAllAsRead(items.map { it.id.toString() })?.isSuccess == true
}
@ -304,7 +304,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
filter: String
): Boolean {
var response = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
response = api.createSourceForVersion(
title,
url,
@ -321,7 +321,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun deleteSource(id: Int): Boolean {
// TODO: Store in DB
var success = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:36:21 +00:00
Outdated
Review
Copy Link

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

This should use the same `isNetworkAvailable()` function as the others.
AmineB commented 2022-08-20 18:36:34 +00:00
Outdated
Review
Copy Link

And it can be refactored as the ones before.

And it can be refactored as the ones before.
val response = api.deleteSource(id)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:33:56 +00:00
Outdated
Review
Copy Link

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.
if (response != null) {
success = response.isSuccess
@ -342,7 +342,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun login(): Boolean {
var result = false
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
try {
val response = api.login()
result = response?.isSuccess == true
@ -369,7 +369,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
private suspend fun updateApiVersion() {
apiMajorVersion = settings.getInt("apiVersionMajor", 0)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:38:11 +00:00
Outdated
Review
Copy Link

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` ?
davidoskky commented 2022-08-20 18:56:57 +00:00
Outdated
Review
Copy Link

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
AmineB commented 2022-08-20 19:20:21 +00:00
Outdated
Review
Copy Link

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 ?
if (isConnectionAvailable.value && !offlineOverride) {
if (isNetworkAvailable()) {
val fetchedVersion = api.version()
if (fetchedVersion != null) {
apiMajorVersion = fetchedVersion.getApiMajorVersion()
@ -378,5 +378,7 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
}
private fun isNetworkAvailable() = isConnectionAvailable.value && !offlineOverride
// TODO: Handle offline actions
}