Skip to content

Conversation

@GiacomoPignoni
Copy link

@GiacomoPignoni GiacomoPignoni commented Jun 23, 2024

Add the localizedSubtitle field on quick actions for iOS

#129759

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla
Copy link

google-cla bot commented Jun 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jmagman
Copy link
Member

jmagman commented Jun 24, 2024

I like this idea, though what you currently have doesn't compile because the quick_actions_platform_interface doesn't have that parameter. Check out https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-platform-interface-method-parameters

 lib/main.dart:56:9: Error: No named parameter with the name 'localizedSubtitle'.
         localizedSubtitle: 'Action one subtitle',
         ^^^^^^^^^^^^^^^^^
 ../../../../../.pub-cache/hosted/pub.dev/quick_actions_platform_interface-1.0.6/lib/types/shortcut_item.dart:11:9: Context: Found this candidate, but the arguments don't match.
   const ShortcutItem({

Also quick_actions_ios will need its CHANGELOG updated. Take a look at that wiki page for more details about how to contribute to packages.
Thank you!

@GiacomoPignoni
Copy link
Author

Ok sorry @jmagman, I've added the deps override (as indicated in the documentation), the quick_actions_ios CHANGELOG and also the quick_actions_platform_interface CHANGELOG.

Now I wait for the review, than I'll make a new PR to update only the platform interface

@jmagman
Copy link
Member

jmagman commented Jun 26, 2024

Swift format:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8744034268588324081/+/u/Run_package_tests/Swift_format/stdout

Are you able to reproduce this test timeout locally?

12:00 +0 -1: loading /Volumes/Work/s/w/ir/x/w/packages/packages/quick_actions/quick_actions/example/integration_test/quick_actions_test.dart [E]   

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8744034268487478385/+/u/Run_package_tests/drive_examples/stdout

@@ -1,5 +1,6 @@
## NEXT

* Add localizedSubtitle field for iOS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see the CHANGELOG style guide linked from the PR checklist.

flutter:
uses-material-design: true

# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert all of the quick_actions_android override entries since the package won't need any changes.

@@ -1,3 +1,7 @@
## NEXT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see the versioning policy linked in from the PR checklist.

@@ -1,5 +1,6 @@
## NEXT
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, please see the versioning policy.


/// Localized subtitle of the item.
///
/// This is ignored on Android, since it's not supported.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Platform interface packages should not make claims about what implementation packages, which are a higher level, do or don't do. This should just say that it's ignored by platforms that don't support it.

quickActions.setShortcutItems(<ShortcutItem>[
const ShortcutItem(type: 'action_main', localizedTitle: 'Main view', icon: 'icon_main'),
const ShortcutItem(type: 'action_help', localizedTitle: 'Help', icon: 'icon_help')
const ShortcutItem(type: 'action_help', localizedTitle: 'Help', localizedSubtitle: 'Tap to get help', icon: 'icon_help')
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me that this is a common enough option that we actually need to update the README and example app to demonstrate it.


let type = shortcut.type
let localizedTitle = shortcut.localizedTitle
let localizedSubtitle = shortcut.localizedSubtitle
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should just be inlined below, as it doesn't seem like the local variables are adding any value here. (The ones above may well be left over from a direct Pigeon conversion, as they would have been more useful prior to Pigeon.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should have corresponding native testing.

@stuartmorgan-g
Copy link
Collaborator

@GiacomoPignoni Are you still planning on updating this PR per the review feedback above?

@stuartmorgan-g
Copy link
Collaborator

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!

@sinyu1012
Copy link
Contributor

I have created a new one based on this PR, please help to view it. @stuartmorgan

#8038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants