Use /sources/stats in the home #133

Merged
AmineB merged 9 commits from davidoskky/ReaderForSelfoss-multiplatform:sources into master 2023-03-13 16:26:57 +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.

This is implements feature #131 and it will allow implementing #132
With this, public mode functions perfectly and also has source filtering.

## 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. - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. - [x] This is **NOT** translation related. This is implements feature #131 and it will allow implementing #132 With this, public mode functions perfectly and also has source filtering.
davidoskky added 5 commits 2023-02-07 19:51:33 +00:00
AmineB reviewed 2023-02-07 19:54:58 +00:00
@ -56,3 +56,3 @@
CoroutineScope(Dispatchers.Main).launch {
val response = repository.getSources()
val response = repository.getSourcesDetails()
Owner

Why would the SourcesActivity use the "public" route, instead of the actual one ?

Why would the SourcesActivity use the "public" route, instead of the actual one ?
Author
Contributor

I just changed the names of the methods it's the same endpoint.
I changed the names to make them more descriptive: getSourcesStats for /sources/stats and getSourcesDetails for /sources/details

I just changed the names of the methods it's the same endpoint. I changed the names to make them more descriptive: getSourcesStats for /sources/stats and getSourcesDetails for /sources/details
AmineB marked this conversation as resolved
AmineB reviewed 2023-02-07 19:57:29 +00:00
@ -62,3 +62,3 @@
}
if (itm.error.isNotBlank()) {
if (itm.error.isNullOrBlank()) {
Owner

This does not make sens. If the error is null or blank, the error text should not be displayed.

This does not make sens. If the error is null or blank, the error text should not be displayed.
AmineB marked this conversation as resolved
davidoskky added 1 commit 2023-02-07 20:46:01 +00:00
Review
All checks were successful
continuous-integration/drone/pr Build is passing
6ace50c306
AmineB reviewed 2023-02-07 20:55:00 +00:00
@ -68,0 +74,4 @@
var params: SourceParams?
) {
constructor(sourceDetail: SourceDetail) : this(
Owner

This should be an extension function fun SourceDetail.toSource(): Source

This should be an extension function `fun SourceDetail.toSource(): Source`
Owner

Ignore this.

Ignore this.
AmineB marked this conversation as resolved
AmineB reviewed 2023-02-07 20:55:21 +00:00
@ -68,0 +85,4 @@
params = sourceDetail.params
)
constructor(sourceStat: SourceStats) : this(
Owner

This should be an extension function fun SourceStats.toSource(): Source

This should be an extension function `fun SourceStats.toSource(): Source`
Owner

Ignore this.

Ignore this.
AmineB marked this conversation as resolved
AmineB reviewed 2023-02-07 21:04:24 +00:00
@ -64,4 +64,3 @@
}
@Serializable
data class Source(
Owner

Source data class should be changed to an interface

 public interface Source {
        val id: Int
        val title: String
        var unread: Int?
        val icon: String?
    }

SourceDetail and SourceStats should implement it.

`Source` data class should be changed to an interface ``` public interface Source { val id: Int val title: String var unread: Int? val icon: String? } ``` `SourceDetail` and `SourceStats` should implement it.
AmineB marked this conversation as resolved
AmineB reviewed 2023-02-07 21:07:38 +00:00
@ -180,23 +181,49 @@ class Repository(
}
}
suspend fun getSources(): ArrayList<SelfossModel.Source> {
Owner

There should be two methods:

suspend fun getSourceDetailsOrStats(): ArrayList<SelfossModel.Source> {
  if (IS PUBLIC MODE) {
    return api.sourcesStats()
  } else {
    copy/past the content of the old `getSources()` method
  }
}

suspend fun getSourcesDetails(): List<SelfossModel.SourceDetail> { // This will be called in the sources activity
    if (isNetworkAvailable()) {
        return api.sourcesDetailed().data.orEmpty()
    } 
    return emptyList<>()
}
There should be two methods: ``` suspend fun getSourceDetailsOrStats(): ArrayList<SelfossModel.Source> { if (IS PUBLIC MODE) { return api.sourcesStats() } else { copy/past the content of the old `getSources()` method } } suspend fun getSourcesDetails(): List<SelfossModel.SourceDetail> { // This will be called in the sources activity if (isNetworkAvailable()) { return api.sourcesDetailed().data.orEmpty() } return emptyList<>() } ```
Author
Contributor

I'm not sure about this. Doing so is fine only as long as we don't care about the unread count. That is only available in the stats.
If we don't plan on displaying the unread count, this is fine; otherwise the amount of unread articles should be available in the home activity.

I'm not sure about this. Doing so is fine only as long as we don't care about the unread count. That is only available in the stats. If we don't plan on displaying the unread count, this is fine; otherwise the amount of unread articles should be available in the home activity.
Owner

Unread count was complicated code-wise. We have no idea if the feature was used/useful, so for now, let's not add it back.

Unread count was complicated code-wise. We have no idea if the feature was used/useful, so for now, let's not add it back.
Author
Contributor

I did find the tags unread count useful. I will not add this here; we can always implement it later.

I did find the tags unread count useful. I will not add this here; we can always implement it later.
AmineB marked this conversation as resolved
AmineB reviewed 2023-02-07 21:10:51 +00:00
AmineB reviewed 2023-02-07 21:15:48 +00:00
AmineB reviewed 2023-02-07 21:16:20 +00:00
AmineB reviewed 2023-02-07 21:23:22 +00:00
@ -45,3 +45,3 @@
val badgeStarred = _badgeStarred.asStateFlow()
private var fetchedSources = false
private var sources = ArrayList<SelfossModel.Source>()
Owner

This shouldn't be changed/added.

This shouldn't be changed/added.
AmineB marked this conversation as resolved
AmineB requested changes 2023-02-07 21:31:55 +00:00
@ -0,0 +1 @@
ALTER TABLE SOURCE ADD COLUMN `unread` INTEGER;
Owner

As unread is only in the public mode, let's not change this.

As `unread` is only in the public mode, let's not change this.
AmineB marked this conversation as resolved
@ -5,3 +4,1 @@
`spout` TEXT NOT NULL,
`error` TEXT NOT NULL,
`icon` TEXT NOT NULL,
`unread` INTEGER,
Owner

As unread is only in the public mode, let's not change this.

As `unread` is only in the public mode, let's not change this.
AmineB marked this conversation as resolved
davidoskky force-pushed sources from c127271ede to 0c942f7a80 2023-03-12 16:26:20 +00:00 Compare
davidoskky added 1 commit 2023-03-12 16:26:32 +00:00
Fix sources tests
All checks were successful
continuous-integration/drone/pr Build is passing
4966fb704e
Author
Contributor

This should address all the comments. Finally the tests have been useful to spot some logical errors I had introduced.

This should address all the comments. Finally the tests have been useful to spot some logical errors I had introduced.
AmineB reviewed 2023-03-12 19:14:06 +00:00
@ -66,0 +71,4 @@
var error: String?
var icon: String?
fun checkSameSource(source: Source): Boolean {
Owner

I don't understand why this is needed ?

I don't understand why this is needed ?
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:14:47 +00:00
@ -66,0 +75,4 @@
return this.id != source.id
}
fun update(source: Source) {
Owner

This should be an "abstract" method.

This should be an "abstract" method.
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:15:02 +00:00
@ -66,0 +82,4 @@
this.update(source)
}
}
fun update(source: SourceStats)
Owner

This should not be here.

This should not be here.
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:15:07 +00:00
@ -66,0 +84,4 @@
}
fun update(source: SourceStats)
fun update(source: SourceDetail)
Owner

This should not be here.

This should not be here.
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:15:31 +00:00
@ -66,0 +88,4 @@
fun toEntity(): SOURCE
operator fun component1(): Int { return id }
Owner

What are these two methods ?

What are these two methods ?
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:16:14 +00:00
@ -70,0 +108,4 @@
this.unread = source.unread
}
override fun update(source: SourceDetail) {
Owner

SourceStats class should not contain SourceDetail update.

`SourceStats` class should not contain `SourceDetail` update.
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:16:44 +00:00
@ -77,0 +142,4 @@
override var icon: String?,
var params: SourceParams?
) : Source {
override fun update(source: SourceStats) {
Owner

SourceDetail should not contain SourceStats update

`SourceDetail` should not contain `SourceStats` update
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:22:33 +00:00
@ -8,0 +4,4 @@
`tags` TEXT,
`spout` TEXT,
`error` TEXT,
`icon` TEXT,
Owner

Why was everything changed to nullable ?

Why was everything changed to nullable ?
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:25:10 +00:00
@ -188,3 +186,1 @@
if (apiSources.success && apiSources.data != null && isDatabaseEnabled) {
resetDBSourcesWithData(apiSources.data)
if (!appSettingsService.isUpdateSourcesEnabled()) {
return if (isDatabaseEnabled) {
Owner

All this is overly complicated.

All this is overly complicated.
AmineB marked this conversation as resolved
AmineB reviewed 2023-03-12 19:25:38 +00:00
@ -191,1 +211,4 @@
val apiSources = api.sourcesStats()
if (apiSources.success && apiSources.data != null) {
fetchedSources = true
sources = updateSources(apiSources.data) as ArrayList<SelfossModel.Source>
Owner

We only update the DB from getSourcesDetails

We only update the DB from `getSourcesDetails`
AmineB marked this conversation as resolved
AmineB requested changes 2023-03-12 19:27:02 +00:00
AmineB left a comment
Owner

After some cleaning, things should be ok.

After some cleaning, things should be ok.
Author
Contributor

It was definitely overcomplicated, I removed most of that update logic. I meant to merge the two information sources to get a complete view, but we're not really using the information anyway so it's not really needed.

It was definitely overcomplicated, I removed most of that update logic. I meant to merge the two information sources to get a complete view, but we're not really using the information anyway so it's not really needed.
davidoskky force-pushed sources from 23c44487ca to 2ae32794be 2023-03-12 20:12:06 +00:00 Compare
AmineB approved these changes 2023-03-13 16:26:23 +00:00
AmineB merged commit e21906e70d into master 2023-03-13 16:26:57 +00:00
davidoskky deleted branch sources 2023-03-13 18:52:38 +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#133
No description provided.