Use /sources/stats in the home #133

Merged
AmineL merged 9 commits from davidoskky/ReaderForSelfoss-multiplatform:sources into master 3 months ago

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 4 months ago
AmineL reviewed 4 months ago
@ -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 ?
Poster

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
AmineL marked this conversation as resolved
AmineL reviewed 4 months ago
@ -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.
AmineL marked this conversation as resolved
davidoskky added 1 commit 4 months ago
continuous-integration/drone/pr Build is passing Details
6ace50c306
Review
AmineL reviewed 4 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 4 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 4 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 4 months ago
@ -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<>() } ```
Poster

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.
Poster

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.
AmineL marked this conversation as resolved
AmineL reviewed 4 months ago
AmineL reviewed 4 months ago
AmineL reviewed 4 months ago
AmineL reviewed 4 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL requested changes 4 months ago
@ -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.
AmineL 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.
AmineL marked this conversation as resolved
davidoskky added 1 commit 3 months ago
davidoskky force-pushed sources from c127271ede to 0c942f7a80 3 months ago
davidoskky added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
4966fb704e
Fix sources tests
Poster

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.
AmineL reviewed 3 months ago
@ -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 ?
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -66,0 +82,4 @@
this.update(source)
}
}
fun update(source: SourceStats)
Owner

This should not be here.

This should not be here.
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -66,0 +84,4 @@
}
fun update(source: SourceStats)
fun update(source: SourceDetail)
Owner

This should not be here.

This should not be here.
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -66,0 +88,4 @@
fun toEntity(): SOURCE
operator fun component1(): Int { return id }
Owner

What are these two methods ?

What are these two methods ?
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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 ?
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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.
AmineL marked this conversation as resolved
AmineL reviewed 3 months ago
@ -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`
AmineL marked this conversation as resolved
AmineL requested changes 3 months ago
AmineL left a comment
Owner

After some cleaning, things should be ok.

After some cleaning, things should be ok.
davidoskky added 1 commit 3 months ago
continuous-integration/drone/pr Build is running Details
23c44487ca
Cleanup
Poster

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 3 months ago
AmineL approved these changes 3 months ago
AmineL merged commit e21906e70d into master 3 months ago
davidoskky deleted branch sources 3 months ago

Reviewers

AmineL approved these changes 3 months ago
continuous-integration/drone/pr Build is failing
The pull request has been merged as e21906e70d.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b davidoskky-sources master
git pull sources

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff davidoskky-sources
git push origin master
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
Loading…
There is no content yet.