[ios] Implement AppBar Assistant button states [chromium/src : main]
Chris Lu (Gerrit)
Chris Lu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 9 comments![]( "Open in Gerrit")
id<BWGCommands> bwgHandler =```suggestion
id<BWGCommands> geminiHandler =
```
(it should be renamed at some point)
bwgService:BwgServiceFactory::GetForProfile(profile)```suggestion
geminiService:BwgServiceFactory::GetForProfile(profile)
```
if (_identityManager) {You can probably remove this check
- (void)updateAssistantButton {Add comment
[self.consumer setAssistantButtonState:state avatar:avatar];How do you plan to handle the action from the button? Is it the VC that will always call the right command? Should it go through the mediator?
_identityManager = nullptr;Shouldn't you stop observation here?
AppBarAssistantButtonStateIneligible,What is this state? Is it signed in + inelligible? I think it maybe should be more clear. Also, is this an enum for the UI states only or is it also an enum for the different actions (for example signed out and signed in with Gemini enabled is the same UI but different action)?
typedef NS_ENUM(NSUInteger, AppBarAssistantButtonState) {Use enum class
Related details
- Adam Arcaro
- Chris Lu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro voted and added 3 comments![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
3 comments
BwgService change lgtm, thanks for the improvement!
id<BWGCommands> bwgHandler =```suggestion
id<BWGCommands> geminiHandler =
```(it should be renamed at some point)
We're planning to rename it soon (it's a heavy rename that touches a lot of files), but +1 to using `gemini` instead of `bwg` in local vars in the meantime
@property(nonatomic, weak) id<BWGCommands> bwgHandler;nit: geminiHandler
Related details
- Chris Lu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Lu (Gerrit)
Chris Lu added 9 comments![]( "Open in Gerrit")
Adam Arcaro```suggestion
id<BWGCommands> geminiHandler =
```(it should be renamed at some point)
We're planning to rename it soon (it's a heavy rename that touches a lot of files), but +1 to using `gemini` instead of `bwg` in local vars in the meantime
Done
bwgService:BwgServiceFactory::GetForProfile(profile)Chris Lu```suggestion
geminiService:BwgServiceFactory::GetForProfile(profile)
```
Done
You can probably remove this check
Done
- (void)updateAssistantButton {Chris LuAdd comment
Done
[self.consumer setAssistantButtonState:state avatar:avatar];How do you plan to handle the action from the button? Is it the VC that will always call the right command? Should it go through the mediator?
I was thinking we go through the mediator, since there seems to be a few different behaviors that could show the same icon. I don't mind just having one mutator API and the mediator decides the exact action.
Shouldn't you stop observation here?
Yes
AppBarAssistantButtonStateIneligible,What is this state? Is it signed in + inelligible? I think it maybe should be more clear. Also, is this an enum for the UI states only or is it also an enum for the different actions (for example signed out and signed in with Gemini enabled is the same UI but different action)?
Yes that is correct, intended to be UI state. However, reflecting more, it should definitely be more aligned with UI-focused state. "eligible" will be determined later after the tap since we may show the ask button even if the assistant is not available.
typedef NS_ENUM(NSUInteger, AppBarAssistantButtonState) {Chris LuUse enum class
Done
@property(nonatomic, weak) id<BWGCommands> bwgHandler;Chris Lunit: geminiHandler
Done
Related details
- Adam Arcaro
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Code-Review | +1 |
3 comments
[self.consumer setAssistantButtonState:state avatar:avatar];Chris LuHow do you plan to handle the action from the button? Is it the VC that will always call the right command? Should it go through the mediator?
I was thinking we go through the mediator, since there seems to be a few different behaviors that could show the same icon. I don't mind just having one mutator API and the mediator decides the exact action.
Acknowledged
AppBarAssistantButtonStateIneligible,Chris LuWhat is this state? Is it signed in + inelligible? I think it maybe should be more clear. Also, is this an enum for the UI states only or is it also an enum for the different actions (for example signed out and signed in with Gemini enabled is the same UI but different action)?
Yes that is correct, intended to be UI state. However, reflecting more, it should definitely be more aligned with UI-focused state. "eligible" will be determined later after the tap since we may show the ask button even if the assistant is not available.
Yeah I agree. I think it makes sense for the mediator to push the UI state + info relevant for the state.
image = _assistantButtonAvatar;Are we expecting the mediator to always pass an avatar (including the default "no avatar" image)? Or can this be nil in which case the VC should handle it?
Related details
- Adam Arcaro
- Chris Lu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
1 comment
base::ScopedObservation<signin::IdentityManager,nit: Comment for consistency
Related details
- Chris Lu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Lu (Gerrit)
Chris Lu added 2 comments![]( "Open in Gerrit")
AppBarAssistantButtonStateIneligible,Chris LuWhat is this state? Is it signed in + inelligible? I think it maybe should be more clear. Also, is this an enum for the UI states only or is it also an enum for the different actions (for example signed out and signed in with Gemini enabled is the same UI but different action)?
Gauthier AmbardYes that is correct, intended to be UI state. However, reflecting more, it should definitely be more aligned with UI-focused state. "eligible" will be determined later after the tap since we may show the ask button even if the assistant is not available.
Yeah I agree. I think it makes sense for the mediator to push the UI state + info relevant for the state.
Acknowledged
Are we expecting the mediator to always pass an avatar (including the default "no avatar" image)? Or can this be nil in which case the VC should handle it?
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Lu (Gerrit)
Chris Lu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Lu
| Commit-Queue | +2 |
1 comment
base::ScopedObservation<signin::IdentityManager,Chris Lunit: Comment for consistency
sorry i didnt see this initially... but now looking through the codebase we don't really leave comments for ScopedObservation properties.. so going to avoid the extra CQ dry run and just let this slide :P
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/app_bar/coordinator/app_bar_mediator.mm
Insertions: 0, Deletions: 2.
@@ -719,8 +719,6 @@
if (avatarProvider) {
avatar = avatarProvider->GetIdentityAvatar(
identity, IdentityAvatarSize::TableViewIcon);
- } else {
- avatar = DefaultSymbolTemplateWithPointSize(kPersonCropCircleSymbol, 30);
}
}
```
```
The name of the file: ios/chrome/browser/app_bar/ui/app_bar_view_controller.mm
Insertions: 2, Deletions: 1.
@@ -284,7 +284,8 @@
UIButtonConfiguration* configuration = _assistantButton.configuration;
configuration.title = title;
- configuration.image = image;
+ configuration.image =
+ image ? image : DefaultAppBarSymbol(kPersonCropCircleSymbol);
_assistantButton.configuration = configuration;
}
```
Change information
[ios] Implement AppBar Assistant button states
Supports three states of the Assistant button represented by
AppBarAssistantButtonState:
- ask: show the assistant icon, shown if feature is enabled and locale requirements are met
- account: show the user's profile icon, shown if signed in
- signed out: show the signed out icon
video: https://screencast.googleplex.com/cast/NDY3NTk2ODgwMjIyNjE3NnxiOGVkMTM5NC05OQ
https://screenshot.googleplex.com/49aehKDnWNwXGnB
- M ios/chrome/browser/app_bar/coordinator/BUILD.gn
- M ios/chrome/browser/app_bar/coordinator/DEPS
- M ios/chrome/browser/app_bar/coordinator/app_bar_coordinator.mm
- M ios/chrome/browser/app_bar/coordinator/app_bar_coordinator_unittest.mm
- M ios/chrome/browser/app_bar/coordinator/app_bar_mediator.h
- M ios/chrome/browser/app_bar/coordinator/app_bar_mediator.mm
- M ios/chrome/browser/app_bar/coordinator/app_bar_mediator_unittest.mm
- M ios/chrome/browser/app_bar/ui/app_bar_constants.h
- M ios/chrome/browser/app_bar/ui/app_bar_constants.mm
- M ios/chrome/browser/app_bar/ui/app_bar_consumer.h
- M ios/chrome/browser/app_bar/ui/app_bar_view_controller.mm
- M ios/chrome/browser/intelligence/bwg/model/bwg_service.h
- M ios/chrome/browser/intelligence/bwg/model/bwg_service.mm
Code-Review: +1 by Gauthier Ambard, +1 by Adam Arcaro
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |