Add 'Share' button to the selection toolbar on Android #139479
Conversation
github-actions
bot
added
a: text input
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #139479 at sha 74b42edd47d343f7473357703e7bc719b6f9100e |
|
FYI, the golden change for |
bleroux
force-pushed
the
add_android_share_option_to_selection_toolbar
branch
from
74b42ed to
3e9b284
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #139479 at sha 3e9b284837cc6cfafcfc33c4abb599cccd6013f4 |
There was a problem hiding this comment.
That is interesting, was this test working with no changes before this PR? I don't see why this PR would cause that to change.
There was a problem hiding this comment.
Yes, I can confirm that this test is working on master without this change.
With this PR, I just found that another solution to make this test green is to add a call to pumpAndSettle() on line 692, so there is probably an animation going on.
Do you think it is better to keep this TODO or to replace it with the call to pumpAndSettle? There are dozens of pumpAndSettle in this test file, so I think it makes sense to use one here, the remaining question is why was it not necesseray before? (it might be related to the toolbar content being longer than before).
There was a problem hiding this comment.
Adding a pumpAndSettle sounds reasonable. I tried the pr out locally and found that if I change the logic in bool get shareEnabled to the code below the test passes. I'm not at all sure why this works. I think we should we keep the switch statement and add a pumpAndSettle since we might support other platforms.
if (defaultTargetPlatform != TargetPlatform.iOS || defaultTargetPlatform != TargetPlatform.android) {
return false;
}
return !widget.obscureText
&& !textEditingValue.selection.isCollapsed
&& textEditingValue.selection.textInside(textEditingValue.text).trim() != '';There was a problem hiding this comment.
nit: consider moving expect(lastShare, 'Test'); out of this if statement since it is common to both branches.
There was a problem hiding this comment.
Is this planned for this PR, or in the future?, same for below.
There was a problem hiding this comment.
I plan to work on this later because it is not a blocker for this PR and it requires some careful thinking because:
- it should be implemented correctly for all platforms.
- It might make the helper functions in the 'utils' file less readable (and I wanted to keep them as readable as possible for this review and for the next PR I'm working on, the one related to [Android] Respect custom system-wide text selection toolbar buttons #139361).
|
Thank you for the contribution @bleroux! These improvements to tests are awesome! I just had a few small comments, but this look great! |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #139479 at sha 075f9dc2111606f4feb4df8c91334b21f1894dc3 |
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM after the pumpAndSettle is added. Thanks for the contribution @bleroux
bleroux
force-pushed
the
add_android_share_option_to_selection_toolbar
branch
from
075f9dc to
a29f89a
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…tter#139738) ## Description This PR adds custom system-wide text selection toolbar buttons on Android. ~~This is a WIP until flutter#139479 is merged (potential conflicts).~~ ## Related Issue Fixes flutter#139361 ## Tests Adds 5 tests.
## Description This PR adds the share button to text selection toolbar buttons on Android ~~and iOS~~ for `SelectableRegion` (and therefore `SelectionArea`). #139479 adds this button for `EditableText` (which is used by `TextField` and `SelectableText` but not by `SelectionArea`). **Edit**: supporting this on iOS will need more work (see #141447 (comment) and #141775). ## Related Issue Follow up for #138728 ## Tests Adds 1 test.
2 participants
Description
This PR adds the 'Share' button to the text selection toolbar on Android.
Related Issue
Fixes #138728
Tests
Refactor a lot of existing tests in order to:
For instance, previous tests contained sections such as:
Where the comment is obsolete, the two cases (6 widgets and 3 widgets) are not explicit (which buttons are expected?), and not accurate (will pass if the number of buttons is right but the buttons are the wrong ones).