Backend Refactoring Megathread

[Spiritreader]

A better place for discussions related to #1038 because we can have conversation threads!

[ChenYiLins]

About back-end reconfiguration:
I think it should be done step by step. Start with some bug fixes (I'll list issues about the backend later. Bugs with unclear or unrepeatable problems will not be listed), and then improve the stability of the backend (that is, start refactoring). CC @Spiritreader @Armin2208 @Jay-o-Way What's your opinion?

Issues

Name	Remark
UTF-8 encoding support for theme	#927
Script execution problem	#922
Windows Spotlight can't work	#891
Color filter didn't change on startup	#889
Theme switching does not work with multiple desktops	#469

[Spiritreader]

The linked issue #1038 (and this discussion) is stricly for refactoring aka changing existing code without impacting functionaltiy, not for fixing bugs. We should not start with bugfixes, but end with that after we're done modernizing the backend.
During the refactoring, I'd suggest we only fix new high impact issues if they come up, where something with theme switching breaks altogether without there being a workaround available.

So refactoring and bugfixes are two different tasks that exist independent from each other.

I can say sth about the issues tho. Almost all of them aren't really feasible to fix, and we always have to consider how many users have this issue vs how much time it is to fix them, especially if workarounds are available.

• Bug: Non UTF-8 encoding breaks theme state synchronization #927 requires research, and we need test cases to test against theme files that break. Requiring users set an ANSI name is not a super big limitation. In the meantime, we could detect invalid charsets and automatically set a default name or w/e.
• Bug: PowerShell script doesn't consistently execute when theme is toggled #922 I honestly don't know what's up there, and we only have one single report of this happening. Nobody else encounters this (including me, I'm using scripts every day), so I would chalk it up to a configuration issue on the user end.
• Bug: Windows Spotlight can't work #891 is a Windows issue and we have no agency there
• Bug: Color filter didn't change on startup #889 requires a re-implementation of the color filter mechanism
• Wallpaper switching does not work with multiple desktops #469 is not on the roadmap for an indefinite time-frame, because there's no proper API support

Lastly, I want to strictly NOT use AI tools for refactoring the backend (unless it is a really chorey staight forward task that cannot be solved with regular linters that auto-apply formatting), because the point of this is to understand every single LoC and make an informed decision on how/whether it needs to be changed.

[ChenYiLins]

The linked issue #1038 (and this discussion) is stricly for refactoring aka changing existing code without impacting functionaltiy, not for fixing bugs. We should not start with bugfixes, but end with that after we're done modernizing the backend. During the refactoring, I'd suggest we only fix new high impact issues if they come up, where something with theme switching breaks altogether without there being a workaround available.

Got it!

Lastly, I want to strictly NOT use AI tools for refactoring the backend (unless it is a really chorey staight forward task that cannot be solved with regular linters that auto-apply formatting), because the point of this is to understand every single LoC and make an informed decision on how/whether it needs to be changed.

I hold the same attitude in this respect, because I still think AI is not reliable ಠ_ಠ, but I don't mind using AI to check spelling mistakes and so on.

[Spiritreader]

I hold the same attitude in this respect, because I still think AI is not reliable ಠ_ಠ, but I don't mind using AI to check spelling mistakes and so on.

Indeed. There's just too big of a risk of Ai models starting to change code at random as well, or removing stuff because it thinks it can be omitted.

[Jay-o-Way]

If I understand correctly - basically something like #1004, but with lots of planning and splitting in several PRs. Right? Then I think these are some nice points to focus on.

• use switch statements
• general spelling etc. (including capitalization)
• accessibility modifiers
• see if _errorService.ShowErrorMessage message can be simplified or more precise?
• see if anything needs a fallback, example: loader.GetString(Name) ?? $"[{Name}]"
• simplifying code, like
  • use ternary conditions
  • unneeded checks, example: if (!Directory.Exists(folderPath)) then CreateDirectory
  • unneeded code, example: args.Handled = false
  • unneeded temporary vars

[Spiritreader]

Yes, but with the constraint of 'only where it makes sense."

The code should follow a standard, but also remain semantically readable and flexible enough where a complex condition that needs to be added to a block of logic doesn't end up in refactoring a switch statement back into an if else block.

Ternary ops sometimes don't simplify code in terms of readability.

There's a difference between code style and semantic style and the latter needs to be thoroughly evaluated.

[Jay-o-Way]

    // Service.cs
    private AdmConfigBuilder Builder { get; } = AdmConfigBuilder.Instance();
    private readonly AdmConfigBuilder builder = AdmConfigBuilder.Instance();

That is a really bad oversight, the lowercase builder should not be used.

~ Sam

#1004 (comment)

[Jay-o-Way]

How, exactly, do we want to split this across PRs?

[Spiritreader]

Backend refactoring will probably not take place for another couple of months. We'll worry about it then.

It's a large project and I need to sign off on every single change because it could potentially break theme switching. Stakes are a lot higher here than breaking UI functionality.

There's probably gonna be a groupings based on impact.

• Syntax changes (low impact)
• Logic refactorings (high impact)

[Spiritreader]

To provide a quick update for this.

We'll now monitor the 11 release for a while so we can tackle any breaking issue that arrive.
There may have to be an interim maintenance hotfix release 11.0.1.

After that, we can go through every backend file one by one and start with syntax refactoring (no functional or semantic changes), as in modernizing to leverage latest C# language features.

I want to go through each file one at a time (only one active PR at a time) to preserve the ability to keep track of changes. Also some files host quite extensive logic.

[Jay-o-Way]

File by file? 😮 Oof, that means a lot or PRs... But okay. How (and when - before or after release of 11.1) would you like to start?

(Edit)
Realize that this is not always possible. If the name of something changes, that often affects multiple files.

[Armin2208]

A refactor will happen earliest after 11.1 - maybe not at all. Because resources are pretty limited currently.

[Jay-o-Way]

I'm happy to help! 😁

[Jay-o-Way]

More examples:

• SystemAreasViewModel > if (_isInitializing | !AllowTaskbarColorSwitch) (must be ||)

• Simplify/consolidate redundant try { _builder.Save(); } catch (Exception ex) { ... } blocks (with a helper per viewmodel)

  • (re-)use SafeSaveBuilder??

• ThemePickerViewModel > WriteSettings: repeated If statements

• Check if SystemTimeFormatAsync is obsolete?

• I prefer all the if (_isInitializing) {return;} blocks to be one line

• MessageParser.cs: In some places SendResponse is followed by .ToString()) and in some places it is not.

• In many ViewModel files, a function/method with the name "RequestThemeSwitch" is declared. This should be consolidated.

• suggested: SYSLIB1054 ("Use LibraryImportAttribute instead of DllImportAttribute")

• HttpClientProgressExtensions.cs floats in folder "AutoDarkModeSvc" (is related to UpdateHandler)

• in LocationData.cs: default sunrise/sunset values are 7 and 19 hours, in AdmConfig.cs defaults are 7 and 20 hours.

• Within "Modules" folder and different files I see:

    private AdmConfigBuilder ConfigBuilder { get; }
    private readonly AdmConfigBuilder Builder = AdmConfigBuilder.Instance();
    private AdmConfigBuilder Builder { get; } = AdmConfigBuilder.Instance();
    AdmConfigBuilder Builder { get; } = AdmConfigBuilder.Instance();

Which one is best?

• Typo or not?

[ChenYiLins]

The part you mentioned should be related to the frontend, not the backend.

[Jay-o-Way]

What part?