Skip to content

CmdPal: Refactor codebase for easier AOT support #40113

Open
0 of 1 issue completed
Open
Feature
0 of 1 issue completed
@zadjii-msft

Description

@zadjii-msft

Turns out getting all of CmdPal to be fully AOT is a Hard Problem.

Fear not! I don't think we immediately need to do all the AOT work now. I think we can bifurcate the codebase, and that'll get us to the point where we can have

  • a core UI "control", which fully supports AOT
  • the whole app, which doesn't yet

Basically, we'll make two halves of the codebase: Microsoft.CmdPal.Core, and Microsoft.CmdPal. .Core is fully AOT, it's minimal (basically just a control). Then the .CmdPal namespace is the whole app - with extensions, windowing, settings - the works

Here's a vague mermaid diagram of that idea:

---
config:
  theme: redux
---
flowchart TD

    subgraph ui["Microsoft.CmdPal"]
        subgraph s1["Hard to AOT"]
            n1["Microsoft.CmdPal.VM"]
            n4["Microsoft.CmdPal.UI"]
        end
        settings ---> n4
        n4 ---> CmdPalApp["Command Palette"]

        subgraph settings["Settings"]
            aliases["Aliases"]
            hotkeys["Hotkeys"]
        end

    end

    subgraph Microsoft.CmdPal.Extensions["Microsoft.CmdPal.Extensions"]
        extensions["Extension Service"] ---> n4
        extensions -----> CmdPalExtensionsNuget
    end

    subgraph core["Microsoft.CmdPal.Core"]
        subgraph s2["Easy to AOT"]
            n5["Microsoft.CmdPal.Core.VM"]
            n6["Microsoft.CmdPal.Core.UI"]
        end

        n6 ----> CmdPalControlNuget["CmdPalControl"]
    end


    subgraph s3["Extenal Dependencies"]
        n7["MarkdownTextBlock"]
        n8["AdaptiveCards"]
    end

    n7 ---> n4
    n8 ---> n1 & n4
    n1 --> n4
    n5 --> n6 & n1
    n6 --> n4
    n5@{ shape: rect}
    n6@{ shape: rect}
    n8@{ shape: rect}
    CmdPalControlNuget@{ shape: hex}
    CmdPalApp@{ shape: hex}
    CmdPalExtensionsNuget@{ shape: hex}
Loading

We can leave extension loading outside of the Core namespace. We can probably make that its own package though.


Big rocks

  • ShellPage.xaml.cs very dangerously has a TON of VM stuff in it. It shouldn't. Move that stuff into ShellViewModel.
  • ShellViewModel has a hard dependency on MainListPage. It needs to not
  • There needs to be a version of ShellViewModel in the core that can load ListPages, but also other types of pages
    • I'm thinking that CoreShellViewModel is unsealed, with a virtual Microsoft.CmdPal.Core.ViewModel.IPageViewModel GetViewModelForPage(Microsoft.CommandPalette.Extensions.IPage page) which can be overridden based on a subclass. That'll let consumers add their own page types
    • IContentPage by itself is actually okay
    • CmdPal: Add a viewmodel factory for pages #40504
  • Similar to the above, but with the ShellPage.xaml.cs view. Need to be able to specify your own XAML views given a viewmodel that you implemented
    • Probably needs to be implemented as a IService which can produce a MUX.Page for a given IPageViewModel
    • This will give us the ability to create a DifferentContentPage that would have different templated for the different content types. The content views can all be out of the core.
    • Actually you know what: I don't think we need this. If someone is swapping out all of ShellPage.xaml.cs, then they would have to implement their own NavigateToPage however they want. With whatever views they want. I think this is fine.
  • ContentPageViewModel needs to be unsealed. ViewModelFromContent in ContentPageViewModel needs to be overridable
    • already is!
  • Create a Core.ViewModels project, that doesn't include ContentPages ContentForms.
  • ShellViewModel has a hard dependency on TopLevelViewModel and IExtensionWrapper. It needs to not.
  • Logging needs to be abstracted out. ILoggingService. Good enough for me.
  • Similarly with any telemetry inside of the core. ITelemetryService.
  • Similarly with settings. ISettingsService
  • AliasManager is currrently in ViewModels. It doesn't go into the core
  • Top-level commands have a CommandSettingsViewModel which has a ContentPageViewModel. That's uhoh.
    • I guess IContentPage by itself isn't bad. We could leave that in the core, but leave MarkdownContent and FormContent out of the core.
    • So this is probably fine actually
  • Details uses markdown currently. This is probably the hardest part to separate out.
    • Since that's on the shellpage, and we do want to be able to use it
  • LOCALIZATION. I'm sure we're not doing it The Right Way which we'd need.
    • ResourceLoaderInstance can't be in Core
  • Ye Old Code Quality nits
    • OpenContextMenuMessage shouldn't live in the Core.ViewModels, since it relies a LOT on XAML elements
    • ExtensionObjectViewModel's ctors should be internal
    • CommandContextItemViewModel.cs(41,13,41,50): error CS8073: The result of the expression is always 'true' since a value of type 'KeyChord' is never equal to 'null' of type 'KeyChord?'
    • Core doesn't need to refer to Microsoft.CmdPal.Common at all
    • All the ItemsRepeaters do not like us.
      • The StatusMessages on CommandBar.xaml
      • the Tags on ListItem: ListPage.ListPage_obj3_Bindings.Update_Tags

Sub-issues

Metadata

Metadata

Assignees

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions