Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement bookmarks #944

Merged
merged 34 commits into from
Sep 12, 2023
Merged

Implement bookmarks #944

merged 34 commits into from
Sep 12, 2023

Conversation

ZacSweers
Copy link
Owner

@ZacSweers ZacSweers commented Sep 10, 2023

Resolves #102

Some highlights

  • Using a nested Circuit screen in a list item for the bookmarks UI
  • Separate composite bookmarks DB
  • A little more compose KMP work
  • New Bookmarks screen with some swipe to dismiss
Screen_recording_20230910_011546.mp4

@ZacSweers
Copy link
Owner Author

I should add an export feature

}

internal class BookmarkRepositoryImpl(private val database: CatchUpDatabase) : BookmarkRepository {
private val scope = CoroutineScope(Dispatchers.IO)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inject this. Constructing this scope internally bakes in the assumption that the class is singleton-y; not doing this is a major reason we use the DI pattern and DI frameworks like Dagger.

Also, don't inject as a val. See comments below about launches on instance methods. Launches on initialization are fine IMO, and don't require making the scope a val, either.

}

override fun addBookmark(id: Long, timestamp: Instant) {
scope.launch { database.transaction { database.bookmarksQueries.addBookmark(id, timestamp) } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API probably wants to be a suspend fun! In many scenarios the caller can and should safely own this, and a suspend fun still opens the door for kicking it over to a longer lived scope if the caller really does need that

If you feel strongly opinionated that this is the right API shape, then scope.launch still isn't the right choice. Use a channel instead:

data class AddBookmark(val id: Long, val timestamp: Instant)

private val addBookmarks = Channel<AddBookmark>(BUFFERED)

init {
  scope.launch {
    for (item in addBookmarks) {
      database.transaction { database.bookmarksQueries.addBookmark(item.id, item.timestamp) }
    }
  }
}

override fun addBookmark(id: Long, timestamp: Instant) {
  addBookmarks.sendOrFail(AddBookMark(id, timestamp))
}

You might say, "I don't need a queue! Why did you make me build a queue?" But you already had a queue: it was the dispatcher, and its failure mode was "my dispatcher is full of hung bookmark add operations".

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern about making it suspend is that I don't fully understand what that communicates to the caller. What happens if they cancel the add? Is that something the UI gets to really decide? Does the add() always work but the UI gets to know if it successfully handed it off (synchronously) to the internal transaction handler?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • When the suspend fun write completes successfully, the caller knows the write succeeded
  • If the write fails, the caller will get an exception (which is good)
  • If the caller cancels the addBookmark call before it completes (e.g. by navigating away), then the write will get discarded
  • If the caller wants to navigate away and not cancel the write, they can still accomplish that by doing exactly what your existing code here does

All of these are valid choices, which is why I tend to prefer suspend funs for this type of thing.

}

override fun removeBookmark(id: Long) {
scope.launch { database.transaction { database.bookmarksQueries.removeBookmark(id) } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@ZacSweers
Copy link
Owner Author

Thanks @jingibus!

@ZacSweers ZacSweers merged commit 5661c86 into main Sep 12, 2023
@ZacSweers ZacSweers deleted the z/bookmarks branch September 12, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature to bookmark articles
2 participants