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

ObjectStorage enhancements #4102

Merged
57 commits merged into from
Jul 29, 2021
Merged

ObjectStorage enhancements #4102

57 commits merged into from
Jul 29, 2021

Conversation

shweaver-MSFT
Copy link
Member

@shweaver-MSFT shweaver-MSFT commented Jul 8, 2021

Fixes #3903

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The ObjectStorage helper interfaces and implementations currently live in the *.Uwp package. This means that they are not consumable from NetStandard projects, such as the CommunityToolkit.Graph package.

In addition, the IObjectStorageHelper is a mixture of support for both dictionary style settings storage, with some file storage features as well. Currently, supporting both file and folder scenarios is odd/difficult for storage endpoints that don't operate in a similar manner to Windows.Storage.ApplicationData.

Lastly, the support for file storage in IObjectStorageHelper is not complete and missing some basic CRUD operations that can make it difficult to work with in real-world application scenarios.

What is the new behavior?

I've done a few things:

  1. Deprecated the existing structures in the Microsoft.Toolkit.Uwp/Helpers/ObjectStorage folder:

    • BaseObjectStorageHelper
    • IObjectSerializer
    • IObjectStorageHelper
    • LocalObjectStorageHelper
    • SystemSerializer
  2. Migrated some of the previous structures up into the Microsoft.Toolkit/Helpers/ObjectStorage folder so that they are consumable from NetStandard:

    • IObjectSerializer
    • SystemSerializer
  3. Created new interfaces to replace the functionality defined in the now defunct IObjectStorageHelper:

    • ISettingsStorageHelper - Interop with values as key/value pairs, like a dictionary.
    • IFileStorageHelper - Interop with a file system to store values in files and folders.
  4. Replaced the functionality provided by BaseObjectStorageHelper and LocalObjectStorageHelper with an implementation of ISettingsStorageHelper and IFileStorageHelper:

    • ApplicationDataStorageHelper - Interop with local settings and files through Windows.Storage.ApplicationData.

Use it like so:

ApplicationDataStorageHelper appDataStorageHelper = ApplicationDataStorageHelper.GetCurrent(new Toolkit.Helpers.SystemSerializer());

// Save and Read simple objects
string keySimpleObject = "simple";
appDataStorageHelper.Save(keySimpleObject, 42);
appDataStorageHelper.TryRead<string>(keySimpleObject, out string result);

// Save and Read complex objects
string complexObjectKey = "complexObject";
await appDataStorageHelper.SaveFileAsync(complexObjectKey, new MyComplexObject());
var complexObject = await appDataStorageHelper.ReadFileAsync<MyComplexObject>(complexObjectKey);

// Complex object example
public class MyComplexObject
{
    public string MyContent { get; set; }
    public List<string> MyContents { get; set; }
    public List<MyComplexObject> MyObjects { get; set; }
}

Altogether, this provides a transition path from the previous ObjectStorage constructs to a more granular set of interfaces with identical signatures plus enhanced support for file and folder CRUD operations.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

Looking for feedback!

…Toolkit package, out of *.Toolkit.Uwp.

Replaced Local/BaseObjectStorageHelper classes with ApplicationDataStorageHelper.
@shweaver-MSFT shweaver-MSFT self-assigned this Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested a review from azchohfi July 8, 2021 20:22
@ghost ghost requested a review from Rosuavio July 8, 2021 20:22
@ghost ghost added the feature request 📬 A request for new changes to improve functionality label Jul 8, 2021
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Awesome start @shweaver-MSFT! Thanks! 🎉🎉🎉 This is going to be Epic!

Glad everything is pretty much just drop-over and replace for existing users, that's amazing. Really like how that all came together and the extra thought you put into how these things get split, combined again, and a few of the missing features.

