Skip to content
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

Migrate LogicalKeySet to SingleActivator #80756

Merged
merged 8 commits into from Apr 26, 2021

Conversation

dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Apr 19, 2021

This PR follows #78522, which introduced SingleActivator and ShortcutActivator, and applies the change to existing code, by replacing all occurrences of LogicalKeySet to SingleActivator or ShortcutActivator.

This is part of the general keyboard project as in #44918, and resolves the shortcut problems described in #78522 in existing components.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Apr 19, 2021
@google-cla google-cla bot added the cla: yes label Apr 19, 2021
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

You might change the PR title to say "LogicalKeySet" instead of "LogicalActionSet"...

@dkwingsmt dkwingsmt changed the title Migrate LogicalActionSet to SingleActivator Migrate LogicalKeySet to SingleActivator Apr 23, 2021
@dkwingsmt dkwingsmt merged commit b8833af into flutter:master Apr 26, 2021
@dkwingsmt dkwingsmt deleted the migrate-single-shortcuts branch April 26, 2021 21:54
@tvolkert
Copy link
Contributor

Even though this didn't break any customer tests, you should probably write a migration guide and list this as a breaking change.

The fact that several tests needed to be updated is an indication that this is likely to affect apps that use shortcuts. It did mine 🙂

tvolkert added a commit to tvolkert/chicago that referenced this pull request Apr 30, 2021
@dkwingsmt dkwingsmt added the a: text input Entering text in a text field or keyboard related problems label Jul 29, 2021
@tvolkert tvolkert added the c: API break Backwards-incompatible API changes label Aug 1, 2021
@tvolkert
Copy link
Contributor

tvolkert commented Aug 1, 2021

@dkwingsmt did you write a migration guide for this?

@dkwingsmt
Copy link
Contributor Author

@tvolkert I'll do it next week. Thanks for reminder.

@dkwingsmt
Copy link
Contributor Author

@tvolkert Sorry to have responded so late. I've tried the tests that got changed in this PR, but all of them still compile. What breakage did you encounter? The only thing I can think of is if you wanted to trigger a system shortcut in a deprecated fashion (e.g. trigger Ctrl-C by pressing KeyC first then Control, or by holding Control and Shift then Key C). Just want to confirm if there are breakages I missed.

@tvolkert
Copy link
Contributor

Here's my code that needed updating:

final Iterable<LogicalKeyboardKey> activateKeys = WidgetsApp.defaultShortcuts.entries
  .where((MapEntry<LogicalKeySet, Intent> entry) => entry.value is ActivateIntent)
  .map<LogicalKeySet>((MapEntry<LogicalKeySet, Intent> entry) => entry.key)
  .where((LogicalKeySet keySet) => keySet.keys.length == 1)
  .map<LogicalKeyboardKey>((LogicalKeySet keySet) => keySet.keys.single);

Again, since no client tests broke, this isn't technically a breaking change according to our policy, but since we had to update so many tests in this PR, it seemed like a good indication that clients may also have to be updated, in which case I think a migration guide may help them.

@Piinks
Copy link
Contributor

Piinks commented Sep 7, 2021

@dkwingsmt was a migration guide made for this?

@dkwingsmt
Copy link
Contributor Author

No I didn't. Was about to make but was stalled by the flutter/website rework. Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems c: API break Backwards-incompatible API changes f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants