-
Notifications
You must be signed in to change notification settings - Fork 500
Added Windows Desktop (WPF) platform implementation #1079
Conversation
…plication and tested available features.
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.
We will need to think about how we do this... What happens if we install this into a GTK app or some other macOS thing. We may need to implement a "desktop" set of files (instead of win32) and then based on what the running OS is, do the p/invokes and things. Basically, net461 will also install into GTK and that is going to cause issues. If we want to do a GTK platform, we need to think about how we know where we are running, before running anything.
Also, how does this play in the new netcoreapp world? (similar a future net5 world) At the end of the day, anything under the net* will install on pretty much everything not Xamarin. But, I think we can be safe with the fact that this will mostly be "desktop" worlds and the non-net* are all specific.
But, after all that said, this looks really good. This is not a little set of APIs, but something that will add value to all desktop apps.
{ | ||
public static partial class Preferences | ||
{ | ||
static Dictionary<string, object> preferences = new Dictionary<string, object>(); |
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.
Is this not actually saved to disk at all?
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.
Need some discussion over the implementation of this. This is temporary and required for the code to build and allow the sample app to run. There isn't a native API we can use (unless taking a dependency on the Windows 10 UWP API). A simple implementation would be to persist the dictionary(s) to a file but that may require a dependency on Newtonsoft.Json or similar. I think that will be the easiest option for now and we can discuss further the pros/cons.
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.
No need for newtonsoft, BinarySerializer etc. are all available and fit the scenario. Persistance is IMHO necessary for this to be considered workable.
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.
You could have a look here: https://docs.microsoft.com/en-us/dotnet/standard/serialization/
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.
Thanks, I'll rework this and use one of the built-in options.
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.
Going the file route comes with another set of problems:
We definitely have to have a lazy mechanism to store and retrieve settings. Loading all properties on first acess would increse the working set unnecessarily. Also, we do need fast key existance checks. Not only for Key enumeration but also for insertion. I am currently hooking up some different benchmarks for a couple of potential solutions, but this all seems very icky to me. Are there any usbale storage technologies available with winapi we could use? or even com perhaps
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.
Okay this could be a stupid idea but how about the registry? Fast key existance checks, key value store just as we like it and as persistent as it can be
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 that's frowned upon, we can't control how much it gets used for and unless the app is installed via Centennial it'll get dumped into the main registry.
</Reference> | ||
<Reference Include="WindowsBase" /> | ||
<Reference Include="PresentationCore" /> | ||
<Reference Include="PresentationFramework" /> |
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.
Just checking to see if we actually use the presentation bits for this project? We shouldn't be using any of the Xaml-related things. Also, this should work for winforms without 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.
At the moment it uses WPF dependencies for MainThread, to support either WinForms or WPF will require some reworking but this is a sensible point. I will investigate alternative methods of identifying the thread.
@@ -1,6 +1,6 @@ | |||
<Project Sdk="MSBuild.Sdk.Extras/2.0.54"> | |||
<PropertyGroup> | |||
<TargetFrameworks>netstandard1.0;netstandard2.0;Xamarin.iOS10;Xamarin.TVOS10;Xamarin.WatchOS10;MonoAndroid60;MonoAndroid70;MonoAndroid71;MonoAndroid80;MonoAndroid81;MonoAndroid90;MonoAndroid10.0;tizen40;</TargetFrameworks> | |||
<TargetFrameworks>netstandard1.0;netstandard2.0;Xamarin.iOS10;Xamarin.TVOS10;Xamarin.WatchOS10;MonoAndroid60;MonoAndroid70;MonoAndroid71;MonoAndroid80;MonoAndroid81;MonoAndroid90;MonoAndroid10.0;tizen40;net461</TargetFrameworks> |
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.
We will need to think about how we do this... net461 works in too many places.
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 assume this will work with some more detailed tweaking to create multiple net461 dlls in the runtimes folder of the NuGet.
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.
we should also add .Net Core WPF
Geolocation and coding is available via the com interfaces in locationapi.h as far as I know. Magnetometers are available in the sensorapi,h header via correspoding com objects |
@@ -1,43 +1,63 @@ | |||
using System; | |||
using System.Collections.Generic; | |||
using System.IO; | |||
using Newtonsoft.Json; | |||
using System.Runtime.Serialization; | |||
using System.Runtime.Serialization.Formatters.Binary; | |||
|
|||
namespace Xamarin.Essentials | |||
{ | |||
public static partial class Preferences | |||
{ | |||
static readonly string settingsPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), AppInfo.Name, "settings.dat"); | |||
|
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.
Any chance that AppInfo.Name
may contain invalid chars for win32 file names, such as a ":" ?
You could sanitize it by first removing any chars returned by Path.GetInvalidFileNameChars
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.
Good catch, the documentation for the attribute says "The assembly title is a friendly name, which can include spaces" but that's not particularly informative.
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.
It could also be blank ... perhaps fallback to using the file name of the launching assembly if the sanitized name is blank?
Path.GetFileNameWithoutExtension(Assembly.GetEntryAssembly().CodeBase);
{ | ||
if (preferences[CleanSharedName(sharedName)].ContainsKey(key)) | ||
{ | ||
return (T)preferences[CleanSharedName(sharedName)][key]; |
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.
Maybe a if (prefs.TryGetValue() && inner.TryGetValue())
using System.Runtime.Serialization; | ||
using System.Runtime.Serialization.Formatters.Binary; | ||
using PreferencesDictionary = System.Collections.Generic.Dictionary<string, System.Collections.Generic.Dictionary<string, object>>; |
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.
Something to consider would be using IStrongBox as value container, as we can ellide boxing/unboxing some of the time this way: IStrongBox {public T Value;}
When reading a key we can instead do (value as IStrongBox).Value. This pattern is common in storing value types without boxing as often.
An easy grab is Telephone and sms, simply invoke the launcher with the corresponding url. Same goes for sms. An improvement to launcher can be made by checking the registry for a corrsponding HKEY_CLASSES_ROOT%protocol%\shell\open\command registry key to check whether launching said url is supported |
|
||
namespace Xamarin.Essentials | ||
{ | ||
public static partial class Geolocation |
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.
Consider using the com interfaces for this, we can avoid having a dependency that way. The actual implementation uses the COM service as well. Very Optional though as the C# wrapper is quite a lot easier to deal with.
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.
@Mrnikbobjeff The System.Devices dll is a built in part of the .NET framework so doesn't introduce a dependency, using the COM interfaces will require more code to achieve the same result.
A few notes as @mattleibow and I have been discussing:
|
@Redth Thanks, Json was rejected before because Json.net adds a dependency and System.Text.Json not available on older .NET versions. The only consistently available text format in-box is XML... DPAPI/ProtectedData for SecureStorage makes sense, again physical storage is an issue. I assume all of these will be non-roaming as the other platforms don't roam their preferences? |
What about DataContractJsonSerializer? Or yeah System.Text.Json. Other platforms implicitly can ‘roam’ their data depending on where it’s stored |
@Redth - yes DataContractJsonSerializer looks like a good option. Looking at the current implementations for Essentials they are all using private/local storage. I assume for WPF we want to match the behaviour of the existing UWP implementation where possible. |
System.Text.Json does work on netstandard2.0 and net4.6.1 |
@mattleibow Given the simple use-case here is it worth using System.Text.Json (introducing a NuGet dependency) or stick with the built-in DataContractJsonFormatter? |
@peterfoot I would just use the contract serializer for now. Just wanted to say that it should work... But as I type this, I somehow feel that there was some issue with the serializer... Like dates... Not sure if anyone knows... Maybe it was around deserialization. But since we serialize and deserialize ourselves, we should be good. |
@mattleibow as we specify which types we accept (and all types we support are sealed) we do not encounter most serialisation issues such as ignoring validation when deserializing objects or expecting different types upon deserialization. Any text format should work without problems. |
@Mrnikbobjeff yes, but my question was that if we can use the already written UWP API for Windows 10, is it worth writing the Win32 ones. It would really be only a subset of tablets/laptops with the hardware and how many would be running Windows 7/8? |
MacOs does not offer an abstraction over general p&p devices which device drivers can target for these sensors. That is the limiting factor IMHO. Also, writing new sensors once the base implementation is done is achieved in less than 20 loc up to and including BarometerSensor |
@Mrnikbobjeff I'm not against implementing sensor APIs, I just wonder how many devices will be able to take advantage of them |
Very few at best, the most common sensors appear to be biometrics and luminosity sensors. My work device seems to have them both, and biometrics seem to be supported by other devices (primarily laptops) as well. |
…rently running even if it contains references to the other.
Just want to say this would be highly appreciated if it made it into Xamarin.Essentials. Given that WPF is part of Xamarin.Forms (even if only as preview) it makes perfect sense to get Essentials supported on WPF as well. At Blueye we are using WPF over UWP due to limitations in some third-party libraries which prevents us from using Store Distribution, and side-loading of UWP is not as nice/trivial as with WPF. I would be glad to help out with the implementation of some of the missing pieces. Would be great with some guidance on what would be required in order to have this merged (i.e. does all APIs need to be ported)? Thank you for your effort so far @peterfoot! It has already been of value as we have adopted parts of your implementation already while awaiting the merge of this PR. |
Oh yes! Xamarin essentials is coming to WPF! When this is planned to be released? |
Are there any updates for Xamarin Essentials supporting WPF? |
@acffordyce973 the PR was a bit stale so I've updated against the latest main branch so it should unblock the conflicts at least. |
Description of Change
Adds platform support for Windows desktop (WPF) apps.
Bugs Fixed
Adds the following platform implementations
API Changes
Added DevicePlatform.WindowsDesktop
Behavioral Changes
N/A
PR Checklist