I did throw some curve-balls questions into the IFileStorageHelper discussion now that it's going to open up potential implementations on other platforms. Also maybe identified a couple of gaps. So let's just work through making sure we can solidify any assumptions there and document how things should behave for other future implementations (hopefully our existing UWP and OneDrive based implementations should provide enough context for what's reasonable for a File System to be able to do and what it should provide).

I think all that is mostly additive though, so we should still be pretty close. 🎉

@ghost
Copy link

ghost commented Jul 13, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@shweaver-MSFT
Copy link
Member Author

@michael-hawker I think I've responded to everything, let me know if I missed something.

I had zero luck getting the nuget package from the PR feed working yesterday, so I'll try again once the latest build completes and spits out a new package

@michael-hawker
Copy link
Member

Closing and re-opening to retrigger build with org name change.

@ghost
Copy link

ghost commented Jul 28, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@michael-hawker
Copy link
Member

Build errors:

 D:\a\1\s\Microsoft.Toolkit.Uwp\Helpers\ObjectStorage\ApplicationDataStorageHelper.cs(25,96): error CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [D:\a\1\s\Microsoft.Toolkit.Uwp\Microsoft.Toolkit.Uwp.csproj]
         D:\a\1\s\Microsoft.Toolkit.Uwp\Helpers\ObjectStorage\ApplicationDataStorageHelper.cs(37,124): error CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [D:\a\1\s\Microsoft.Toolkit.Uwp\Microsoft.Toolkit.Uwp.csproj]
         D:\a\1\s\Microsoft.Toolkit.Uwp\Helpers\ObjectStorage\ApplicationDataStorageHelper.cs(68,103): error CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [D:\a\1\s\Microsoft.Toolkit.Uwp\Microsoft.Toolkit.Uwp.csproj]

FYI @Sergio0694 there seem to be a lot of build warnings in the azure pipeline for t he Unit Tests for the Source Generators and some things for the HighPerformance package?

@ghost ghost removed the needs attention 👋 label Jul 29, 2021
@Sergio0694
Copy link
Member

"there seem to be a lot of build warnings in the azure pipeline for t he Unit Tests for the Source Generators and some things for the HighPerformance package?"

@michael-hawker thanks for the heads up! I've opened #4142 to fix them 😄

@ghost
Copy link

ghost commented Jul 29, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@michael-hawker
Copy link
Member

@Sergio0694 @shweaver-MSFT everything set now on this PR once it builds?

@ghost ghost removed the needs attention 👋 label Jul 29, 2021
@shweaver-MSFT
Copy link
Member Author

@Sergio0694 @shweaver-MSFT everything set now on this PR once it builds?

I believe it is!

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Wo-hooooo!! 🎉🎉🎉

:shipit:

@ghost
Copy link

ghost commented Jul 29, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit f7d00c7 into main Jul 29, 2021
@ghost ghost deleted the shweaver/storage-helpers branch July 29, 2021 19:26
@shweaver-MSFT
Copy link
Member Author

🎉🎉🎉 Woot!

ghost pushed a commit that referenced this pull request Aug 4, 2021
## Fixes #4102 (comment)
<!-- Add the relevant issue number after the "#" mentioned above (for ex: "## Fixes #1234") which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more options below that apply to this PR. -->

- Maintenance
<!-- - Bugfix -->
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
Some HighPerformance/Mvvm unit tests produce warnings when the projects are built.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
No more warnings 🙌

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [X] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [X] Sample in sample app has been added / updated (for bug fixes / features)
    - [X] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/CommunityToolkit/WindowsCommunityToolkit-design-assets)
- [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/CommunityToolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes
Sergio0694 pushed a commit to CommunityToolkit/dotnet that referenced this pull request Oct 29, 2021
## Fixes CommunityToolkit/WindowsCommunityToolkit#4102 (comment)
<!-- Add the relevant issue number after the "#" mentioned above (for ex: "## Fixes #1234") which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more options below that apply to this PR. -->

- Maintenance
<!-- - Bugfix -->
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
Some HighPerformance/Mvvm unit tests produce warnings when the projects are built.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
No more warnings 🙌

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [X] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [X] Sample in sample app has been added / updated (for bug fixes / features)
    - [X] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/CommunityToolkit/WindowsCommunityToolkit-design-assets)
- [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/CommunityToolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ feature request 📬 A request for new changes to improve functionality helpers ✋ improvements ✨ next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. priority 🚩
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Migrate ObjectStorage interfaces to *.Toolkit package to enable NetStandard consumption
4 participants