Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all usage of Device throughout the repo #4982

Merged
merged 16 commits into from
Mar 3, 2022

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Mar 1, 2022

Description of Change

The Device class now no longer holds any new or useful functionality that cannot be found in a better and more coherent place after #1965 .

This PR now goes and marks Device as obsolete and then replaces all usages with the new and improved APIs. Device now just exists as an obsolete compatibility API.

Issues Fixed

Fixes #4549

@mattleibow mattleibow marked this pull request as draft March 1, 2022 11:51
@mattleibow mattleibow self-assigned this Mar 1, 2022
@mattleibow mattleibow added this to the 6.0.300-preview.14 milestone Mar 1, 2022
@mattleibow mattleibow marked this pull request as ready for review March 1, 2022 13:35
@mattleibow mattleibow requested a review from Eilon March 1, 2022 15:58
@mattleibow mattleibow marked this pull request as draft March 1, 2022 15:58
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Blazor changes look fine to me.

@mattleibow mattleibow changed the title Try and remove all usage of Device Remove all usage of Device throughout the repo Mar 1, 2022
Comment on lines -134 to +135
if (Device.RuntimePlatform == Device.iOS)
if (DeviceInfo.Platform == DevicePlatform.iOS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should checks like these use the OperatingSystem class?

https://docs.microsoft.com/dotnet/api/system.operatingsystem.isios?view=net-6.0

Do we even need DeviceInfo.Platform?

Copy link
Member

@akoeplinger akoeplinger Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, OperatingSystem is the recommended API going forward.

It also makes it easier for the linker to trim out code since e.g. OperatingSystem.IsIOS() is hardcoded to true in the BCL on these platforms (though one thing to keep in mind is that IsIOS() also returns true on Catalyst, so you need to check !IsMacCatalyst() if you really only mean iOS).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess one issue is that DevicePlatform has entries for UWP/Tizen which OperatingSystem does not, though it should still be worthwhile to switch the other cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for the BCL API to have a more generic API? Instead of IsXxx and have a string based one? Or even mark some api as a "this method returns true on platform X"? So we can add our own platforms and then inform the linker?

Copy link
Member Author

@mattleibow mattleibow Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose IsOSPlatform("Tizen") exists and is linker-friendly? Maybe we can use that? If Samsung implemented it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah OperatingSystem.IsOSPlatform("Tizen") should work (assuming they implement it), but it won't be linker friendly since the linker can't tell it from the string I think.

Looking around in dotnet/runtime it seems that they instead opted to be regarded as a Linux distribution like Ubuntu, Debian, etc. and just return true for .IsLinux().

_webviewManager = new AndroidWebKitWebViewManager(
PlatformView,
Services!,
new MauiDispatcher(Services!.GetRequiredService<IDispatcher>()),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using the Device.Invoke we can just capture the dispatcher at the time of adding the view to the UI. This changes for all platforms.

Device.StartTimer(TimeSpan.FromMilliseconds(10), () =>
{
UpdateToolbar();
return false;
});
PostDelayed(() => UpdateToolbar(), 10);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases I just used the native "run on UI thread" stuff as that means we don't need to care about passing a dispatcher or the maui control in.

public static IItemsViewSource Create(IEnumerable itemsSource, ICollectionChangedNotifier notifier)
public static IItemsViewSource Create(IEnumerable itemsSource, BindableObject container, ICollectionChangedNotifier notifier)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PureWeen I saw this was done for ObservableItemTemplateCollection for Windows, so I copied that. Mainly just for the dispatcher, but if we pass the view in, then we never have to worry about reaching out into the Application. or Device. worlds.

Not sure if this is going to do terrible things with lifetime and/or memory. If so, we can change and find another way.

Comment on lines -216 to +217
var source = ItemsSourceFactory.Create(_groupSource[n] as IEnumerable, this);
var source = ItemsSourceFactory.Create(_groupSource[n] as IEnumerable, _groupableItemsView, this);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of apssing in the groupable view itself into the items source collection thing above.

Comment on lines +101 to 103
[Obsolete]
public static void Init(IActivationState activationState, InitializationOptions? options = null) =>
Init(activationState.Context, options);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked all of the Init things in the Forms type obsolete so that everyone knows not to use it. It is still used internally for now, but this is basically a non-public thing with all the work for the hosting and registration.

Comment on lines +25 to +27
#pragma warning disable CS0612 // Type or member is obsolete
Forms.Init(state, new InitializationOptions { Flags = InitializationFlags.SkipRenderers });
#pragma warning restore CS0612 // Type or member is obsolete
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for us to still use the Forms.Init in the builder, we can just do this. Eventually, all this goes away and it is pretty slim right now anyway.

Comment on lines -24 to +25
var ois = ItemsSourceFactory.Create(source, new MockCollectionChangedNotifier());
var ois = ItemsSourceFactory.Create(source, Application.Current, new MockCollectionChangedNotifier());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, but this is exactly what Device.InvokeOnMainThread did. And in the case of a test, the "container" is the app. So... yeah?

@@ -10,30 +10,41 @@
namespace Microsoft.Maui.Controls
{
/// <include file="../../docs/Microsoft.Maui.Controls/Device.xml" path="Type[@FullName='Microsoft.Maui.Controls.Device']/Docs" />
//[Obsolete]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't go 100% removed because of Device.GetNamedSize and potentially Device.Styles

Comment on lines +387 to 388
[Obsolete]
public static void RegisterAll(Type[] attrTypes, IFontRegistrar fontRegistrar = null)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the RegisterAll methods obsolete as these are probably not the droids users are looking for.

Comment on lines -115 to +116
if (Device.RuntimePlatform == Device.GTK && GTK != s_notset)
if (DeviceInfo.Platform == DevicePlatform.Create("GTK") && GTK != s_notset)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not worth the code and will be removed in #4491 but it is here for now and does not break anything.

@mattleibow mattleibow marked this pull request as ready for review March 3, 2022 10:26
@mattleibow mattleibow requested a review from a team as a code owner March 3, 2022 10:26
@mattleibow mattleibow changed the base branch from main to release/6.0.2xx-preview14 March 3, 2022 15:47
@mattleibow mattleibow merged commit b915256 into release/6.0.2xx-preview14 Mar 3, 2022
@mattleibow mattleibow deleted the dev/obsolete-device branch March 3, 2022 17:29
Redth added a commit that referenced this pull request Mar 3, 2022
* Implement IBorder on Windows (#5008)

* Implement IBorder on Windows

* Fix rectangles

Co-authored-by: redth <jondick@gmail.com>

* Remove all usage of Device throughout the repo (#4982)

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Redth added a commit that referenced this pull request Mar 4, 2022
* Implement IBorder on Windows (#5008)

* Implement IBorder on Windows

* Fix rectangles

Co-authored-by: redth <jondick@gmail.com>

* Remove all usage of Device throughout the repo (#4982)

* Remove unsupported targets in Device class (#4491)

* Remove unsupported targets in Device class

* Fixed broken tests

* Deprecate UWP target

* Allow to use UWP target on XAML

* Fix build errors

* comments

* UWP == WinUI

* Fix Build errors

* Added more tests

* fix

* vbnm

* More fixes

* things

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Redth added a commit that referenced this pull request Mar 4, 2022
* Implement IBorder on Windows (#5008)

* Implement IBorder on Windows

* Fix rectangles

Co-authored-by: redth <jondick@gmail.com>

* Remove all usage of Device throughout the repo (#4982)

* Remove unsupported targets in Device class (#4491)

* Remove unsupported targets in Device class

* Fixed broken tests

* Deprecate UWP target

* Allow to use UWP target on XAML

* Fix build errors

* comments

* UWP == WinUI

* Fix Build errors

* Added more tests

* fix

* vbnm

* More fixes

* things

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

* [ci] Remove image override (#5053)

* [ci] Remove image override

* [ci] Don't provisioning windows

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Co-authored-by: Rui Marinho <me@ruimarinho.net>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2!
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Obsolete Device in Controls
6 participants