512237 - Device Orientation issues

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Doug Turner (:dougt)]

+++ This bug was initially created as a clone of Bug #511902 +++

Created an attachment (id=395856)
patch v.1 (cocoa widget implementation)

Implementation for cocoa.


Smaug made these suggestions (which probably can apply to all implementations)



* AccelerationChanged shouldn't be called if acceleration hasn't changed.

* Currently it seems that mUpdateTimer just runs every 200ms.  That should be 50ms or something to get smoother animations.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
> * AccelerationChanged shouldn't be called if acceleration hasn't changed.
But when an event listener is added, there should be MozOrientation event soon.

 
> * Currently it seems that mUpdateTimer just runs every 200ms.  That should be
> 50ms or something to get smoother animations.
As the XXX comment in the code says, there could be a pref for this.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2

We may want to push the filtering into the platform specific code, but for now it should be fine here.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

The patch looks ok, but there is still one case to handle, I think.
When adding a MozOrientation listener first time to a window,
perhaps that window should get an event pretty soon so that it can get the initial values for the orientation. Does that make sense?

Comment 5

[Doug Turner (:dougt)]

i do not think it matters that much because we get events pretty fast from the hardware.  If we increased the delta to something like 0.1 or .2, then we would have to consider the case you are talking about.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 396682 [details] [diff] [review]
patch v.2

> void 
> nsAccelerometer::AccelerationChanged(double x, double y, double z)
> {
>+
>+  if ( PR_ABS(mLastX - x) < .01 &&
>+       PR_ABS(mLastY - y) < .01 &&
>+       PR_ABS(mLastZ - z) < .01 )
>+    return;
Extra space after ( and before )


>+
>+  mLastX = x; mLastY = y; mLastZ = z;
>+
Put these to separate lines.

>-protected:
>+  double mLastX, mLastY, mLastZ;
And these too.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> i do not think it matters that much because we get events pretty fast from the
> hardware.
Well, some hardware may not provide results so fast. So if the the previous
value has been cached, and user doesn't move the device at all, it may
take long time before there is an orientation change which is above the change
threshold.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So, please fix that too.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.3

ensures that all new listeners get the latest data when they are added.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 396687 [details] [diff] [review]
patch v.3


>@@ -144,16 +152,17 @@ NS_IMETHODIMP nsAccelerometer::AddListen
> {
>   if (mStarted == PR_FALSE) {
>     startDisconnectTimer();
>     Startup();
>     mStarted = PR_TRUE;
>   }
> 
>   mListeners.AppendObject(aListener);
>+  AccelerationChanged(mLastX, mLastY, mLastZ, PR_FALSE);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP nsAccelerometer::RemoveListener(nsIAccelerationListener *aListener)
> {
>   mListeners.RemoveObject(aListener);
>   return NS_OK;
> }
>@@ -162,28 +171,41 @@ NS_IMETHODIMP nsAccelerometer::AddWindow
> {
>   if (mStarted == PR_FALSE) {
>     startDisconnectTimer();
>     Startup(); 
>    mStarted = PR_TRUE;
>   }
> 
>   mWindowListeners.AppendObject(aWindow);
>+  AccelerationChanged(mLastX, mLastY, mLastZ, PR_FALSE);
>   return NS_OK;
> }
> 
Sorry to be such a nitpicker, but is that really a safe time to dispatch an event?
Perhaps you could have something like "ResetCachedValues()", which would force the
next (asynchronous) AccelerationChanged?

Comment 11

[Doug Turner (:dougt)]

no problem about nitpicking; that is what this process is for.

Are you worried that the listener will be called before AddEventListener returns?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> Are you worried that the listener will be called before AddEventListener
> returns?
Well, for example about that.
Calling the listener asynchronously would be good.
Adding some reset flag should be trivial.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I mean some flag which cause the next timer run to call AccelerationChanged.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.4

This adds a new timer that fires 10ms after the event listener is added (hopeful enough time for us to recv data) ensuring that data is passed to new listeners.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.5 (without timer)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

[02:18] <smaug> dougt: this is not really about this bug... but the reason for StartDisconnectTimer  is to not shutdown immediately?
[02:19]	<smaug>	dougt: could we start that in RemoveListener if there are no listeners
[02:20]	<smaug>	dougt: in RemoveListener/RemoveWindowListener if mListeners.Count() == 0 && mWindowListeners.Count()
[02:20]	<smaug>	dougt: and if someone adds a new listener, cancel that timer
[02:22]	<smaug>	dougt: I'm assuming here that the reason to not shutdown immediately is to prevent activate/deactivate accelerometer all the time

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.6

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 396902 [details] [diff] [review]
patch v.6


> void 
> nsAccelerometer::AccelerationChanged(double x, double y, double z)
> {
>+  if (x > 1 || y > 1 || z > 1 || x < -1 || y < -1 || z < -1)
>+    return;
>+
>+  if (!mNewListener) {
>+    if (PR_ABS(mLastX - x) < .01 &&
>+        PR_ABS(mLastY - y) < .01 &&
>+        PR_ABS(mLastZ - z) < .01)
>+      return;
>+  }
>+
>+  mLastX = x;
>+  mLastY = y;
>+  mLastZ = z;
>+
You need to set mNewListener to PR_FALSE here.

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/cdd4847ae67f

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4418d05690d0