Skip to content

Commit ff495c3

Browse files
spacecowboyspacecowboy
committed
Fixed mark as read on scroll also marking items when opening items
1 parent a0c4a8b commit ff495c3

File tree

3 files changed

+158
-65
lines changed

3 files changed

+158
-65
lines changed

‎app/src/androidTest/java/com/nononsenseapps/feeder/ui/compose/FeedScreenMarkAsReadOnScrollTest.kt

+118-23
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package com.nononsenseapps.feeder.ui.compose
22

33
import androidx.compose.ui.geometry.Offset
44
import androidx.compose.ui.test.ExperimentalTestApi
5+
import androidx.compose.ui.test.click
56
import androidx.compose.ui.test.hasTestTag
67
import androidx.compose.ui.test.junit4.createAndroidComposeRule
78
import androidx.compose.ui.test.onNodeWithTag
@@ -11,7 +12,6 @@ import com.nononsenseapps.feeder.archmodel.Repository
1112
import com.nononsenseapps.feeder.archmodel.SyncFrequency
1213
import com.nononsenseapps.feeder.db.room.Feed
1314
import com.nononsenseapps.feeder.db.room.FeedItem
14-
import com.nononsenseapps.feeder.db.room.ID_ALL_FEEDS
1515
import com.nononsenseapps.feeder.ui.MainActivity
1616
import kotlinx.coroutines.delay
1717
import kotlinx.coroutines.flow.first
@@ -24,6 +24,7 @@ import java.net.URL
2424
import java.time.Instant
2525
import java.time.ZoneOffset
2626
import java.time.ZonedDateTime
27+
import kotlin.test.assertEquals
2728
import kotlin.test.assertTrue
2829

