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.

Sign up for GitHub

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

[AOT compatible] Clean up some AOT build issue in PowerAccent.Core #36264

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+18 −9
Open

[AOT compatible] Clean up some AOT build issue in PowerAccent.Core #36264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f798e5e
init
Nov 28, 2024
e1be3a6
Merge branch 'main' into yuleng/AOT/PowerAccent.Core
Dec 9, 2024
01a34a4
Use AotCompatibility instead
Dec 9, 2024
42b5b98
Replace typeof(Lanaguge) with GetValues<Language>
Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/modules/poweraccent/PowerAccent.Core/Languages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ internal sealed class Languages
{
public static string[] GetDefaultLetterKey(LetterKey letter, Language[] langs)
{
if (langs.Length == Enum.GetValues(typeof(Language)).Length)
if (langs.Length == Enum.GetValues<Language>().Length)
{
return GetDefaultLetterKeyALL(letter);
}
Expand Down
1 change: 1 addition & 0 deletions src/modules/poweraccent/PowerAccent.Core/PowerAccent.Core.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<!-- Look at Directory.Build.props in root for common stuff as well -->
<Import Project="..\..\..\Common.Dotnet.CsWinRT.props" />
<Import Project="..\..\..\Common.SelfContained.props" />
<Import Project="..\..\..\Common.Dotnet.AotCompatibility.props" />

<PropertyGroup>
<ImplicitUsings>enable</ImplicitUsings>
Expand Down
2 changes: 1 addition & 1 deletion src/modules/poweraccent/PowerAccent.Core/PowerAccent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

namespace PowerAccent.Core;

public class PowerAccent : IDisposable
public partial class PowerAccent : IDisposable
{
private readonly SettingsService _settingService;

Expand Down
8 changes: 1 addition & 7 deletions src/modules/poweraccent/PowerAccent.Core/Services/SettingsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@ public SettingsService(KeyboardListener keyboardListener)
_watcher = Helper.GetFileWatcher(PowerAccentModuleName, "settings.json", () => { ReadSettings(); });
}

private static readonly JsonSerializerOptions _serializerOptions = new JsonSerializerOptions
{
WriteIndented = true,
};

private void ReadSettings()
{
// TODO this IO call should by Async, update GetFileWatcher helper to support async
Expand All @@ -46,9 +41,8 @@ private void ReadSettings()
{
Logger.LogInfo("QuickAccent settings.json was missing, creating a new one");
var defaultSettings = new PowerAccentSettings();
var options = _serializerOptions;

_settingsUtils.SaveSettings(JsonSerializer.Serialize(this, options), PowerAccentModuleName);
_settingsUtils.SaveSettings(JsonSerializer.Serialize(this, SourceGenerationContext.Default.SettingsService), PowerAccentModuleName);
}

var settings = _settingsUtils.GetSettingsOrDefault<PowerAccentSettings>(PowerAccentModuleName);
Expand Down
14 changes: 14 additions & 0 deletions src/modules/poweraccent/PowerAccent.Core/Services/SourceGenerationContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Text.Json.Serialization;

namespace PowerAccent.Core.Services
{
[JsonSourceGenerationOptions(WriteIndented = true)]
[JsonSerializable(typeof(SettingsService))]
public partial class SourceGenerationContext : JsonSerializerContext
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should use a common Helpers or Contexts folder to put these JsonSerializerContext classes underneath, as there may ones that encompass multiple JsonSerializable types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@moooyo - Other than this, change, it LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. any suggestion? How about move to ManagedCommon? @snickler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one confuse, It seems that we cannot centrailize it? Let me explain why I think that.

Consider we have a sub-project named JsonContextCommon, which include all the jsonSerializerContext we need.
Then, if we add a new jsonSerializerContext, we also need to add the [JsonSerializeable(typeof(T))]. Which require us to ref the Type T sub-project in the JsonContextCommon sub-project.
Now, we need to use this context. So, we need to ref JsonContextCommon sub-project in the project which we want to use it. We will necounter cycle reference error.

Only if we move all Type T to a independent sub-project such as ModelCommon, can we move the JsonSerializerContext to the same centralized location.

@snickler

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mooyo - In terms of centralization, I meant per project. The JsonSerializerContext classes created should have common folder placement and structure for consistency.

{
}
}
Loading