-
Notifications
You must be signed in to change notification settings - Fork 505
Conversation
* Implemented Barometer across all platforms. * Added tests modeling compass sensor tests * Added more unit tests * Added samples * removed unused usings * Added barometer to all sensor page * Updated docs * Removed unused ios properties * Implemented maps as mentioned in branch name issue. also fixed a spelling mistake * Fixed typo which caused ui bug * Finished reset of fork * removed proj items * Android emulator has barometer * Code cleanup, lazy introduction, access modifiers * Modified barometer to be in sync with generic event handlers * copy paste * Updated barometer docs/en/Xamarin.Essentials/Barometer.xml * Finished updating barometer docs * Removed remnants from delegates * Changes requested in pr * iOS queue change * Sensor Speed * sensor reset
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
File | Status | Preview URL | Details |
---|---|---|---|
Details |
- Line 80: [Warning] Failed to run script W:\w41c-s_dependentPackages\ECMA2Yaml\tools\Run.ps1 with exit code -1
For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.
This comment has been minimized.
This comment has been minimized.
static void OnChanged(BarometerChangedEventArgs e) | ||
{ | ||
if (useSyncContext) | ||
MainThread.BeginInvokeOnMainThread(() => ReadingChanged?.Invoke(null, e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check here whether we are already in main thread? If so we should use the else branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what BeginInvokeMainThread does :) and your fixes fix this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that comment prompted my investigation. Irrelevant now :)
public BarometerData Reading { get; } | ||
} | ||
|
||
public struct BarometerData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make struct readonly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is should the struct itself be readonly or should the property be readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i guess cause auto property is readonly alreayd then the struct could be. I don't think this is an issue at all. All of them could be. Can you add that to your PR for other structs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did, it is in my struct optimization pr.
This comment has been minimized.
This comment has been minimized.
true; | ||
#elif __IOS__ | ||
// iphone 6 and never have a barometer. looking in how to test this. | ||
Xamarin.Essentials.DeviceInfo.DeviceType == Xamarin.Essentials.DeviceType.Physical; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs the iphone 6 and newer check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is fine, need to just document it.
@@ -176,6 +176,29 @@ | |||
<Button Grid.Row="5" Grid.Column="1" Text="Stop" Command="{Binding StopCommand}" | |||
IsEnabled="{Binding IsActive}" /> | |||
</Grid> | |||
<Label Text="Monitor barometer for changes." FontAttributes="Bold" Margin="12" /> | |||
<Grid x:Name="GridBarometer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compress this Layout?
This comment has been minimized.
This comment has been minimized.
The BarometerData struct is missing the override for GetHashCode and does not implement IEquatable yet |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
Description of Change
Describe your changes here.
Bugs Fixed
Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.
API Changes
List all API changes here (or just put None), example:
Behavioral Changes
Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.
PR Checklist