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
9 changed files with 130 additions and 75 deletions

View File

@ -49,13 +49,13 @@ class SourcesActivity : AppCompatActivity(), DIAware {
super.onResume()
val mLayoutManager = LinearLayoutManager(this)
var items: ArrayList<SelfossModel.Source>
var items: ArrayList<SelfossModel.SourceDetail>
binding.recyclerView.setHasFixedSize(true)
binding.recyclerView.layoutManager = mLayoutManager
CoroutineScope(Dispatchers.Main).launch {
val response = repository.getSources()
val response = repository.getSourcesDetails()
AmineL marked this conversation as resolved
Review

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 ?
Review

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
if (response.isNotEmpty()) {
items = response
val mAdapter = SourcesListAdapter(

View File

@ -24,7 +24,7 @@ import org.kodein.di.instance
class UpsertSourceActivity : AppCompatActivity(), DIAware {
private var existingSource: SelfossModel.Source? = null
private var existingSource: SelfossModel.SourceDetail? = null
private var mSpoutsValue: String? = null
private lateinit var binding: ActivityUpsertSourceBinding
@ -68,7 +68,7 @@ class UpsertSourceActivity : AppCompatActivity(), DIAware {
private fun initFields(items: Map<String, SelfossModel.Spout>) {
binding.nameInput.setText(existingSource!!.title)
binding.tags.setText(existingSource!!.tags.joinToString(", "))
binding.tags.setText(existingSource!!.tags?.joinToString(", "))
binding.sourceUri.setText(existingSource!!.params?.url)
binding.spoutsSpinner.setSelection(items.keys.indexOf(existingSource!!.spout))
binding.progress.visibility = View.GONE

View File

@ -31,7 +31,7 @@ import org.kodein.di.instance
class SourcesListAdapter(
private val app: Activity,
private val items: ArrayList<SelfossModel.Source>
private val items: ArrayList<SelfossModel.SourceDetail>
) : RecyclerView.Adapter<SourcesListAdapter.ViewHolder>(), DIAware {
private val c: Context = app.baseContext
private val generator: ColorGenerator = ColorGenerator.MATERIAL
@ -61,7 +61,7 @@ class SourcesListAdapter(
c.circularBitmapDrawable(itm.getIcon(repository.baseUrl), binding.itemImage)
}
if (itm.error.isNotBlank()) {
if (!itm.error.isNullOrBlank()) {
AmineL marked this conversation as resolved Outdated

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.
binding.errorText.visibility = View.VISIBLE
binding.errorText.text = itm.error
} else {

View File

@ -84,7 +84,7 @@ class FilterSheetFragment : BottomSheetDialogFragment(), DIAware {
) {
val sourceGroup = binding.sourcesGroup
repository.getSources().forEach { source ->
repository.getSourcesDetailsOrStats().forEach { source ->
val c = Chip(context)
c.ellipsize = TextUtils.TruncateAt.END
@ -141,9 +141,9 @@ class FilterSheetFragment : BottomSheetDialogFragment(), DIAware {
selectedChip = c
}
c.isEnabled = source.error.isBlank()
c.isEnabled = source.error.isNullOrBlank()
if (source.error.isNotBlank() && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
if (!source.error.isNullOrBlank() && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
c.tooltipText = source.error
}

View File

@ -300,9 +300,10 @@ class RepositoryTest {
every { appSettingsService.isItemCachingEnabled() } returns true
initializeRepository(MutableStateFlow(false))
repository.setSourceFilter(SelfossModel.Source(
repository.setSourceFilter(SelfossModel.SourceDetail(
1,
"Test",
null,
listOf("tags"),
SPOUT,
"",
@ -609,30 +610,32 @@ class RepositoryTest {
fun get_sources() {
val (sources, sourcesDB) = prepareSources()
initializeRepository()
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertSame(sources, testSources)
assertEquals(sources, testSources)
assertNotEquals(sourcesDB.map { it.toView() }, testSources)
coVerify(exactly = 1) { api.sources() }
coVerify(exactly = 1) { api.sourcesDetailed() }
}
private fun prepareSources(): Pair<ArrayList<SelfossModel.Source>, List<SOURCE>> {
private fun prepareSources(): Pair<ArrayList<SelfossModel.SourceDetail>, List<SOURCE>> {
val sources = arrayListOf(
SelfossModel.Source(
SelfossModel.SourceDetail(
1,
"First source",
null,
listOf("Test", "second"),
SPOUT,
"",
IMAGE_URL_2,
SelfossModel.SourceParams("url")
),
SelfossModel.Source(
SelfossModel.SourceDetail(
2,
"Second source",
null,
listOf("second"),
SPOUT,
"",
@ -661,7 +664,7 @@ class RepositoryTest {
)
)
coEvery { api.sources() } returns StatusAndData(success = true, data = sources)
coEvery { api.sourcesDetailed() } returns StatusAndData(success = true, data = sources)
every { db.sourcesQueries.sources().executeAsList() } returns sourcesDB
return Pair(sources, sourcesDB)
}
@ -675,13 +678,13 @@ class RepositoryTest {
initializeRepository()
var testSources: List<SelfossModel.Source>?
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
// Sources will be fetched from the database on the second call, thus testSources != sources
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
coVerify(exactly = 1) { api.sources() }
assertNotSame(sources, testSources)
coVerify(exactly = 1) { api.sourcesDetailed() }
assertNotEquals(sources, testSources)
assertEquals(sourcesDB.map { it.toView() }, testSources)
verify(atLeast = 1) { db.sourcesQueries.sources().executeAsList() }
}
@ -693,13 +696,13 @@ class RepositoryTest {
every { appSettingsService.isUpdateSourcesEnabled() } returns true
every { appSettingsService.isItemCachingEnabled() } returns false
initializeRepository()
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertSame(sources, testSources)
coVerify(exactly = 1) { api.sources() }
assertEquals(sources, testSources)
coVerify(exactly = 1) { api.sourcesDetailed() }
verify(exactly = 0) { db.sourcesQueries }
}
@ -710,13 +713,13 @@ class RepositoryTest {
every { appSettingsService.isUpdateSourcesEnabled() } returns false
every { appSettingsService.isItemCachingEnabled() } returns false
initializeRepository()
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertSame(sources, testSources)
coVerify(exactly = 1) { api.sources() }
assertEquals(sources, testSources)
coVerify(exactly = 1) { api.sourcesDetailed() }
verify(atLeast = 1) { db.sourcesQueries }
}
@ -724,13 +727,13 @@ class RepositoryTest {
fun get_sources_without_connection() {
val (_, sourcesDB) = prepareSources()
initializeRepository(MutableStateFlow(false))
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertEquals(sourcesDB.map { it.toView() }, testSources)
coVerify(exactly = 0) { api.sources() }
coVerify(exactly = 0) { api.sourcesDetailed() }
verify(atLeast = 1) { db.sourcesQueries.sources().executeAsList() }
}
@ -741,13 +744,13 @@ class RepositoryTest {
every { appSettingsService.isItemCachingEnabled() } returns false
every { appSettingsService.isUpdateSourcesEnabled() } returns true
initializeRepository(MutableStateFlow(false))
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertEquals(emptyList<SelfossModel.Source>(), testSources)
coVerify(exactly = 0) { api.sources() }
coVerify(exactly = 0) { api.sourcesDetailed() }
verify(exactly = 0) { db.sourcesQueries.sources().executeAsList() }
}
@ -758,13 +761,13 @@ class RepositoryTest {
every { appSettingsService.isItemCachingEnabled() } returns true
every { appSettingsService.isUpdateSourcesEnabled() } returns false
initializeRepository(MutableStateFlow(false))
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertEquals(sourcesDB.map { it.toView() }, testSources)
coVerify(exactly = 0) { api.sources() }
coVerify(exactly = 0) { api.sourcesDetailed() }
verify(atLeast = 1) { db.sourcesQueries.sources().executeAsList() }
}
@ -775,13 +778,13 @@ class RepositoryTest {
every { appSettingsService.isItemCachingEnabled() } returns false
every { appSettingsService.isUpdateSourcesEnabled() } returns false
initializeRepository(MutableStateFlow(false))
var testSources: List<SelfossModel.Source>?
var testSources: List<SelfossModel.Source>
runBlocking {
testSources = repository.getSources()
testSources = repository.getSourcesDetails()
}
assertEquals(sourcesDB.map { it.toView() }, testSources)
coVerify(exactly = 0) { api.sources() }
coVerify(exactly = 0) { api.sourcesDetailed() }
verify(atLeast = 1) { db.sourcesQueries.sources().executeAsList() }
}
@ -1102,9 +1105,10 @@ class RepositoryTest {
private fun prepareSearch() {
repository.setTagFilter(SelfossModel.Tag("Tag", "read", 0))
repository.setSourceFilter(
SelfossModel.Source(
SelfossModel.SourceDetail(
1,
"First source",
5,
listOf("Test", "second"),
SPOUT,
"",

View File

@ -63,17 +63,36 @@ class SelfossModel {
fun isPublicModeEnabled() = publicMode ?: false
}
interface Source {
val id: Int
var title: String
var unread: Int?
var error: String?
var icon: String?
}
@Serializable
AmineL marked this conversation as resolved
Review

I don't understand why this is needed ?

I don't understand why this is needed ?
data class Source(
AmineL marked this conversation as resolved
Review

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.
val id: Int,
val title: String,
data class SourceStats(
override val id: Int,
override var title: String,
AmineL marked this conversation as resolved
Review

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

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

Ignore this.

Ignore this.
override var unread: Int?,
AmineL marked this conversation as resolved
Review

This should be an "abstract" method.

This should be an "abstract" method.
override var error: String? = null,
override var icon: String? = null
) : Source
@Serializable
data class SourceDetail(
override val id: Int,
AmineL marked this conversation as resolved
Review

This should not be here.

This should not be here.
override var title: String,
override var unread: Int? = null,
AmineL marked this conversation as resolved
Review

This should not be here.

This should not be here.
@Serializable(with = TagsListSerializer::class)
AmineL marked this conversation as resolved
Review

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

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

Ignore this.

Ignore this.
val tags: List<String>,
val spout: String,
val error: String,
val icon: String?,
val params: SourceParams?
)
var tags: List<String>?,
var spout: String?,
override var error: String?,
AmineL marked this conversation as resolved
Review

What are these two methods ?

What are these two methods ?
override var icon: String?,
var params: SourceParams?
) : Source
@Serializable
data class SourceParams(
val url: String

View File

@ -44,11 +44,11 @@ class Repository(
private val _badgeStarred = MutableStateFlow(0)
val badgeStarred = _badgeStarred.asStateFlow()
private var fetchedSources = false
private var fetchedTags = false
AmineL marked this conversation as resolved
Review

This shouldn't be changed/added.

This shouldn't be changed/added.
private var fetchedSources = false
private var _readerItems = ArrayList<SelfossModel.Item>()
private var _selectedSource: SelfossModel.Source? = null
private var _selectedSource: SelfossModel.SourceDetail? = null
suspend fun getNewerItems(): ArrayList<SelfossModel.Item> {
var fetchedItems: StatusAndData<List<SelfossModel.Item>> = StatusAndData.error()
@ -180,23 +180,46 @@ class Repository(
}
}
suspend fun getSources(): ArrayList<SelfossModel.Source> {
AmineL marked this conversation as resolved
Review

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<>() } ```
Review

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

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

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.
suspend fun getSourcesDetailsOrStats(): ArrayList<SelfossModel.Source> {
var sources = ArrayList<SelfossModel.Source>()
val isDatabaseEnabled =
appSettingsService.isItemCachingEnabled() || !appSettingsService.isUpdateSourcesEnabled()
AmineL marked this conversation as resolved
Review

All this is overly complicated.

All this is overly complicated.
return if (isNetworkAvailable() && !fetchedSources) {
val apiSources = api.sources()
if (apiSources.success && apiSources.data != null && isDatabaseEnabled) {
resetDBSourcesWithData(apiSources.data)
if (!appSettingsService.isUpdateSourcesEnabled()) {
val shouldFetch = if (!appSettingsService.isUpdateSourcesEnabled()) !fetchedSources else true
if (shouldFetch && isNetworkAvailable()) {
if (appSettingsService.getPublicAccess()) {
val apiSources = api.sourcesStats()
if (apiSources.success && apiSources.data != null) {
fetchedSources = true
sources = apiSources.data as ArrayList<SelfossModel.Source>
}
} else {
sources = getSourcesDetails() as ArrayList<SelfossModel.Source>
}
} else if (isDatabaseEnabled) {
sources = getDBSources().map { it.toView() } as ArrayList<SelfossModel.Source>
}
return sources
}
suspend fun getSourcesDetails(): ArrayList<SelfossModel.SourceDetail> {
var sources = ArrayList<SelfossModel.SourceDetail>()
val isDatabaseEnabled =
appSettingsService.isItemCachingEnabled() || !appSettingsService.isUpdateSourcesEnabled()
val shouldFetch = if (!appSettingsService.isUpdateSourcesEnabled()) !fetchedSources else true
if (shouldFetch && isNetworkAvailable()) {
val apiSources = api.sourcesDetailed()
if (apiSources.success && apiSources.data != null) {
fetchedSources = true
sources = apiSources.data
AmineL marked this conversation as resolved
Review

We only update the DB from getSourcesDetails

We only update the DB from `getSourcesDetails`
if (isDatabaseEnabled) {
resetDBSourcesWithData(sources)
}
}
apiSources.data ?: ArrayList()
} else if (isDatabaseEnabled) {
ArrayList(getDBSources().map { it.toView() })
} else {
ArrayList()
sources = getDBSources().map { it.toView() } as ArrayList<SelfossModel.SourceDetail>
}
return sources
}
suspend fun markAsRead(item: SelfossModel.Item): Boolean {
@ -482,7 +505,7 @@ class Repository(
}
}
private fun resetDBSourcesWithData(sources: List<SelfossModel.Source>) {
private fun resetDBSourcesWithData(sources: List<SelfossModel.SourceDetail>) {
db.sourcesQueries.deleteAllSources()
db.sourcesQueries.transaction {
@ -592,7 +615,7 @@ class Repository(
ReaderForSelfossDB.Schema.migrate(driverFactory.createDriver(), 0, 1)
}
fun setSelectedSource(source: SelfossModel.Source) {
fun setSelectedSource(source: SelfossModel.SourceDetail) {
_selectedSource = source
}
@ -600,7 +623,7 @@ class Repository(
_selectedSource = null
}
fun getSelectedSource(): SelfossModel.Source? {
fun getSelectedSource(): SelfossModel.SourceDetail? {
return _selectedSource
}
}

View File

@ -183,7 +183,15 @@ class SelfossApi(private val appSettingsService: AppSettingsService) {
}
})
suspend fun sources(): StatusAndData<ArrayList<SelfossModel.Source>> =
suspend fun sourcesStats(): StatusAndData<ArrayList<SelfossModel.SourceStats>> =
bodyOrFailure(client.tryToGet(url("/sources/stats")) {
if (!shouldHavePostLogin()) {
parameter("username", appSettingsService.getUserName())
parameter("password", appSettingsService.getPassword())
}
})
suspend fun sourcesDetailed(): StatusAndData<ArrayList<SelfossModel.SourceDetail>> =
bodyOrFailure(client.tryToGet(url("/sources/list")) {
if (!shouldHavePostLogin()) {
parameter("username", appSettingsService.getUserName())

View File

@ -12,24 +12,25 @@ fun TAG.toView(): SelfossModel.Tag =
this.unread.toInt()
)
fun SOURCE.toView(): SelfossModel.Source =
SelfossModel.Source(
fun SOURCE.toView(): SelfossModel.SourceDetail =
SelfossModel.SourceDetail(
this.id.toInt(),
this.title,
this.tags.split(","),
null,
this.tags?.split(","),
this.spout,
this.error,
this.icon,
if (this.url != null) SelfossModel.SourceParams(this.url) else null
)
fun SelfossModel.Source.toEntity(): SOURCE =
fun SelfossModel.SourceDetail.toEntity(): SOURCE =
SOURCE(
this.id.toString(),
this.title.getHtmlDecoded(),
this.tags.joinToString(","),
this.spout,
this.error,
this.tags?.joinToString(",").orEmpty(),
this.spout.orEmpty(),
this.error.orEmpty(),
this.icon.orEmpty(),
this.params?.url
)