-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Search with Favorites widget #1321
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
Conversation
- widget manifest - configuration activity - widget class
- search bar with clickable intent - grid of items for favorites (using WidgetService)
…are the same values
…ixes bug of widget using wrong adapter)
| private val favoritesDao: FavoritesDao, | ||
| private val faviconManager: Lazy<FaviconManager>, | ||
| ) : FavoritesRepository { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove this change
| fun inject(searchAndFavsWidget: SearchAndFavoritesWidget) | ||
|
|
||
| fun inject(favoritesWidgetItemFactory: FavoritesWidgetService.FavoritesWidgetItemFactory) | ||
|
|
||
| fun inject(emptyFavoritesWidgetItemFactory: EmptyFavoritesWidgetService.EmptyFavoritesWidgetItemFactory) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these? For instance for the SearchAndFavoritesWidget why can't we do the injection in onReceive?
BTW I think we also don't need the fun inject(searchWidget: SearchWidget)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aitorvs and I talked about this, and try directly using AndroidInjector, however it will not work without providing an AndroidInjector.Factory in the same way we do for each Activity. We decided to keep it as it is since we will remove dagger-android soon.
|
|
||
| @OnLifecycleEvent(Lifecycle.Event.ON_START) | ||
| fun notifyWidgets() { | ||
| GlobalScope.launch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove GlobalScope
CDRussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking awesome! 🎉
| } else null | ||
| } | ||
|
|
||
| override suspend fun loadFromDiskWithParams(tabId: String?, url: String, radius: Int, width: Int, height: Int): Bitmap? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| override suspend fun loadFromDiskWithParams(tabId: String?, url: String, radius: Int, width: Int, height: Int): Bitmap? { | |
| override suspend fun loadFromDiskWithParams(tabId: String?, url: String, cornerRadius: Int, width: Int, height: Int): Bitmap? { |
| } | ||
|
|
||
| private fun launchedFromSearchWithFavsWidget(intent: Intent): Boolean { | ||
| return intent.getBooleanExtra(WIDGET_SEARCH_WITH_FAVS_EXTRA, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a pixel
| return EmptyFavoritesWidgetItemFactory(this.applicationContext, intent) | ||
| } | ||
|
|
||
| class EmptyFavoritesWidgetItemFactory(val context: Context, intent: Intent) : RemoteViewsFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment around why this is needed
| super.onDeleted(context, appWidgetIds) | ||
| } | ||
|
|
||
| private fun updateWidget(context: Context, appWidgetManager: AppWidgetManager, appWidgetId: Int, newOptions: Bundle?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pull out smaller functions from this?
| var columns = calculateColumns(context, width) | ||
| var rows = calculateRows(context, height) | ||
|
|
||
| columns = if (columns < 2) 2 else columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use coerce here too for consistency with rows
| return Pair(columns, rows) | ||
| } | ||
|
|
||
| private fun calculateColumns(context: Context, width: Int): Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could likely extract calculateColumns and calculateRows to a utility class which could be unit tested
| <string name="widgetConfigurationAddWidgetOption">Add widget</string> | ||
| <string name="searchWidgetEmtpyFavoritesHint">No favorites added yet</string> | ||
| <string name="searchWidgetEmtpyFavoritesCta">Add Favorite</string> | ||
| <string name="daxFavoritesOnboardingCtaText"><![CDATA[Visit your favorite sites in a flash!<br/><br/>Go to a site you love. Then tap the \"⋯\" icon and select Add Favorite.]]></string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try the unicode character for the overflow (will also requiring manually setting the accessibility text)
| <dimen name="searchWidgetSearchBarSideMargin">16dp</dimen> | ||
| <dimen name="searchWidgetFavoriteItemContainerWidth">64dp</dimen> | ||
| <dimen name="searchWidgetFavoriteItemContainerHeight">78dp</dimen> | ||
| <dimen name="searchWidget4colWidth">280dp</dimen> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4col probably not still valid
…s values on favorites grid
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt
CDRussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looking good! added some tiny optional suggestions but don't mind about any of them
| info.text = v?.context?.getString(R.string.daxFavoritesOnboardingCtaContentDescription) | ||
| } | ||
| } | ||
| // Using braille unicode inside textview, override description for accessibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth clarifying why we are doing that... e.g., using it to show something that looks like the overflow icon
| @OnLifecycleEvent(Lifecycle.Event.ON_START) | ||
| fun notifyWidgets() { | ||
| GlobalScope.launch { | ||
| appCoroutineScope.launch() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't checked in an IDE, but could probably remove the () i think?
| rows = 1.coerceAtLeast(rows) | ||
| rows = 4.coerceAtMost(rows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 and 4 might read slightly better if they had constants explaining what they were (reads like magic numbers currently)
- replace magic numbers with constant values
| var portraitWidth = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MIN_WIDTH) | ||
|
|
||
| if (newOptions != null) { | ||
| portraitWidth = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MIN_WIDTH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it bug? Check newOptions is null and set same value for portraitWidth
| portraitWidth = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MIN_WIDTH) | ||
| landsWidth = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MAX_WIDTH) | ||
| landsHeight = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MIN_HEIGHT) | ||
| portraitHeight = appWidgetOptions.getInt(AppWidgetManager.OPTION_APPWIDGET_MAX_HEIGHT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it bug? Check newOptions is null and set same value for variables?
Task/Issue URL: https://app.asana.com/0/488551667048375/1200651343654716/f
Tech Design URL:
CC:
Description:
This PR introduces a new widget for Android. The new widget will show user's favorites and the search bar.
The widget empty case includes a CTA to take the user through a small onboarding about how to add favorites.
We've also updated the UI for the existing widgets.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template