2930
class FeedScreenMarkAsReadOnScrollTest : BaseComposeTest {
@@ -39,26 +40,32 @@ class FeedScreenMarkAsReadOnScrollTest : BaseComposeTest {
3940
repository.setSyncFrequency(SyncFrequency.MANUAL)
4041

4142
// Ensure we have feeds and items
42-
runBlocking {
43-
val feedId =
44-
repository.saveFeed(
45-
Feed(
46-
title = "Ampersands are & the worst",
47-
url = URL("https://example.com/ampersands"),
48-
),
49-
)
50-
repository.upsertFeedItems(
51-
(1..50).map { i ->
52-
FeedItem(
53-
guid = "guid$i",
54-
title = "Item $i",
55-
feedId = feedId,
56-
pubDate = ZonedDateTime.now(ZoneOffset.UTC).plusDays(i.toLong()),
57-
primarySortTime = Instant.now().plusSeconds(i.toLong()),
58-
) to ""
59-
},
60-
) { _, _ -> }
61-
}
43+
val feedId =
44+
runBlocking {
45+
val feedId =
46+
repository.saveFeed(
47+
Feed(
48+
title = "Ampersands are & the worst",
49+
url = URL("https://example.com/ampersands"),
50+
),
51+
)
52+
53+
repository.setCurrentFeedAndTag(feedId, "")
54+
55+
repository.upsertFeedItems(
56+
(1..50).map { i ->
57+
FeedItem(
58+
guid = "guid$i",
59+
title = "Item $i",
60+
feedId = feedId,
61+
pubDate = ZonedDateTime.now(ZoneOffset.UTC).plusDays(i.toLong()),
62+
primarySortTime = Instant.now().plusSeconds(i.toLong()),
63+
) to ""
64+
},
65+
) { _, _ -> }
66+
67+
feedId
68+
}
6269

6370
composeTestRule.waitUntilExactlyOneExists(
6471
hasTestTag("feed_list"),
@@ -67,7 +74,7 @@ class FeedScreenMarkAsReadOnScrollTest : BaseComposeTest {
6774

6875
val unreadCountBefore: Int =
6976
runBlocking {
70-
repository.getUnreadCount(ID_ALL_FEEDS).first()
77+
repository.getUnreadCount(feedId).first()
7178
}
7279

7380
assertTrue("There should be unread items before test: $unreadCountBefore") {
@@ -84,13 +91,101 @@ class FeedScreenMarkAsReadOnScrollTest : BaseComposeTest {
8491
swipe(start, end, durationMillis = 10_000)
8592
}
8693

94+
composeTestRule.waitForIdle()
95+
8796
runBlocking {
8897
withTimeout(5_000L) {
89-
while (repository.getUnreadCount(ID_ALL_FEEDS).first() == unreadCountBefore) {
98+
while (repository.getUnreadCount(feedId).first() == unreadCountBefore) {
9099
composeTestRule.waitForIdle()
91100
delay(100L)
92101
}
93102
}
94103
}
95104
}
105+
106+
@OptIn(ExperimentalTestApi::class)
107+
@Test
108+
fun markAsReadOnScrollOnlyHappensOnScroll() {
109+
val repository by (composeTestRule.activity).di.instance<Repository>()
110+
111+
repository.setIsMarkAsReadOnScroll(true)
112+
repository.setSyncFrequency(SyncFrequency.MANUAL)
113+
114+
// Ensure we have feeds and items
115+
val feedId =
116+
runBlocking {
117+
val feedId =
118+
repository.saveFeed(
119+
Feed(
120+
title = "Ampersands are & the worst",
121+
url = URL("https://example.com/ampersands"),
122+
),
123+
)
124+
125+
repository.setCurrentFeedAndTag(feedId, "")
126+
127+
repository.upsertFeedItems(
128+
(1..50).map { i ->
129+
FeedItem(
130+
guid = "guid$i",
131+
title = "Item $i",
132+
feedId = feedId,
133+
pubDate = ZonedDateTime.now(ZoneOffset.UTC).plusDays(i.toLong()),
134+
primarySortTime = Instant.now().plusSeconds(i.toLong()),
135+
) to ""
136+
},
137+
) { _, _ -> }
138+
139+
feedId
140+
}
141+
142+
composeTestRule.waitUntilExactlyOneExists(
143+
hasTestTag("feed_list"),
144+
10_000L,
145+
)
146+
147+
val unreadCountBefore: Int =
148+
runBlocking {
149+
repository.getUnreadCount(feedId).first()
150+
}
151+
152+
assertTrue("There should be unread items before test: $unreadCountBefore") {
153+
unreadCountBefore > 0
154+
}
155+
156+
// Make sure items are on screen a while before click. We do that with a slow swipe to the side
157+
composeTestRule
158+
.onNodeWithTag("feed_list")
159+
.performTouchInput {
160+
val start = Offset(centerX, top)
161+
val end = Offset(-50f, top)
162+
swipe(start, end, durationMillis = 10_000)
163+
}
164+
165+
composeTestRule.waitForIdle()
166+
167+
// Click first item
168+
composeTestRule
169+
.onNodeWithTag("feed_list")
170+
.performTouchInput {
171+
click(Offset(centerX, top + 100f))
172+
}
173+
174+
composeTestRule.waitUntilExactlyOneExists(
175+
hasTestTag("readerColumn"),
176+
10_000L,
177+
)
178+
179+
composeTestRule.waitForIdle()
180+
181+
runBlocking {
182+
// Delay to ensure background tasks - if any - complete
183+
delay(500L)
184+
assertEquals(
185+
unreadCountBefore - 1,
186+
repository.getUnreadCount(feedId).first(),
187+
"Should have marked as read on click but no more",
188+
)
189+
}
190+
}
96191
}

‎app/src/main/java/com/nononsenseapps/feeder/ui/compose/feed/FeedScreen.kt

+34-41
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ import com.nononsenseapps.feeder.ApplicationCoroutineScope
106106
import com.nononsenseapps.feeder.R
107107
import com.nononsenseapps.feeder.archmodel.FeedItemStyle
108108
import com.nononsenseapps.feeder.archmodel.FeedType
109+
import com.nononsenseapps.feeder.db.FAR_FUTURE
109110
import com.nononsenseapps.feeder.db.room.FeedItemCursor
110111
import com.nononsenseapps.feeder.db.room.ID_SAVED_ARTICLES
111112
import com.nononsenseapps.feeder.db.room.ID_UNSET
@@ -142,12 +143,11 @@ import com.nononsenseapps.feeder.ui.compose.utils.ImmutableHolder
142143
import com.nononsenseapps.feeder.ui.compose.utils.addMargin
143144
import com.nononsenseapps.feeder.ui.compose.utils.isCompactDevice
144145
import com.nononsenseapps.feeder.ui.compose.utils.onKeyEventLikeEscape
145-
import com.nononsenseapps.feeder.ui.compose.utils.rememberApplicationCoroutineScope
146146
import com.nononsenseapps.feeder.ui.compose.utils.rememberIsItemMostlyVisible
147-
import com.nononsenseapps.feeder.ui.compose.utils.rememberIsItemVisible
148147
import com.nononsenseapps.feeder.util.ActivityLauncher
149148
import com.nononsenseapps.feeder.util.ToastMaker
150149
import com.nononsenseapps.feeder.util.emailBugReportIntent
150+
import kotlinx.coroutines.CoroutineScope
151151
import kotlinx.coroutines.Dispatchers
152152
import kotlinx.coroutines.delay
153153
import kotlinx.coroutines.launch
@@ -1135,18 +1135,15 @@ fun FeedListContent(
11351135
val previewItem = pagedFeedItems[itemIndex] ?: PLACEHOLDER_ITEM
11361136

11371137
if (viewState.markAsReadOnScroll && previewItem.unread) {
1138-
val visible: Boolean by listState.rememberIsItemVisible(
1139-
key = previewItem.id,
1140-
)
11411138
val mostlyVisible: Boolean by listState.rememberIsItemMostlyVisible(
11421139
key = previewItem.id,
11431140
screenHeightPx = screenHeightPx,
11441141
)
11451142
MarkItemAsReadOnScroll(
11461143
itemId = previewItem.id,
1147-
visible = visible,
11481144
mostlyVisible = mostlyVisible,
11491145
currentFeedOrTag = viewState.currentFeedOrTag,
1146+
coroutineScope = coroutineScope,
11501147
markAsRead = markAsUnread,
11511148
)
11521149
}
@@ -1335,18 +1332,15 @@ fun FeedGridContent(
13351332
// Very important that items don't change size or disappear when scrolling
13361333
// Placeholder will have no id
13371334
if (previewItem.id > ID_UNSET && viewState.markAsReadOnScroll && previewItem.unread) {
1338-
val visible: Boolean by gridState.rememberIsItemVisible(
1339-
key = previewItem.id,
1340-
)
13411335
val mostlyVisible: Boolean by gridState.rememberIsItemMostlyVisible(
13421336
key = previewItem.id,
13431337
screenHeightPx = screenHeightPx,
13441338
)
13451339
MarkItemAsReadOnScroll(
13461340
itemId = previewItem.id,
1347-
visible = visible,
13481341
mostlyVisible = mostlyVisible,
13491342
currentFeedOrTag = viewState.currentFeedOrTag,
1343+
coroutineScope = coroutineScope,
13501344
markAsRead = markAsUnread,
13511345
)
13521346
}
@@ -1445,57 +1439,56 @@ fun <T : Any> LazyPagingItems<T>.rememberLazyListState(): LazyListState {
14451439

14461440
/**
14471441
* @param itemId id of item to mark as read
1448-
* @param visible if item is visible at all
14491442
* @param mostlyVisible if item is mostly visible
14501443
* @param currentFeedOrTag current feed or tag at the time of display
1444+
* @param coroutineScope a scope which will be cancelled when navigated away from feed screen
14511445
* @param markAsRead action to run
14521446
*/
14531447
@Composable
14541448
fun MarkItemAsReadOnScroll(
14551449
itemId: Long,
1456-
visible: Boolean,
14571450
mostlyVisible: Boolean,
14581451
currentFeedOrTag: FeedOrTag,
1452+
coroutineScope: CoroutineScope,
14591453
markAsRead: (Long, Boolean, FeedOrTag?) -> Unit,
14601454
) {
1461-
val coroutineScope = rememberApplicationCoroutineScope()
1462-
1463-
var debounced by remember {
1464-
mutableStateOf(false)
1455+
var visibleTime by remember {
1456+
mutableStateOf(FAR_FUTURE)
14651457
}
1466-
if (mostlyVisible) {
1467-
LaunchedEffect(null) {
1468-
delay(800)
1469-
debounced = true
1458+
LaunchedEffect(mostlyVisible) {
1459+
if (mostlyVisible && visibleTime == FAR_FUTURE) {
1460+
visibleTime = Instant.now()
14701461
}
14711462
}
1472-
if (visible) {
1473-
DisposableEffect(null) {
1474-
onDispose {
1475-
if (debounced) {
1476-
coroutineScope.launch(Dispatchers.IO) {
1477-
// Why Coroutine? Why a delay?
1478-
// Because this scope will be cancelled if the screen
1479-
// is navigated away from and I only want things to be marked
1480-
// during scroll - not during navigation.
1481-
// Navigating between feeds is a special case, which is
1482-
// why the currentFeedOrTag needs to be passed to the
1483-
// view model.
1484-
@Suppress("UNNECESSARYVARIABLE")
1485-
val feedOrTag = currentFeedOrTag
1486-
delay(100)
1487-
markAsRead(
1488-
itemId,
1489-
false,
1490-
feedOrTag,
1491-
)
1492-
}
1463+
1464+
DisposableEffect(visibleTime) {
1465+
onDispose {
1466+
// Check time BEFORE delaying action
1467+
if (visibleTime.isBefore(Instant.now().minusMillis(REQUIRED_VISIBLE_TIME_FOR_MARK_AS_READ))) {
1468+
coroutineScope.launch(Dispatchers.IO) {
1469+
// Why Coroutine? Why a delay?
1470+
// Because this scope will be cancelled if the screen
1471+
// is navigated away from and I only want things to be marked
1472+
// during scroll - not during navigation.
1473+
// Navigating between feeds is a special case, which is
1474+
// why the currentFeedOrTag needs to be passed to the
1475+
// view model.
1476+
@Suppress("UNNECESSARYVARIABLE")
1477+
val feedOrTag = currentFeedOrTag
1478+
delay(100)
1479+
markAsRead(
1480+
itemId,
1481+
false,
1482+
feedOrTag,
1483+
)
14931484
}
14941485
}
14951486
}
14961487
}
14971488
}
14981489

1490+
private const val REQUIRED_VISIBLE_TIME_FOR_MARK_AS_READ = 500L
1491+
14991492
private val PLACEHOLDER_ITEM =
15001493
FeedListItem(
15011494
id = ID_UNSET,

‎app/src/main/java/com/nononsenseapps/feeder/ui/compose/feedarticle/ReaderView.kt

+6-1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import androidx.compose.ui.semantics.clearAndSetSemantics
4747
import androidx.compose.ui.semantics.contentDescription
4848
import androidx.compose.ui.semantics.customActions
4949
import androidx.compose.ui.semantics.semantics
50+
import androidx.compose.ui.semantics.testTag
5051
import androidx.compose.ui.tooling.preview.Preview
5152
import androidx.compose.ui.unit.dp
5253
import coil.compose.AsyncImage
@@ -59,6 +60,7 @@ import com.nononsenseapps.feeder.archmodel.isImage
5960
import com.nononsenseapps.feeder.model.MediaImage
6061
import com.nononsenseapps.feeder.model.ThumbnailImage
6162
import com.nononsenseapps.feeder.ui.compose.coil.rememberTintedVectorPainter
63+
import com.nononsenseapps.feeder.ui.compose.components.safeSemantics
6264
import com.nononsenseapps.feeder.ui.compose.text.WithBidiDeterminedLayoutDirection
6365
import com.nononsenseapps.feeder.ui.compose.text.WithTooltipIfNotBlank
6466
import com.nononsenseapps.feeder.ui.compose.text.rememberMaxImageWidth
@@ -120,7 +122,10 @@ fun ReaderView(
120122
modifier =
121123
modifier
122124
.fillMaxWidth()
123-
.focusGroup(),
125+
.focusGroup()
126+
.safeSemantics {
127+
testTag = "readerColumn"
128+
},
124129
) {
125130
item {
126131
val goToFeedLabel = stringResource(R.string.go_to_feed, feedTitle)

0 commit comments

Comments
 (0)