Skip to content

Add UWP support#139

Merged
kspearrin merged 12 commits intobitwarden:masterfrom
hichamboushaba:master
Oct 3, 2017
Merged

Add UWP support#139
kspearrin merged 12 commits intobitwarden:masterfrom
hichamboushaba:master

Conversation

@hichamboushaba
Copy link
Copy Markdown

@hichamboushaba hichamboushaba commented Sep 29, 2017

Hi, this is the first PR for UWP support, the application can compile and run (tested on a laptop with Windows CU).

The main changes to the PCL project:

The interfaces IGoogleAnalyticsService and IDeviceActionService are still not implemented.
And the application still uses the default theme which needs some tweaking.

#9

Copy link
Copy Markdown
Member

@kspearrin kspearrin left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

Comment thread bitwarden-mobile.sln
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "UWP", "src\UWP\UWP.csproj", "{3A2D5669-ED71-4F2B-BA85-2D36BAA05141}"
EndProject
Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "UWP.Images", "src\UWP.Images\UWP.Images.shproj", "{0BE54BBB-7772-4289-BD51-1FDBB0CC2446}"
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.

Does this really have to be a whole separate project?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Images have to be on root folder in UWP, which is not perfect for files organization.
Which is why I put them in a separate shared project.
Do you have a better idea?

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.

Fine for now I guess.

};

if(Device != null)
if(false && Device != null)
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.

Undo?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I was testing the connection and I forgot it
I will correct it

<Import_RootNamespace>UWP.Images</Import_RootNamespace>
</PropertyGroup>
<ItemGroup>
<Content Include="$(MSBuildThisFileDirectory)camera.png" />
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.

How are 2x, 3x, etc handled in UWP?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

these are the guidelines for images scaling: https://msdn.microsoft.com/library/windows/apps/xaml/hh965325?f=255&MSPPError=-2147217396

but the other resolutions can be added after having a read to publish app, if you are OK?

Comment thread src/UWP/App.xaml.cs Outdated
sealed partial class App : Application
{

public ISettings Settings { get; set; }
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.

move below ctor please

private const int KeyLength = 256; // 32 bytes

//todo review this
public byte[] DeriveKey(byte[] password, byte[] salt, uint rounds)
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.

Seems fine since you are using the same BC implementation as Android. I know .NET Core has a native implementation for PBKDF2 so maybe we can switch to that when we move to .NET standard.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I didn't know about native implementation of PBKDF2, I added it now

{
public class SecureStorageService : ISecureStorageService
{
private string resourceName = "BitWarden";
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.

use bitwarden

public class SecureStorageService : ISecureStorageService
{
private string resourceName = "BitWarden";
private PasswordVault _vault = new PasswordVault();
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.

readonly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK

Copy link
Copy Markdown
Member

@kspearrin kspearrin left a comment

Choose a reason for hiding this comment

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

Looks fine now. I'll merge sometime next week after the current mobile release is finished.

@hichamboushaba
Copy link
Copy Markdown
Author

I just added two commits, adding IDeviceActionService implementation, and fixing an issue with HasCamera property.

@kspearrin kspearrin merged commit d651606 into bitwarden:master Oct 3, 2017
@kspearrin
Copy link
Copy Markdown
Member

Thanks for this again. I have merged it in. I updated the code with a few commits to fix formatting issues and code style to match the rest of the project. Feel free to continue additional PRs as needed. I will revisit this at some point myself as well.

vvolkgang pushed a commit that referenced this pull request Jun 20, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
UCan927 pushed a commit to UCan927/BitWarden-Android that referenced this pull request Jun 22, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
vvolkgang pushed a commit that referenced this pull request Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants