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

Use /sources/stats in the home #133

Merged
AmineL merged 9 commits from davidoskky/ReaderForSelfoss-multiplatform:sources into master 2023-03-13 16:26:57 +00:00
Conversation 2 Commits 9 Files Changed 9 +130 -75
davidoskky commented 2023-02-07 19:51:32 +00:00
Contributor
Copy Link

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
Use source stats in the home to filter since it is available in public mode
All checks were successful
continuous-integration/drone/pr Build is passing
Details
97793a7fc8
AmineL reviewed 2023-02-07 19:54:58 +00:00
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/SourcesActivity.kt
@ -56,3 +56,3 @@
CoroutineScope(Dispatchers.Main).launch {
val response = repository.getSources()
val response = repository.getSourcesDetails()
AmineL commented 2023-02-07 19:54:58 +00:00
Owner
Copy Link

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 ?
davidoskky commented 2023-02-07 19:56:53 +00:00
Author
Contributor
Copy Link

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 2023-02-07 19:57:29 +00:00
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/adapters/SourcesListAdapter.kt Outdated
@ -62,3 +62,3 @@
}
if (itm.error.isNotBlank()) {
if (itm.error.isNullOrBlank()) {
AmineL commented 2023-02-07 19:57:29 +00:00
Owner
Copy Link

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.
👍 1
AmineL 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
Details
6ace50c306
AmineL reviewed 2023-02-07 20:55:00 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -68,0 +74,4 @@
var params: SourceParams?
) {
constructor(sourceDetail: SourceDetail) : this(
AmineL commented 2023-02-07 20:55:00 +00:00
Owner
Copy Link

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

This should be an extension function `fun SourceDetail.toSource(): Source`
AmineL commented 2023-02-07 21:03:37 +00:00
Owner
Copy Link

Ignore this.

Ignore this.
AmineL marked this conversation as resolved
AmineL reviewed 2023-02-07 20:55:21 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -68,0 +85,4 @@
params = sourceDetail.params
)
constructor(sourceStat: SourceStats) : this(
AmineL commented 2023-02-07 20:55:21 +00:00
Owner
Copy Link

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

This should be an extension function `fun SourceStats.toSource(): Source`
AmineL commented 2023-02-07 21:03:46 +00:00
Owner
Copy Link

Ignore this.

Ignore this.
AmineL marked this conversation as resolved
AmineL reviewed 2023-02-07 21:04:24 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -64,4 +64,3 @@
}
@Serializable
data class Source(
AmineL commented 2023-02-07 21:04:24 +00:00
Owner
Copy Link

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.
👍 1
AmineL marked this conversation as resolved
AmineL reviewed 2023-02-07 21:07:38 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -180,23 +181,49 @@ class Repository(
}
}
suspend fun getSources(): ArrayList<SelfossModel.Source> {
AmineL commented 2023-02-07 21:07:38 +00:00
Owner
Copy Link

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<>() } ```
davidoskky commented 2023-02-11 17:02:43 +00:00
Author
Contributor
Copy Link

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.
AmineL commented 2023-02-12 19:41:48 +00:00
Owner
Copy Link

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.
👍 1
davidoskky commented 2023-02-13 08:44:58 +00:00
Author
Contributor
Copy Link

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 2023-02-07 21:10:51 +00:00
AmineL reviewed 2023-02-07 21:15:48 +00:00
AmineL reviewed 2023-02-07 21:16:20 +00:00
AmineL reviewed 2023-02-07 21:23:22 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -45,3 +45,3 @@
val badgeStarred = _badgeStarred.asStateFlow()
private var fetchedSources = false
private var sources = ArrayList<SelfossModel.Source>()
AmineL commented 2023-02-07 21:23:22 +00:00
Owner
Copy Link

This shouldn't be changed/added.

This shouldn't be changed/added.
AmineL marked this conversation as resolved
AmineL requested changes 2023-02-07 21:31:55 +00:00
shared/src/commonMain/sqldelight/bou/amine/apps/readerforselfossv2/dao/3.sqm
@ -0,0 +1 @@
ALTER TABLE SOURCE ADD COLUMN `unread` INTEGER;
AmineL commented 2023-02-07 21:31:44 +00:00
Owner
Copy Link

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
shared/src/commonMain/sqldelight/bou/amine/apps/readerforselfossv2/dao/Sources.sq
@ -5,3 +4,1 @@
`spout` TEXT NOT NULL,
`error` TEXT NOT NULL,
`icon` TEXT NOT NULL,
`unread` INTEGER,
AmineL commented 2023-02-07 21:31:50 +00:00
Owner
Copy Link

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.
👍 1
AmineL 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
Details
4966fb704e
davidoskky commented 2023-03-12 16:30:17 +00:00
Author
Contributor
Copy Link

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 2023-03-12 19:14:06 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -66,0 +71,4 @@
var error: String?
var icon: String?
fun checkSameSource(source: Source): Boolean {
AmineL commented 2023-03-12 19:14:05 +00:00
Owner
Copy Link

I don't understand why this is needed ?

I don't understand why this is needed ?
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:14:47 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -66,0 +75,4 @@
return this.id != source.id
}
fun update(source: Source) {
AmineL commented 2023-03-12 19:14:47 +00:00
Owner
Copy Link

This should be an "abstract" method.

This should be an "abstract" method.
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:15:02 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -66,0 +82,4 @@
this.update(source)
}
}
fun update(source: SourceStats)
AmineL commented 2023-03-12 19:15:02 +00:00
Owner
Copy Link

This should not be here.

This should not be here.
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:15:07 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -66,0 +84,4 @@
}
fun update(source: SourceStats)
fun update(source: SourceDetail)
AmineL commented 2023-03-12 19:15:06 +00:00
Owner
Copy Link

This should not be here.

This should not be here.
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:15:31 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -66,0 +88,4 @@
fun toEntity(): SOURCE
operator fun component1(): Int { return id }
AmineL commented 2023-03-12 19:15:31 +00:00
Owner
Copy Link

What are these two methods ?

What are these two methods ?
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:16:14 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -70,0 +108,4 @@
this.unread = source.unread
}
override fun update(source: SourceDetail) {
AmineL commented 2023-03-12 19:16:14 +00:00
Owner
Copy Link

SourceStats class should not contain SourceDetail update.

`SourceStats` class should not contain `SourceDetail` update.
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:16:44 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
@ -77,0 +142,4 @@
override var icon: String?,
var params: SourceParams?
) : Source {
override fun update(source: SourceStats) {
AmineL commented 2023-03-12 19:16:44 +00:00
Owner
Copy Link

SourceDetail should not contain SourceStats update

`SourceDetail` should not contain `SourceStats` update
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:22:33 +00:00
shared/src/commonMain/sqldelight/bou/amine/apps/readerforselfossv2/dao/Sources.sq
@ -8,0 +4,4 @@
`tags` TEXT,
`spout` TEXT,
`error` TEXT,
`icon` TEXT,
AmineL commented 2023-03-12 19:22:33 +00:00
Owner
Copy Link

Why was everything changed to nullable ?

Why was everything changed to nullable ?
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:25:10 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -188,3 +186,1 @@
if (apiSources.success && apiSources.data != null && isDatabaseEnabled) {
resetDBSourcesWithData(apiSources.data)
if (!appSettingsService.isUpdateSourcesEnabled()) {
return if (isDatabaseEnabled) {
AmineL commented 2023-03-12 19:25:09 +00:00
Owner
Copy Link

All this is overly complicated.

All this is overly complicated.
AmineL marked this conversation as resolved
AmineL reviewed 2023-03-12 19:25:38 +00:00
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
@ -191,1 +211,4 @@
val apiSources = api.sourcesStats()
if (apiSources.success && apiSources.data != null) {
fetchedSources = true
sources = updateSources(apiSources.data) as ArrayList<SelfossModel.Source>
AmineL commented 2023-03-12 19:25:38 +00:00
Owner
Copy Link

We only update the DB from getSourcesDetails

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

After some cleaning, things should be ok.

After some cleaning, things should be ok.
davidoskky commented 2023-03-12 20:10:20 +00:00
Author
Contributor
Copy Link

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
AmineL approved these changes 2023-03-13 16:26:23 +00:00
AmineL 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.
Reviewers
No reviewers
AmineL
Labels
Clear labels   
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 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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
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.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:sources"

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?