Fix for Android Cancel/Outside touch event / Added SensorBase.CurrentValue update #1140

Merged
merged 4 commits into from Jan 6, 2013

5 participants

@picrap

I don't know why, the two commits came in the same pull request, and I can't figure out how to create a pull request with only one commit.

picrap added some commits Jan 2, 2013
@picrap picrap Fix for Android Cancel/Outside touch event
The wrong id was used, the whole range needed to be cancelled.
8fbe458
@picrap picrap Added SensorBase.CurrentValue update
Prior to this, only the CurrentValueChanged event was fired, but the
CurrentValue never changed.
d0ccec0
@dellis1972

@picrap If you want this as two separate PR's you will need to create a branch for each and submit the PR from each branch. This is one of the things I think git is lacking, it would be nice to submit a PR for a specific commit.

@Aranda

To elaborate on Dean's comment, pull requests should be created from branches to allow the change set to be reviewed and merged individually. So to fix your situation, you could create two new branches from the commit prior to your changes, then cherry pick each individual commit onto it's own branch. Or, you could reset your head to the first commit and create a branch and pull request, then wait for that to be merged before creating a pull request for the second commit.

@Aranda

That said, both these changes seem good to me (with the improvement I mentioned above), so if others agree we can probably leave it and merge them both at once. Alternatively you could treat it as a git learning exercise :)

@KonajuGames

The changes look good to me. Learn about git branches for the next pull request you make. We won't make you change this one.

@KonajuGames

iOS will need to be updated as well though.

@picrap

Thanks guys, I'll fix the firing of event only if data has changed, and, of course, work with different branches :)
Regarding iOS, how do you proceed for this feature? Do you report the missing implementation as a bug?

@Nezz
MonoGame member

What's missing from iOS?

@KonajuGames
@picrap picrap Re-added SensorBarse.FireOnCurrentValueChanged
In order to keep a smooth transition for other platforms
037a7ef
@picrap

Oops. I readded the deleted method, with the ObsoleteAttribute.

@Nezz Nezz commented on the diff Jan 3, 2013
...ame.Framework/Microsoft/Devices/Sensors/SensorBase.cs
@@ -14,7 +14,18 @@ public abstract class SensorBase<TSensorReading> : IDisposable
#endif
bool disposed;
private TimeSpan timeBetweenUpdates;
- public TSensorReading CurrentValue { get; protected set; }
+ private TSensorReading currentValue;
+
+ public TSensorReading CurrentValue
+ {
+ get { return currentValue; }
+ protected set
+ {
+ currentValue = value;
@Nezz
MonoGame member
Nezz added a line comment Jan 3, 2013

You should check here if the value was really changed like:
if (currentValue != value)
{
do stuff
}

@picrap
picrap added a line comment Jan 3, 2013

I just checked, and it doesn't work like that. Operator != can not apply to generics, so I'll need to dig a bit further.

@Nezz
MonoGame member
Nezz added a line comment Jan 5, 2013

!EqualityComparer.Default.Equals(currentValue, value)?

@picrap
picrap added a line comment Jan 5, 2013

To me this is another improvement, and I see two reasons not to hurry (the third being that I'm lazy :)):
1. Due to sensors sensibility, the values are actually different each time (just make the test).
2. The behavior until today was to always fire the event, and apparently there was not much complain about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Nezz
MonoGame member

Instead of readding the method, please remove the calls of FireOnCurrentValueChanged from the two iOS files (that's just deleting one line per file). If we leave it like this, changed events will happen twice.

@picrap

I'll take a look, but I'm uncomfortable with this, since I have no way to check the result, not event the build.

@Nezz
MonoGame member

That's why I doublechecked this and if you remove the call it's fine :)

@picrap picrap Removed FireOnCurrentValueChanged and iOS calls
Since it appears to be the only platform still using the method
6d7a065
@picrap

Done, removed FireOnCurrentValueChanged and iOS calls.

@KonajuGames

Merging.

@KonajuGames KonajuGames merged commit 24bcdf7 into MonoGame:develop3d Jan 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment