Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

[PM-3349] [PM-3350] MAUI Migration Initial - #2806

Merged
fedemkr merged 213 commits into
mainfrom
feature/maui-migration
Feb 8, 2024
Merged

[PM-3349] [PM-3350] MAUI Migration Initial#2806
fedemkr merged 213 commits into
mainfrom
feature/maui-migration

Conversation

@fedemkr

@fedemkr fedemkr commented Sep 29, 2023

Copy link
Copy Markdown
Member

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Migrate app to NET MAUI

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@bitwarden-bot

bitwarden-bot mannequin commented Sep 29, 2023

Copy link
Copy Markdown
Mannequin

Logo
Checkmarx One – Scan Summary & Detailsa6f03895-0189-451c-bd5c-34aa7140205c

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Unpinned Actions Full Length Commit SHA /build.yml: 491 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity Issue Source File / Package
MEDIUM Privacy_Violation /src/App/App.xaml.cs: 111
MEDIUM Privacy_Violation /src/App/App.xaml.cs: 67
MEDIUM Unpinned Actions Full Length Commit SHA /build.yml: 500

fedemkr and others added 26 commits September 29, 2023 12:12
Added CustomTabbedPageHandler for Android to handle the tab "reselection" for PopToRoot.
Commented support for Windows in App.csproj
Disabled Interpreter on Android to avoid very slow app in Debug (during Login for example)
Added some null checks that were causing crashes (on GeneratorPageVM and PickerVM)
Minor TabsPage cleanup
Changed MainApplication SpecialFolder.Personal to SpecialFolder.LocalApplicationData
…d factor auth flows

Ensured CustomTabbedPageHandler had it's DisconnectHandler called
Some minor code upgrades of older obsolete Xamarin Forms code.
…Android bugs where they fill much larger space.
Added (migrated) CustomNavigationHandler (which should partially fix the AvatarIcon in the NavBar in iOS)
Added (migrated) CustomContentPageHandler (which should mostly place the AvatarIcon in the navBar in the correct place for iOS)
Added Task.Delay (workaround) to allow the Avatar to load in iOS on the LoginPage
Added workaround for iOS bug with the toolbar size (more info in comment in AvatarImageSource.cs)
Went through the AccountViewCell MAUI-Migration comments. (and deleted/added more comments as needed)
Migrated some Device calls to DeviceInfo and MainThread
Added (migrated) CustomTabbedHandler (for managing the iOS TabBar)
…the buggy shadows on the Android Fab Button.
…h fonts as MAUI is taking care of Accessibility and no custom code should be needed

Migrated SelectableLabelRenderer to Handler
Cleaned LabelHandlerMappings and added logic to migrate the CustomLabelRenderer
Added Argon libraries for Android
minor change to gitignore so that the Argon x86 lib is not ignored on the Android platform
…ound with some comments to be able to see the Generated Password on iOS
SearchBarHandlerMapping: IME options working as intended
SliderHandlerMappings: The MAUI "replacement" for Color.Default seems to be White so the old use case doesn't seem to be needed anymore.
fedemkr and others added 8 commits January 29, 2024 13:58
# Conflicts:
#	src/App/Platforms/Android/AndroidManifest.xml
…hat is causing the background crash on iOS; so we can test this in TestFlight
* workaround for sliding elements in duo 2fa flow

* restrict workaround to Android

* restrict workaround to Android

* Revert "restrict workaround to Android"

This reverts commit c2753d5.

* Revert "restrict workaround to Android"

This reverts commit 69688cf.
@fedemkr fedemkr added the MAUI label Jan 31, 2024

@andrebispo5 andrebispo5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome work!
Just a couple of things that came to attention on a first pass.

