add FavIconPresenter to handle favicon retrieval and caching #1482
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors favicon handling for RSS feeds and account tabs by introducing a new FavIconPresenter with database caching and improving the favicon detection logic in RssService. The changes replace direct favicon URL generation with a more robust system that fetches, validates, and caches favicons.
- Introduces
FavIconPresenterfor fetching and caching favicons using the database - Enhances
RssService.fetchIconto intelligently select the best available icon by size and handle various URL formats - Replaces
IconType.UrlwithIconType.FavIconfor better type safety and integration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| FavIconPresenter.kt | New presenter that fetches and caches favicons using the database |
| RssService.kt | Improved favicon detection logic with better icon selection and URL handling |
| EmojiContent.kt | Added FavIcon sealed interface member for database caching |
| Rss.kt | Replaced tryRun with standard runCatching for consistency |
| TabSettings.kt | Added IconType.FavIcon and updated HomeTimelineTabItem constructor |
| EditTabPresenter.kt | Updated to use new IconType.FavIcon instead of IconType.Url |
| HomeTimelineWithTabsPresenter.kt | Removed direct favicon URL generation, now handled by presenter |
| TabIcon.kt | Added rendering support for IconType.FavIcon with loading state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is Feed.RDF -> null | ||
| is Feed.Rss20 -> null | ||
| else -> null | ||
| null -> null |
There was a problem hiding this comment.
The null -> null branch is redundant since this is already the default behavior of a when expression that exhausts all sealed interface members. Consider using else -> null instead, or remove the explicit null handling if the sealed interface is exhaustive.
| null -> null |
| tryRun { | ||
| ktorClient( | ||
| config = {}, | ||
| ).get(url).bodyAsText() | ||
| }.getOrNull() ?: return null |
There was a problem hiding this comment.
[nitpick] Inconsistent use of tryRun vs runCatching. The file Rss.kt was updated to use standard Kotlin runCatching, but this file still uses the custom tryRun function. For consistency across the codebase, consider using runCatching here as well, similar to the changes in Rss.kt.
No description provided.