Provide haptic/acoustic feedback for tap & long-press on Android #10920

goderbauer · 2017-06-23T01:00:44Z

Hixie · 2017-06-23T01:02:14Z

packages/flutter/lib/src/material/feedback.dart

+  }
+
+  static TargetPlatform _platform(BuildContext context) => Theme.of(context).platform;
+


nit: extraneous blank line

Hixie · 2017-06-23T01:02:34Z

packages/flutter/lib/src/material/feedback.dart

+
+  /// Provides platform-specific feedback for a tap.
+  static Future<Null> forTap(BuildContext context) async {
+    switch(_platform(context)) {


nit: space after switch

Hixie · 2017-06-23T01:02:38Z

packages/flutter/lib/src/material/feedback.dart

+
+  /// Provides platform-specific feedback for a long press.
+  static Future<Null> forLongPress(BuildContext context) {
+    switch(_platform(context)) {


Hixie · 2017-06-23T01:05:15Z

packages/flutter/lib/src/material/feedback.dart

+  FeedbackWrapper._();
+
+  /// Provides platform-specific feedback for a tap.
+  static GestureTapCallback forTap(GestureTapCallback callback, BuildContext context) {


This is really quite clever.

Thanks 😄

Hixie · 2017-06-23T01:06:04Z

packages/flutter/lib/src/material/feedback.dart

+        return new Future<Null>.value();
+    }
+  }
+


Can we merge these classes and have Feedback.forTap(context) and Feedback.wrapForTap(callback, context) ?

Having two classes with two static methods seems like it will lead to confusion.

Good idea. Done.

Hixie · 2017-06-23T01:08:30Z

packages/flutter/lib/src/material/feedback.dart

+import 'package:flutter/services.dart';
+import 'package:flutter/widgets.dart';
+
+/// Provides platform-specific acoustic and/or haptic feedback for certain


These docs need to be more elaborate.

Things to add at each place there are docs in this file (it's perfectly ok to have duplication in the docs):

What does this do on Android?

What does this do on iOS?

How do you handle things like onPressed: enabled ? callback : null?

Sample code? (with the ## Sample code pattern, see other files)

Hixie · 2017-06-23T01:09:08Z

packages/flutter/lib/src/material/ink_well.dart

@@ -93,7 +94,8 @@ class InkResponse extends StatefulWidget {
    this.borderRadius: BorderRadius.zero,
    this.highlightColor,
    this.splashColor,
-  }) : super(key: key);
+    bool enableFeedback,
+  }) : enableFeedback = enableFeedback ?? true, super(key: key);


Why not just make true the default value for the argument?

(and then assert that it's not null, and mention in the docs for the constructor that it must be non-null)

The idea was that this way I don't have to specify the default value in two places (here and below for InkWell). Changed it nonetheless as I didn't have strong feelings.

Hixie · 2017-06-23T01:10:16Z

packages/flutter/lib/src/material/ink_well.dart

    _currentSplash?.confirm();
    _currentSplash = null;
    if (widget.onLongPress != null)
+      if (widget.enableFeedback)
+        Feedback.forLongPress(context);
      widget.onLongPress();


you missed the braces here

please add a test that would have failed here

Oh! Good catch!

I still wish ifs would always come with braces... goto fail; 😉

As to writing a test: there is actually another onLongPress != null check that guards the entire execution of this method (see line 359). So, it's actually impossible to exploit the missing braces in a test here.

In that case maybe we should just assert(widget.onLongPress != null) and remove the test?

Hixie · 2017-06-23T01:12:20Z

packages/flutter/lib/src/material/ink_well.dart

@@ -392,6 +405,7 @@ class InkWell extends InkResponse {
    Color highlightColor,
    Color splashColor,
    BorderRadius borderRadius,
+    bool enableFeedback,


default value

constructor docs update

Hixie · 2017-06-23T01:13:24Z

packages/flutter/lib/src/material/tooltip.dart

    if (_entry != null) {
      _timer?.cancel();
      _timer = null;
      _controller.forward();
-      return;  // Already visible.
+      return false;  // Already visible.


nit: extraneous space before comment

Hixie · 2017-06-23T01:14:39Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -650,3 +656,6 @@ class _Editable extends LeafRenderObjectWidget {
    return new TextSpan(style: style, text: text);
  }
 }
+
+/// Signature for the callback that reports when the user changes the selection.
+typedef void SelectionChangedCallback(TextSelection selection, bool longPress);


put typedefs near the top of the file, before first use if possible

Hixie · 2017-06-23T01:15:15Z

packages/flutter/lib/src/material/text_field.dart

@@ -243,6 +249,7 @@ class _TextFieldState extends State<TextField> {
        selectionControls: materialTextSelectionControls,
        onChanged: widget.onChanged,
        onSubmitted: widget.onSubmitted,
+        onSelectionChanged: (TextSelection _, bool longPress) => _onSelectionChanged(context, longPress),


interesting, i would have expected this to belong to the selection controls delegate. This works too.

Hixie · 2017-06-23T01:16:15Z

This is fantastic.

Hixie · 2017-06-23T18:50:54Z

packages/flutter/lib/src/material/feedback.dart

+/// tapped, call [forTap]. For the Android-specific vibration when long pressing
+/// an element, call [forLongPress]. Alternatively, you can also wrap your
+/// [onTap] or [onLongPress] callback in [wrapForTap] or [wrapForLongPress] to
+/// achive the same (see example code below).


Hixie · 2017-06-23T18:51:23Z

packages/flutter/lib/src/material/feedback.dart

+/// typically don't provide haptic or acoustic feedback.
+///
+/// All methods in this class are usually called from within a [build] method
+/// as you have to provide a [BuildContext].


or from a State's methods

Hixie · 2017-06-23T19:01:37Z

packages/flutter/lib/src/material/feedback.dart

+/// To trigger platform-specific feedback before executing the actual callback:
+///
+/// ```dart
+/// @override


Ideally each example would be something you can copy and paste into code and have work right away, so here we would usually include the StatefulWidget and State boilerplate, as in:
https://master-docs-flutter-io.firebaseapp.com/flutter/painting/TextSpan/recognizer.html

Hixie · 2017-06-23T19:04:29Z

packages/flutter/lib/src/material/ink_well.dart

@@ -93,7 +94,8 @@ class InkResponse extends StatefulWidget {
    this.borderRadius: BorderRadius.zero,
    this.highlightColor,
    this.splashColor,
-  }) : super(key: key);
+    this.enableFeedback: true,
+  }) : assert(enableFeedback != null), super(key: key);


the doc for the constructor should mention that containedInkWell, borderRadius, and enableFeedback must not be null, and the asserts should include the other two as well.

Asserts are throwing left and right when I assert on containedInkWell and borderRadius. If the assert is really true we should do that (and the associated clean-up) in a follow-up PR.

Hixie · 2017-06-23T19:04:51Z

packages/flutter/lib/src/material/ink_well.dart

+  /// Whether detected gestures should provide acoustic and/or haptic feedback.
+  ///
+  /// For example, on Android a tap will produce a clicking sound and a
+  /// long-press will produce a short vibration, when feedback is enabled.


maybe point to Feedback?

Hixie · 2017-06-23T19:05:52Z

packages/flutter/lib/src/material/tooltip.dart

@@ -110,12 +111,13 @@ class _TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
      _removeEntry();
  }

-  void ensureTooltipVisible() {
+  /// Returns `false` when the tooltip was already visible.
+  bool ensureTooltipVisible() {


this doesn't say the most important part, that it shows a tooltip! :-)

Hixie · 2017-06-23T19:06:23Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -226,6 +230,9 @@ class EditableText extends StatefulWidget {
  /// Called when the user indicates that they are done editing the text in the field.
  final ValueChanged<String> onSubmitted;

+  /// Called when the selection of text changed.


or when the cursor has moved? or not?

Hixie · 2017-06-23T19:07:22Z

packages/flutter/test/material/feedback_tester.dart

+
+import 'package:flutter/services.dart';
+
+/// Tracks how often feedback has been requested since its instantiation.


This should probably mention that only one of these can be used at a time.

Hixie · 2017-06-23T19:07:39Z

packages/flutter/test/material/feedback_tester.dart

+
+  /// Number of times the click sound was requested to play.
+  int get clickSoundCount => _clickSoundCount;
+  int _clickSoundCount = 0;


Maybe we should add a dispose() method than unhooks the mock handler?

or don't bother, it's only used in tests, so...

I added it for completeness.

Hixie · 2017-06-23T19:09:27Z

…tter#10920) * Provide haptic/acoustic feedback for tap & long-press on Android * review comments * fixed example code * review comments * comment fix

Provide haptic/acoustic feedback for tap & long-press on Android

e306787

googlebot added the cla: yes label Jun 23, 2017

goderbauer requested a review from jason-simmons June 23, 2017 01:00

Hixie reviewed Jun 23, 2017

View reviewed changes

goderbauer added 2 commits June 23, 2017 10:50

review comments

d63a585

fixed example code

ebd7ef8

Hixie reviewed Jun 23, 2017

View reviewed changes

goderbauer added 2 commits June 23, 2017 14:23

review comments

68c7fab

Merge branch 'master' into androidFeedback2

065c174

comment fix

480c37d

goderbauer merged commit fe40eed into flutter:master Jun 23, 2017

goderbauer deleted the androidFeedback2 branch June 23, 2017 22:02

goderbauer added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Oct 17, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021

		}

		static TargetPlatform _platform(BuildContext context) => Theme.of(context).platform;


		import 'package:flutter/services.dart';

		/// Tracks how often feedback has been requested since its instantiation.

Provide haptic/acoustic feedback for tap & long-press on Android #10920

Provide haptic/acoustic feedback for tap & long-press on Android #10920

Uh oh!

Conversation

goderbauer commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 23, 2017

Uh oh!

Hixie Jun 23, 2017 •

edited

Loading

Hixie Jun 23, 2017 •

edited

Loading