Description
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}
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 intoShellViewModel
. -
ShellViewModel
has a hard dependency onMainListPage
. 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 avirtual 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
- I'm thinking that
-
Similar to the above, but with theShellPage.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 aMUX.Page
for a givenIPageViewModel
- 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 ownNavigateToPage
however they want. With whatever views they want. I think this is fine.
- Probably needs to be implemented as a
-
ContentPageViewModel
needs to be unsealed.ViewModelFromContent
inContentPageViewModel
needs to be overridable- already is!
- Create a
Core.ViewModels
project, that doesn't includeContentPage
sContentForm
s.dev/migrie/40113/new-core-project
- see dev/migrie/40113/viewmodel-factory...dev/migrie/40113/new-core-project
- Create a Microsoft.CmdPal.Core.ViewModels project #40560
-
ShellViewModel
has a hard dependency onTopLevelViewModel
andIExtensionWrapper
. It needs to not.- Some notes in
dev/migrie/40113/extension-hosts-are-bodgy
- better prototype in
dev/migrie/40113/extension-hosts-try-2
- see dev/migrie/40113/new-core-project...dev/migrie/40113/extension-hosts-try-2
- This is truly horrific
- We probably need to pull
IExtensionWrapper
andIExtensionService
into their own project first CommandPaletteHost
needs to live inui.vm
, not incore.vm
- We need a better way of resolving the extension host for commands & pages, rather than the horrifying "stick it into the
TopLevelViewModel
and get it out later" that we currently do - CmdPal: Separate AppHosts from _extension_ app hosts #40561
- Some notes in
- Logging needs to be abstracted out.
ILoggingService
. Good enough for me.dev/migrie/b/remove-core-managedcommon-dep
- see dev/migrie/40113/extension-hosts-try-2...dev/migrie/b/remove-core-managedcommon-dep
- Similarly with any telemetry inside of the core.
ITelemetryService
. - Similarly with settings.
ISettingsService
-
AliasManager
is currrently inViewModels
. It doesn't go into the core - Top-level commands have a
CommandSettingsViewModel
which has aContentPageViewModel
. That's uhoh.- I guess
IContentPage
by itself isn't bad. We could leave that in the core, but leaveMarkdownContent
andFormContent
out of the core. - So this is probably fine actually
- I guess
- 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 theCore.ViewModels
, since it relies a LOT on XAML elements -
ExtensionObjectViewModel
's ctors should beinternal
-
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
StatusMessage
s onCommandBar.xaml
- the
Tags
on ListItem:ListPage.ListPage_obj3_Bindings.Update_Tags
- The
-