feat: Introduce sharpnesses parameter for detailed iOS haptic feedb…#147
feat: Introduce sharpnesses parameter for detailed iOS haptic feedb…#147yeop-sang wants to merge 3 commits into
sharpnesses parameter for detailed iOS haptic feedb…#147Conversation
…ack and refine `sharpness` default behavior.
There was a problem hiding this comment.
Pull request overview
This PR extends the vibration plugin API to support per-step iOS CoreHaptics sharpness by introducing a sharpnesses list parameter, enabling more detailed haptic patterns.
Changes:
- Add
sharpnessesto the platform interface and method-channel payload forvibrate(). - Implement iOS CoreHaptics event sharpness mapping (with scalar fallback behavior).
- Update Flutter-side tests and Android argument parsing to accommodate the new argument.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| vibration_platform_interface/lib/vibration_platform_interface.dart | Extends vibrate() signature with sharpnesses and changes default sharpness. |
| vibration_platform_interface/lib/src/method_channel_vibration.dart | Forwards sharpnesses via method channel on vibrate(). |
| vibration/lib/vibration.dart | Exposes sharpnesses in the public Dart API and forwards to platform. |
| vibration/ios/vibration/Sources/vibration/VibrationPlugin.swift | Adds iOS CoreHaptics logic to apply sharpness per pattern element. |
| vibration/android/src/main/java/com/benjaminabel/vibration/VibrationMethodChannelHandler.java | Reads new sharpness args (currently ignored) to align with new payload. |
| vibration/test/vibration_test.dart | Updates existing method-channel argument expectations to include sharpness changes. |
| vibration/pubspec.yaml | Switches vibration_platform_interface dependency from hosted to Git ref. |
| vibration/example/ios/Podfile | Adds a Podfile for the example iOS project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Future<void> vibrate({ | ||
| int duration = 500, | ||
| List<int> pattern = const [], | ||
| int repeat = -1, | ||
| List<int> intensities = const [], | ||
| int amplitude = -1, | ||
| double sharpness = 0.5, | ||
| double sharpness = -1.0, | ||
| List<double> sharpnesses = const [], |
There was a problem hiding this comment.
Adding the new named parameter sharpnesses changes the vibrate() override signature. At least one in-repo platform implementation (vibration_ohos/lib/vibration_ohos.dart) currently overrides vibrate() without this parameter, which will cause an invalid override / compilation failure. Update all VibrationPlatform implementers to include the new parameter (even if ignored) so the repo builds.
| vibration_platform_interface: | ||
| git: | ||
| url: https://github.com/yeop-sang/flutter_vibration.git | ||
| path: vibration_platform_interface | ||
| ref: main |
There was a problem hiding this comment.
vibration is a versioned package (version: 3.1.5) but it now depends on vibration_platform_interface via a Git reference (ref: main). This is not reproducible and will generally prevent publishing to pub.dev (Git dependencies are disallowed for published packages). Prefer bumping vibration_platform_interface's version and using a hosted constraint, or use a local path:/dependency_overrides for development only.
| vibration_platform_interface: | |
| git: | |
| url: https://github.com/yeop-sang/flutter_vibration.git | |
| path: vibration_platform_interface | |
| ref: main | |
| vibration_platform_interface: ^3.1.0 |
| /// Returns sharpness value. If sharpness is not explicitly provided (indicated by -1.0), | ||
| /// it calculates sharpness as intensity * 0.5 for backward compatibility. | ||
| private func getSharpness(myArgs: [String: Any], intensity: Float) -> Float { | ||
| let sharpness = myArgs["sharpness"] as? Float ?? -1.0 | ||
| if sharpness < 0 { | ||
| // If sharpness not provided, use intensity * 0.5 as default | ||
| return intensity * 0.5 | ||
| } | ||
| return sharpness | ||
| } |
There was a problem hiding this comment.
getSharpness(...) returns the provided sharpness as-is when it is >= 0, but CoreHaptics expects .hapticSharpness values in the 0.0–1.0 range. Passing values outside that range (or NaN) can cause pattern creation/playback to fail and be silently swallowed by the catch block. Clamp/validate the computed and user-provided sharpness to [0, 1] before constructing CHHapticEventParameter.
| 'sharpnesses': [], | ||
| }) | ||
| ], | ||
| ); |
There was a problem hiding this comment.
There’s no test asserting that a non-empty sharpnesses list is forwarded over the method channel (only the default empty list case is covered). Add a unit test that calls Vibration.vibrate(..., sharpnesses: [...]) and verifies the MethodCall arguments include that list to prevent regressions.
| let pattern = getPattern(myArgs: myArgs) | ||
| let intensity = Float(getAmplitude(myArgs: myArgs)) / 255.0 | ||
|
|
||
| if pattern.count == 1 { |
There was a problem hiding this comment.
getSharpnesses() ignores a caller-provided sharpnesses list when pattern.count == 1 (it always returns the scalar getSharpness(...) result). If a user calls Vibration.vibrate(duration: ...) with sharpnesses: [x], the value will be dropped. Consider using the first list element when provided, and only falling back to the scalar/default when the list is empty.
| if pattern.count == 1 { | |
| if pattern.count == 1 { | |
| // For a single pattern element, prefer a caller-provided sharpness if available, | |
| // and fall back to the default scalar sharpness otherwise. | |
| if let firstSharpness = sharpnesses.first { | |
| return [Float(firstSharpness)] | |
| } |
| // sharpness is iOS only. -1.0 means not provided (will use intensity * 0.5) | ||
| double sharpness = -1.0, |
There was a problem hiding this comment.
The Dart API now uses sharpness = -1.0 as a sentinel for “not provided”, which changes behavior for existing callers who previously relied on the default sharpness: 0.5 (especially when specifying amplitude/intensities). To avoid this implicit behavior change and keep the range contract (0.0–1.0), consider making sharpness nullable (double? sharpness) and omitting the method-channel key when null, with native code treating a missing key as “not provided”.
| // sharpness is iOS only. -1.0 means not provided (will use intensity * 0.5) | |
| double sharpness = -1.0, | |
| // sharpness is iOS only. null means not provided (will use intensity * 0.5) | |
| double? sharpness, |
| // sharpness is iOS only. Ignored on Android. | ||
| Double sharpness = call.argument("sharpness"); | ||
| List<Double> sharpnesses = call.argument("sharpnesses"); |
There was a problem hiding this comment.
The newly added sharpness / sharpnesses locals are read from the MethodCall but never used. If the goal is only to document/accept the arguments, consider removing the unused variables (extra arguments do not break call.argument(...) parsing) or explicitly suppressing the warning, to keep the handler clean.
| // sharpness is iOS only. Ignored on Android. | |
| Double sharpness = call.argument("sharpness"); | |
| List<Double> sharpnesses = call.argument("sharpnesses"); | |
| // sharpness / sharpnesses are iOS-only parameters and are ignored on Android. |
| // 1. If sharpnesses list is available and has index, use it. | ||
| // 2. Fallback to scalar getSharpness logic. | ||
| let sharpness: Float | ||
| if !sharpnesses.isEmpty && i < sharpnesses.count { | ||
| sharpness = sharpnesses[i] |
There was a problem hiding this comment.
sharpnesses[i] from the list is used directly when present. Since these values bypass getSharpness(...), they also bypass any defaulting/validation. Consider clamping list-provided values to [0, 1] (and handling NaN) before creating CHHapticEventParameter(parameterID: .hapticSharpness, ...) to avoid CoreHaptics failures.
| // 1. If sharpnesses list is available and has index, use it. | |
| // 2. Fallback to scalar getSharpness logic. | |
| let sharpness: Float | |
| if !sharpnesses.isEmpty && i < sharpnesses.count { | |
| sharpness = sharpnesses[i] | |
| // 1. If sharpnesses list is available and has index, use it (with validation/clamping). | |
| // 2. Fallback to scalar getSharpness logic. | |
| let sharpness: Float | |
| if !sharpnesses.isEmpty && i < sharpnesses.count { | |
| let rawSharpness = sharpnesses[i] | |
| if rawSharpness.isNaN || !rawSharpness.isFinite { | |
| // Fallback to scalar logic if the provided value is invalid | |
| sharpness = getSharpness(myArgs: myArgs, intensity: normalizedIntensity) | |
| } else { | |
| // Clamp provided value to [0, 1] to satisfy CoreHaptics requirements | |
| sharpness = max(0.0, min(1.0, rawSharpness)) | |
| } |
| }) { | ||
| throw UnimplementedError('deviceInfo() has not been implemented.'); | ||
| } |
There was a problem hiding this comment.
The platform-interface vibrate() default implementation throws UnimplementedError('deviceInfo() has not been implemented.'), which is misleading for callers debugging missing platform implementations. Update the message to reference vibrate() (and ideally align messages across the interface).
- make sharpness nullable in the Dart API and omit the channel key when absent - sync platform interface and OHOS override signatures with sharpnesses support - sanitize and clamp iOS sharpness values with per-segment fallback logic - remove unused Android sharpness argument parsing - add payload tests for explicit sharpness and sharpnesses lists - replace git dependency with hosted platform interface constraints and bump versions - document and script local monorepo verification using temporary pubspec overrides
|
Addressed the Copilot review feedback in the latest update. Changes included:
Verification:
This runs:
The verification script creates temporary local overrides when needed and removes them |
| @@ -0,0 +1,43 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Do we really need this kind of verification? Seems a bit redundant since the packages will fail to install if something is wrong.
There was a problem hiding this comment.
My appologies.
While incorporating the Copilot review feedback, I added verify_local.sh as part of my local validation process and accidentally committed it.
I'll revert just that file.
Thank you for your comment.
Are there any comment or feedback you'd like to share?
There was a problem hiding this comment.
Besides merge conflicts - all good!
benjamindean
left a comment
There was a problem hiding this comment.
Thanks for the contribution and sorry for the late reply! Left one minor comment.
Problem
Currently, the
vibratemethod supports aList<int> intensitiesparameter to vary intensity across a vibration pattern, butsharpnessis limited to a singledoublevalue applied to the entire pattern.Solution
Implemented a
List<double> sharpnessesparameter to thevibratemethod.Key Changes
List<double> sharpnessestoVibrationPlatform.vibrateandMethodChannelVibration.sharpnesseselements to eachCHHapticEvent.intensitieswhich uses 1-255 (Android scale) and is normalized internally.VibrationMethodChannelHandlerto prevent crashes.VibrationEffectlogic works differently.sharpnessesis empty/null -> Fallback to singlesharpness.sharpnessis -1.0 -> Fallback tointensity * 0.5.Usage Example
Parameter Range Note
intensities/amplitude: 1 - 255 (Follows Android standard, normalized to 0.0-1.0 on iOS).sharpnesses/sharpness: 0.0 - 1.0 (Follows iOS CoreHaptics standard).