Comment thread src/Core/Controls/AuthenticatorViewCell/AuthenticatorViewCell.xaml Outdated
@@ -8,13 +9,14 @@ public class IconButton : Button
public IconButton()
{
Padding = 0;
// TODO Xamarin.Forms.Device.RuntimePlatform is no longer supported. Use Microsoft.Maui.Devices.DeviceInfo.Platform instead. For more details see https://learn.microsoft.com/en-us/dotnet/maui/migration/forms-projects#device-changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ was this address? can it be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As you can see Device.RuntimePlatform is still being used, it should be changed for DeviceInfo.Platform instead or use precompiler directives. However, this is really low priority.

@@ -9,13 +10,14 @@ public class IconLabel : Label

public IconLabel()
{
// TODO Xamarin.Forms.Device.RuntimePlatform is no longer supported. Use Microsoft.Maui.Devices.DeviceInfo.Platform instead. For more details see https://learn.microsoft.com/en-us/dotnet/maui/migration/forms-projects#device-changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ was this address? can it be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As you can see Device.RuntimePlatform is still being used, it should be changed for DeviceInfo.Platform instead or use precompiler directives. However, this is really low priority.

@@ -7,13 +8,14 @@ public class MiButton : Button
public MiButton()
{
Padding = 0;
// TODO Xamarin.Forms.Device.RuntimePlatform is no longer supported. Use Microsoft.Maui.Devices.DeviceInfo.Platform instead. For more details see https://learn.microsoft.com/en-us/dotnet/maui/migration/forms-projects#device-changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ was this address? can it be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As you can see Device.RuntimePlatform is still being used, it should be changed for DeviceInfo.Platform instead or use precompiler directives. However, this is really low priority.


namespace Bit.App.Controls
{
public class MiLabel : Label
{
public MiLabel()
{
// TODO Xamarin.Forms.Device.RuntimePlatform is no longer supported. Use Microsoft.Maui.Devices.DeviceInfo.Platform instead. For more details see https://learn.microsoft.com/en-us/dotnet/maui/migration/forms-projects#device-changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ was this address? can it be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As you can see Device.RuntimePlatform is still being used, it should be changed for DeviceInfo.Platform instead or use precompiler directives. However, this is really low priority.

StyleClass="box-value"
AutomationProperties.IsInAccessibleTree="True"
AutomationProperties.Name="{u:I18n Name}"
SemanticProperties.Description="{u:I18n Name}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔧 According to Microsoft docs Entry's should use Placeholder instead of Description because of Android's Talkback.
SemanticProperties.Placeholder="{u:I18n Name}"
MicrosoftDocs
Github Issue
image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we proceed with this change there a couple more places that this is happening and needs to me fixed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice finding 🎉 , can you create a Jira ticket with this? At the moment, we just replaced the whole solution Name with Description to compile, but yes we should definitely tackle this at some point.

@aj-rosado aj-rosado left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work!
Just added some comments regarding cleanup work and a couple workarounds that seem to be fixed.

Comment thread src/App/Platforms/Android/Handlers/LabelHandlerMappings.cs
{
base.OnResume();
Xamarin.Essentials.Platform.OnResume();
//Xamarin.Essentials.Platform.OnResume();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not necessary anymore we could remove this

Xamarin.Essentials.Platform.OnRequestPermissionsResult(requestCode, permissions, grantResults);
PermissionsHandler.OnRequestPermissionsResult(requestCode, permissions, grantResults);
Platform.OnRequestPermissionsResult(requestCode, permissions, grantResults);
//Xamarin.Essentials.Platform.OnRequestPermissionsResult(requestCode, permissions, grantResults);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If not necessary, remove the commented code

ZXing.Net.Mobile.Forms.Android.Platform.Init();
});
CrossFingerprint.SetCurrentActivityResolver(() => CrossCurrentActivity.Current.Activity);
//Task.Run(() =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be removed?


//Workaround: [MAUI-Migration] There is currently a bug in MAUI where the actual size of the image is used instead of the size it should occupy in the Toolbar.
//This causes some issues with the position of the icon. As a workaround we make the icon smaller until this is fixed.
//Github issues: https://github.com/dotnet/maui/issues/12359 and https://github.com/dotnet/maui/pull/17120

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This issues have been closed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think @dinisvieira tested it and the issue was still happening. Was it? Maybe we should check again.

fedemkr and others added 4 commits February 2, 2024 15:48
…rs and also made easier to maintain loading the argon2id library on the iOS projects by setting a general Directory.Build.props that is shared.
…rties instead of replacing the existing Options object which would cause the AccountSwitcher button bug (#2973)
…on (#2969)

* PM-5896 Fix background crash on iOS due to lock files when app gets suspended. Changed loading and error placeholders of the CachedImage to not be used and use default icon of IconLabel instead changing visibility.

* PM-5896 Changed methods to be protected so that they don't get removed by the linker.

* PM-5896 Added stub class and references to it so to have stronger references to Icon_Success and Icon_Error so the linker doesn't remove them.
@vgrassia
vgrassia self-requested a review February 5, 2024 15:54

@LRNcardozoWDF LRNcardozoWDF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Amazing work! Just found a nitpick.

var magImage = (Android.Widget.ImageView)handler.PlatformView.FindViewById(magId);
magImage.LayoutParameters = new Android.Widget.LinearLayout.LayoutParams(0, 0);
}
catch { }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this empty catch intentional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dinisvieira
dinisvieira mannequin dismissed a stale review via 759627b February 7, 2024 17:50
dinisvieira and others added 4 commits February 7, 2024 19:08
… light theme on iOS (#2982)

* PM-5907 workaround for incorrect textcolor when programmatically changing text on Entry

* Update src/Core/Pages/Vault/CipherAddEditPage.xaml.cs

Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

---------

Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
…when using light theme on iOS (#2981)

* PM-5906 workaround for incorrect textcolor when programmatically changing text on Entry

* Update src/Core/Pages/Send/SendAddEditPage.xaml.cs

Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

---------

Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

@aj-rosado aj-rosado left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@fedemkr
fedemkr merged commit 39a34bd into main Feb 8, 2024
@fedemkr
fedemkr deleted the feature/maui-migration branch February 8, 2024 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants