You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by TimBarham <gi...@git.apache.org> on 2015/07/29 02:28:02 UTC
[GitHub] cordova-plugin-device-motion pull request: CB-9426 Fix exception u...
GitHub user TimBarham opened a pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33
CB-9426 Fix exception using device motion plugin on browser platform
The plugin's `plugin.xml` defines a general `<js-module>` with the name `accelerometer`. It defines another `<js-module>` for the `browser` platform with the same name. Modules with the same name are not allowed, so Cordova throws an exception at runtime when trying to define the browser version of the module.
Fix is to rename the browser version of the module `browser-accelerometer`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-plugin-device-motion CB-9426
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-device-motion/pull/33.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 #33
----
commit 15ab61a5b15487fc79e657f973fd5a8bcac0cdbf
Author: Tim Barham <ti...@microsoft.com>
Date: 2015-07-29T00:19:43Z
CB-9426 Fix exception when using device motion plugin on browser platform.
The plugin's plugin.xml defines a general <js-module> with the name 'accelerometer'. It defines another <js-module> for the browser platform (that merges with the general module) with the same name. Modules with the same name is not allowed, so Cordova throws an exception at runtime when trying to define the browser version of the module.
Fix is to rename the browser module 'browser-accelerometer'.
----
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125792526
LGTM!
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-device-motion/pull/33
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-126129030
I agree. My goal here however was just to fix the exception, not change the behavior of a plugin I'm not familiar with :smile:... So what's the consensus - should I merge this change now, or just leave it pendinging @purplecabbage's rewrite (same question applies to the `device-orientation` change as well).
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125805744
Ok, I've deleted the browser specific implementation and moved the browser specific logic into the general implementation, and also added some logic to avoid multiple event timers being triggered.
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125955137
The PR LGTM.
However, IMO the code that emits `devicemotion` events might be removed due to:
a) this is not a scope of plugin to provide stubs for browser functionality
b) the `devicemotion` event is already supported by most of the browsers (http://caniuse.com/#search=devicemotion).
@purplecabbage, what do you think?
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125975741
Yeah. I was still going to look at a rewrite in the near future. No need to fake data...
> On Jul 29, 2015, at 6:38 AM, Vladimir Kotikov <no...@github.com> wrote:
>
> The PR LGTM.
>
> However, IMO the code that emits devicemotion events might be removed due to:
> a) this is not a scope of plugin to provide stubs for browser functionality
> b) the devicemotion event is already supported by most of the browsers (http://caniuse.com/#search=devicemotion).
>
> @purplecabbage, what do you think?
>
> —
> Reply to this email directly or view it on GitHub.
>
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125794448
Yeah, I have just been wondering the same thing. The only `browser` specific thing is that it starts firing the `devicemotion` event when you call `watchAcceleration()` (and turns it off when you call `clearWatch()`) - that could easily be included in the general `accelerometer.js`.
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125791006
@stevengill and @purplecabbage - interested in whether this fix looks right to you guys, since I've not worked in this code much before. Certainly after my change the functionality seems correct - the `browser` specific module is a merge, with the same target that the general module clobbers. Without my change, we don't get the merge. With my change, I verified we do.
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-125793282
Actually, on second thought ...
I don't think there is much reason for the browser implementation to have it's own version of accelerometer.js This file is virtually identical to the one at www/accelerometer.js so maybe it is time for a quick refactor.
Your understanding of merges and uniqueness of name are correct though. ;)
---
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-device-motion pull request: CB-9426 Fix exception u...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-plugin-device-motion/pull/33#issuecomment-126136377
Yeah. Merge as is to side step the error.
I'll create jira tickets and revisit later.
---
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