You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by daserge <gi...@git.apache.org> on 2015/09/03 15:31:01 UTC
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
GitHub user daserge opened a pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38
CB-9365 Add support for 'vibrateWithPattern' to Windows Phone 8.1 / W…
…indows 10
[Jira issue](https://issues.apache.org/jira/browse/CB-9365)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-plugin-vibration CB-9365
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-vibration/pull/38.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #38
----
commit 7dc4c8e22e6bdcc9c3385b3060a698909b2d3760
Author: daserge <v-...@microsoft.com>
Date: 2015-09-03T13:25:33Z
CB-9365 Add support for 'vibrateWithPattern' to Windows Phone 8.1 / Windows 10
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38659073
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
--- End diff --
Can we factor out args[0] to a more meaningful variable name?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38669919
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
+ args[0].shift(0);
+ }
+
+ patternChainPromise = args[0].reduce(function (previousValue, currentValue, index) {
+ if (index % 2 === 0) {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete, error) {
+ module.exports.vibrate(function () {}, function(err) {
+ console.error(err);
+ error(err);
+ }, [currentValue]);
+
+ setTimeout(function () {
+ complete();
+ }, parseInt(currentValue, 10));
+ });
+ });
+ } else {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete) {
+ setTimeout(function () {
+ complete();
+ }, parseInt(currentValue, 10));
+ });
+ });
+ }
+ }, WinJS.Promise.as());
+ }, fail);
},
cancelVibration: function(success, fail, args) {
try {
+ patternChainPromise && patternChainPromise.cancel();
--- End diff --
If you're currently vibrating, will the vibration itself also be cancelled?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38724321
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
+ args[0].shift(0);
+ }
+
+ patternChainPromise = args[0].reduce(function (previousValue, currentValue, index) {
+ if (index % 2 === 0) {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete, error) {
+ module.exports.vibrate(function () {}, function(err) {
+ console.error(err);
+ error(err);
+ }, [currentValue]);
+
+ setTimeout(function () {
+ complete();
+ }, parseInt(currentValue, 10));
+ });
+ });
+ } else {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete) {
+ setTimeout(function () {
+ complete();
+ }, parseInt(currentValue, 10));
+ });
+ });
+ }
+ }, WinJS.Promise.as());
+ }, fail);
},
cancelVibration: function(success, fail, args) {
try {
+ patternChainPromise && patternChainPromise.cancel();
--- End diff --
Yes, it will be cancelled by the next line:
```javascript
VibrationDevice.getDefault().cancel();
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38670056
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
+ args[0].shift(0);
+ }
+
+ patternChainPromise = args[0].reduce(function (previousValue, currentValue, index) {
+ if (index % 2 === 0) {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete, error) {
+ module.exports.vibrate(function () {}, function(err) {
+ console.error(err);
+ error(err);
+ }, [currentValue]);
--- End diff --
We make a lot of references to `currentValue`. Can we just parse it once and save the integer form? Maybe we can opportunistically parse all of them on input and if any parse return NaN then we fail fast.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#issuecomment-137694325
@nikhilkh, @robpaveza thanks for review!
Please take a look once more - I have addressed your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38669813
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
+ args[0].shift(0);
+ }
+
+ patternChainPromise = args[0].reduce(function (previousValue, currentValue, index) {
+ if (index % 2 === 0) {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete, error) {
+ module.exports.vibrate(function () {}, function(err) {
+ console.error(err);
+ error(err);
+ }, [currentValue]);
+
+ setTimeout(function () {
+ complete();
+ }, parseInt(currentValue, 10));
+ });
+ });
+ } else {
+ return previousValue.then(function () {
+ return new WinJS.Promise(function (complete) {
--- End diff --
Prefer `WinJS.Promise.timeout(parseInt(currentValue, 10))` to `new WinJS.Promise( ... setTimeout(...))`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38659317
--- Diff: README.md ---
@@ -113,7 +113,9 @@ Vibrates the device with a given pattern
####Windows Quirks
-- vibrate(pattern) falls back on vibrate with default duration
+- `repeat` parameter is not supported
--- End diff --
Why is repeat not supported?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#discussion_r38659458
--- Diff: src/windows/VibrationProxy.js ---
@@ -60,12 +61,43 @@ if (VibrationDevice) {
fail(e);
}
},
- vibrateWithPattern: function(success, fail, args) {
- // TODO: Implement with setTimeout.
- fail('"vibrateWithPattern" is unsupported by this platform.');
+ vibrateWithPattern: function (success, fail, args) {
+ // Cancel current vibrations first
+ module.exports.cancelVibration(function () {
+ // Removes the first zero
+ if (args[0] && args[0][0] === 0) {
+ args[0].shift(0);
+ }
+
+ patternChainPromise = args[0].reduce(function (previousValue, currentValue, index) {
--- End diff --
Do we need to test for presence of args[0]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-plugin-vibration/pull/38#issuecomment-137486956
Should any mobilespec tests be updated now that this is supported for Windows? Or is mobilespec not testing this because its not supported on emulators?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-vibration pull request: CB-9365 Add support for 'vi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-vibration/pull/38
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org