refactor!: remove ProjectService/EditorService/ExtensionProvider singletons#1982
Open
yuto-trd wants to merge 4 commits into
Open
refactor!: remove ProjectService/EditorService/ExtensionProvider singletons#1982yuto-trd wants to merge 4 commits into
yuto-trd wants to merge 4 commits into
Conversation
BeutlApiApplication now receives ExtensionProvider from the composition root instead of registering ExtensionProvider.Current directly. This localizes global access to app entry points and lets tests construct an isolated provider. The app and package tools now pass their provider explicitly. Refs: Project #9 "AI Review" item "設計改善: グローバル singleton / service locator を減らす" BREAKING CHANGE: Beutl.Api's BeutlApiApplication constructor now requires an ExtensionProvider argument. Create or pass the desired provider from the composition root, for example ExtensionProvider.Current in the Beutl app.
📝 WalkthroughWalkthroughThe PR replaces static service access with injected ChangesService injection and editor context flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
…letons Replace the global ProjectService.Current, EditorService.Current, and ExtensionProvider.Current singletons with constructor injection threaded from the composition roots. MainViewModel owns the instances and threads them into child view models, services, views (via DataContext), startup tasks (Startup -> LoadPrimitiveExtensionTask / RestoreLastProjectTask), settings pages (SettingsDialogViewModel), and the per-editor tier: EditViewModel exposes ExtensionProvider/EditorService to DockHostViewModel, OutputViewModel, OutputService and the property editors. Editor contexts are created without the global singletons: EditorExtension .TryCreateContext now receives an IEditorContextServices - a typed host-services seam in Beutl.Extensibility exposing IExtensionProvider - instead of reaching for X.Current. ExtensionProvider implements the new IExtensionProvider. The built-in tutorials receive their services through DefaultTutorialExtension's constructor, built by LoadPrimitiveExtensionTask. Refs: Project #9 "AI Review" item "設計改善: グローバル singleton / service locator を減らす" BREAKING CHANGE: EditorExtension.TryCreateContext gains an IEditorContextServices parameter as its second argument: TryCreateContext(CoreObject obj, IEditorContextServices services, out IEditorContext? context). Implementations must add the parameter; those that need no host services may ignore it. Use services.ExtensionProvider (IExtensionProvider) to query other extensions.
The .editorconfig requires charset = utf-8-bom for C# sources, but five files touched while removing the ProjectService/EditorService/ExtensionProvider singletons lost their BOM during editing. dotnet format --verify-no-changes flagged them with CHARSET errors. Re-add the BOM so the changed set passes the format gate that CI and beutl-reviewer run.
yuto-trd
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Beutl’s extension/service wiring to reduce global singleton/service-locator usage by pushing ExtensionProvider (and related editor-session services) into the composition root and threading them through constructors/resource graphs. It also introduces extensibility-facing abstractions (IExtensionProvider, IEditorContextServices) and updates call sites across the app and tooling to pass dependencies explicitly.
Changes:
- Replace
BeutlApiApplication(HttpClient)withBeutlApiApplication(HttpClient, ExtensionProvider)and register the provided provider instance in the API app’s resource graph. - Remove/avoid
*.Currentsingletons in several UI/service paths by injectingExtensionProvider,ProjectService, andEditorServicefromMainViewModel(composition root), and passing host services toEditorExtension.TryCreateContext. - Add unit tests verifying
ExtensionProviderimplementsIExtensionProviderand that the injected provider is reused byPackageManager.
Reviewed changes
Copilot reviewed 81 out of 81 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/PackageSample/SampleEditorExtension.cs | Updates sample editor extension to new TryCreateContext(..., IEditorContextServices, ...) signature. |
| tests/Beutl.UnitTests/Beutl.UnitTests.csproj | Adds Beutl.Api project reference for new API-related tests. |
| tests/Beutl.UnitTests/Api/ExtensionProviderTests.cs | Adds tests for IExtensionProvider surface behavior. |
| tests/Beutl.UnitTests/Api/BeutlApiApplicationTests.cs | Adds test ensuring injected ExtensionProvider is registered/reused in API resource graph. |
| src/Beutl/Views/Tools/OutputView.axaml.cs | Switches file-type lookup to instance OutputViewModel method. |
| src/Beutl/Views/Tools/OutputTab.axaml.cs | Uses OutputTabViewModel to obtain extensions (no static/global access). |
| src/Beutl/Views/MainView.axaml.InitializeMenuBar.cs | Wires dialog VMs and editor/project/extension services via MainViewModel instead of globals. |
| src/Beutl/Views/MainView.axaml.cs | Passes EditorContextServices to EditorExtension.TryCreateContext and removes EditorService.Current usage. |
| src/Beutl/Views/MacWindow.axaml.cs | Mirrors MainView menu/service injection changes for macOS native menu wiring. |
| src/Beutl/Views/Dialogs/CreateNewScene.axaml | Removes XAML-created DataContext to allow constructor-injected VM wiring. |
| src/Beutl/Views/Dialogs/CreateNewProject.axaml | Removes XAML-created DataContext to allow constructor-injected VM wiring. |
| src/Beutl/ViewModels/Tools/OutputViewModel.cs | Threads ExtensionProvider into encoder settings VMs and removes ExtensionProvider.Current usage. |
| src/Beutl/ViewModels/Tools/OutputTabViewModel.cs | Exposes output extensions via instance method to avoid removed singleton access. |
| src/Beutl/ViewModels/SettingsPages/KeyMapSettingsPageViewModel.cs | Injects ExtensionProvider to resolve extensions for keymap grouping. |
| src/Beutl/ViewModels/SettingsPages/ExtensionsSettingsPageViewModel.cs | Injects ExtensionProvider and threads it into child settings VMs. |
| src/Beutl/ViewModels/SettingsPages/EditorExtensionPriorityPageViewModel.cs | Injects ExtensionProvider for editor-extension priority display mapping. |
| src/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cs | Injects ExtensionProvider into property editor matching for extension settings. |
| src/Beutl/ViewModels/SettingsDialogViewModel.cs | Requires ExtensionProvider so settings sub-pages can avoid singleton lookups. |
| src/Beutl/ViewModels/MenuBarViewModel.View.cs | Converts reset handler to instance method (removes reliance on globals). |
| src/Beutl/ViewModels/MenuBarViewModel.Scene.cs | Uses injected _projectService / _editorService instead of *.Current. |
| src/Beutl/ViewModels/MenuBarViewModel.Files.cs | Uses injected services; makes OpenFileCore instance-based. |
| src/Beutl/ViewModels/MenuBarViewModel.cs | Adds constructor injection for ProjectService and EditorService. |
| src/Beutl/ViewModels/MainViewModel.cs | Becomes the composition root for editor-session services and passes them down. |
| src/Beutl/ViewModels/ExtensionsPageViewModel.cs | Threads editor/project services into discover/library page creation. |
| src/Beutl/ViewModels/ExtensionsPages/RemoteUserPackageViewModel.cs | Uses instance PackageOperationHandler (with injected services) for project-close checks. |
| src/Beutl/ViewModels/ExtensionsPages/PackageOperationHandler.cs | Removes static global access; operates via injected ProjectService/EditorService. |
| src/Beutl/ViewModels/ExtensionsPages/LocalUserPackageViewModel.cs | Same handler/service threading as remote package VM. |
| src/Beutl/ViewModels/ExtensionsPages/LibraryPageViewModel.cs | Threads editor/project services into per-package VMs. |
| src/Beutl/ViewModels/ExtensionsPages/DiscoverPageViewModel.cs | Threads editor/project services into discover page factory. |
| src/Beutl/ViewModels/ExtensionsPages/DiscoverPages/PackageDetailsPageViewModel.cs | Threads editor/project services into package operation handler. |
| src/Beutl/ViewModels/ExtensionsPages/DiscoverPages/DataContextFactory.cs | Extends factory to carry editor/project services to created pages. |
| src/Beutl/ViewModels/EncoderSettingsViewModel.cs | Injects ExtensionProvider for property editor matching (no singleton). |
| src/Beutl/ViewModels/EditViewModel.cs | Accepts injected ExtensionProvider/EditorService; provides non-singleton property editor factories. |
| src/Beutl/ViewModels/Editors/TransformEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/Editors/TextureSourceEditorViewModel.cs | Gates child-context creation on injected provider availability. |
| src/Beutl/ViewModels/Editors/PropertiesEditorViewModel.cs | Requires ExtensionProvider and uses it for property editor matching. |
| src/Beutl/ViewModels/Editors/PenEditorViewModel.cs | Gates eager subscriptions on provider injection; passes provider to matching. |
| src/Beutl/ViewModels/Editors/PathOperationEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/Editors/PathFigureEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider and predicates. |
| src/Beutl/ViewModels/Editors/ListEditorViewModel.cs | Routes list-item property editor matching through injected IPropertyEditorFactory. |
| src/Beutl/ViewModels/Editors/GraphModelNodeMemberViewModel.cs | Uses injected IPropertyEditorFactory rather than static matching. |
| src/Beutl/ViewModels/Editors/GraphModelEditorViewModel.cs | Gates construction on provider injection to avoid premature factory resolution. |
| src/Beutl/ViewModels/Editors/GeometryEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/Editors/FilterEffectEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/Editors/DisplacementMapTransformEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cs | Gates child-context creation on provider injection and disposes previous values. |
| src/Beutl/ViewModels/Editors/BrushEditorViewModel.cs | Gates child-context creation on provider injection and disposes previous values. |
| src/Beutl/ViewModels/Editors/BaseEditorViewModel.cs | Adds injected-provider plumbing (GetExtensionProvider / ObserveExtensionProvider) populated via Accept. |
| src/Beutl/ViewModels/Editors/AudioEffectEditorViewModel.cs | Creates PropertiesEditorViewModel with injected provider. |
| src/Beutl/ViewModels/EditorHostViewModel.cs | Accepts injected ProjectService/EditorService instead of using globals. |
| src/Beutl/ViewModels/DockHostViewModel.cs | Uses injected EditViewModel.ExtensionProvider for tool-tab extension discovery/restore. |
| src/Beutl/ViewModels/Dialogs/CreateNewSceneViewModel.cs | Injects project/editor services for scene creation and activation. |
| src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs | Injects ProjectService for project creation. |
| src/Beutl/ViewModels/Dialogs/AddOutputProfileViewModel.cs | Uses OutputTabViewModel.GetExtensions rather than static lookup. |
| src/Beutl/ViewModels/CommandPaletteViewModel.cs | Uses injected EditorService for active-tab tracking. |
| src/Beutl/Services/Tutorials/TutorialHelpers.cs | Removes globals by taking ProjectService/EditorService parameters. |
| src/Beutl/Services/Tutorials/TimelineBasicsTutorial.cs | Wires tutorial prerequisites/actions via injected services. |
| src/Beutl/Services/Tutorials/AnimationEditTutorial.cs | Wires tutorial prerequisites/actions via injected services. |
| src/Beutl/Services/StartupTasks/Startup.cs | Threads editor/project services to tasks that need them; updates task construction. |
| src/Beutl/Services/StartupTasks/RestoreLastProjectTask.cs | Uses injected ProjectService for restore logic. |
| src/Beutl/Services/StartupTasks/LoadPrimitiveExtensionTask.cs | Stops using ExtensionProvider.Current; builds tutorial extension with injected services. |
| src/Beutl/Services/PropertyEditorService.cs | Requires an explicit provider for matching property editor extensions. |
| src/Beutl/Services/ProjectService.cs | Removes ProjectService.Current singleton. |
| src/Beutl/Services/PrimitiveImpls/SceneEditorExtension.cs | Updates editor context creation to accept host services and inject into EditViewModel. |
| src/Beutl/Services/PrimitiveImpls/ExtensionsToolWindowExtension.cs | Creates Extensions page VM with injected editor/project services. |
| src/Beutl/Services/PrimitiveImpls/DefaultTutorialExtension.cs | Replaces static singleton with constructor-injected services. |
| src/Beutl/Services/OutputService.cs | Removes EditorService.Current/ExtensionProvider.Current usage via injected fields/params. |
| src/Beutl/Services/EditorService.cs | Removes EditorService.Current; injects ExtensionProvider and passes IEditorContextServices to extensions. |
| src/Beutl/Services/EditorContextServices.cs | Adds host implementation of IEditorContextServices to pass to editor extensions. |
| src/Beutl/Services/CommandPaletteService.cs | Injects editor/extension services and removes global lookups. |
| src/Beutl/Services/CommandPaletteHandlerProvider.cs | Injects EditorService to resolve context handlers without globals. |
| src/Beutl/Services/Adapters/PropertyEditorFactoryAdapter.cs | Removes static singleton factory; captures provider instance for matching. |
| src/Beutl/Services/Adapters/PropertiesEditorFactoryImpl.cs | Removes static singleton; captures provider instance for creating editor VMs. |
| src/Beutl/Pages/SettingsPages/ExtensionsSettingsPage.axaml | Removes XAML-created DataContext to support injected VM creation. |
| src/Beutl/App.axaml.cs | Removes tutorial wiring that depended on static singleton tutorial extension. |
| src/Beutl.PackageTools.UI/ViewModels/MainViewModel.cs | Updates package tools to pass an explicit ExtensionProvider into BeutlApiApplication. |
| src/Beutl.Extensibility/IExtensionProvider.cs | Introduces read-only extension-provider abstraction for extension authors. |
| src/Beutl.Extensibility/IEditorContextServices.cs | Introduces host-services abstraction for EditorExtension.TryCreateContext. |
| src/Beutl.Extensibility/EditorExtension.cs | Updates editor context creation API to accept host services. |
| src/Beutl.Api/Services/ExtensionProvider.cs | Implements IExtensionProvider and removes ExtensionProvider.Current singleton. |
| src/Beutl.Api/BeutlApiApplication.cs | Injects ExtensionProvider via ctor and registers it in the resource graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f72dad53a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Beutl/Services/StartupTasks/LoadPrimitiveExtensionTask.cs (1)
60-82: 📐 Maintainability & Code Quality | 🟡 MinorAdd NUnit coverage for the injected primitive extension set
LoadPrimitiveExtensionTasknow appendsDefaultTutorialExtensionand registersallExtensions, but there’s no NUnit test undertests/covering that wiring. Add a test that asserts the tutorial extension is included and built with the providedEditorServiceandProjectService.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/StartupTasks/LoadPrimitiveExtensionTask.cs` around lines 60 - 82, Add NUnit coverage for LoadPrimitiveExtensionTask’s primitive extension wiring by testing that allExtensions includes DefaultTutorialExtension and that it is constructed with the provided EditorService and ProjectService. Use the LoadPrimitiveExtensionTask constructor as the entry point, and assert the injected services are the ones passed through to the tutorial extension when provider.AddExtensions is reached.Source: Coding guidelines
🧹 Nitpick comments (3)
src/Beutl.Api/BeutlApiApplication.cs (1)
55-61: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding
httpClienttoo.
extensionProvidergetsArgumentNullException.ThrowIfNull, buthttpClientis dereferenced immediately at Line 61 (httpClient.BaseAddress). A nullhttpClientthrows an opaqueNullReferenceExceptiondeep in the constructor instead of a clear argument error. Adding a matching guard keeps the contract consistent.♻️ Proposed guard
public BeutlApiApplication(HttpClient httpClient, ExtensionProvider extensionProvider) { + ArgumentNullException.ThrowIfNull(httpClient); ArgumentNullException.ThrowIfNull(extensionProvider); _httpClient = httpClient; _extensionProvider = extensionProvider;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Api/BeutlApiApplication.cs` around lines 55 - 61, The constructor in BeutlApiApplication currently guards extensionProvider but not httpClient, so a null client will fail later when BaseAddress is assigned. Add a matching null check for httpClient at the start of the BeutlApiApplication(HttpClient httpClient, ExtensionProvider extensionProvider) constructor, alongside the existing ArgumentNullException.ThrowIfNull call, so both dependencies fail fast with a clear argument error.src/Beutl/ViewModels/EditViewModel.cs (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider null-guarding the injected services for fail-fast consistency.
EditorService(this PR) guards its injectedExtensionProviderwithArgumentNullException.ThrowIfNull. TheEditViewModelconstructor storesextensionProvider/editorServicewithout a guard, so a null would only surface much later (e.g. when a property editor callsGetExtensionProvider()). A guard here makes the failure point match the rest of the refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/ViewModels/EditViewModel.cs` around lines 55 - 61, Add fail-fast null checks in the EditViewModel constructor before assigning the injected dependencies. Mirror the existing pattern used in EditorService by guarding extensionProvider and editorService (and any other required constructor inputs if needed) with ArgumentNullException.ThrowIfNull so a null is rejected immediately; locate this in the EditViewModel constructor where Scene, ExtensionProvider, and EditorService are initialized.src/Beutl/Services/PropertyEditorService.cs (1)
31-34: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: guard against a null
extensionProvider.
MatchPropertyimmediately dereferencesextensionProvider.GetExtensions<...>(). All current callers pass an injected non-null instance, but anArgumentNullException.ThrowIfNull(extensionProvider)would fail fast with a clearer signal than a raw NRE if a future caller forgets to wire it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/PropertyEditorService.cs` around lines 31 - 34, `PropertyEditorService.MatchProperty` currently dereferences `extensionProvider` immediately via `GetExtensions<PropertyEditorExtension>()`, so add a fail-fast null check at the start of the method; use `ArgumentNullException.ThrowIfNull(extensionProvider)` (or equivalent) before any property/extension lookup so future callers get a clear argument error instead of a null reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Extensibility/EditorExtension.cs`:
- Around line 33-36: Update the public signature change in
EditorExtension.TryCreateContext to be explicitly documented as a breaking
extensibility change, since adding IEditorContextServices affects all
out-of-tree overrides. Add a refactor!/BREAKING CHANGE note near the
TryCreateContext declaration and include a short migration hint telling
extension authors to update their EditorExtension overrides to accept the new
services parameter and use it when creating IEditorContext instances.
In `@src/Beutl/ViewModels/SettingsPages/KeyMapSettingsPageViewModel.cs`:
- Around line 58-67: The KeyMapSettingsPageViewModel constructor currently
assumes every command definition’s ExtensionType exists in
ExtensionProvider.AllExtensions via First, which can throw when they drift.
Update the grouping logic in KeyMapSettingsPageViewModel to safely handle
missing injected extensions by skipping unmatched groups or otherwise guarding
the lookup instead of throwing. Add a matching NUnit test under tests/ for this
constructor behavior to verify Key Map no longer crashes when a
ContextCommandManager definition has no corresponding extension.
---
Outside diff comments:
In `@src/Beutl/Services/StartupTasks/LoadPrimitiveExtensionTask.cs`:
- Around line 60-82: Add NUnit coverage for LoadPrimitiveExtensionTask’s
primitive extension wiring by testing that allExtensions includes
DefaultTutorialExtension and that it is constructed with the provided
EditorService and ProjectService. Use the LoadPrimitiveExtensionTask constructor
as the entry point, and assert the injected services are the ones passed through
to the tutorial extension when provider.AddExtensions is reached.
---
Nitpick comments:
In `@src/Beutl.Api/BeutlApiApplication.cs`:
- Around line 55-61: The constructor in BeutlApiApplication currently guards
extensionProvider but not httpClient, so a null client will fail later when
BaseAddress is assigned. Add a matching null check for httpClient at the start
of the BeutlApiApplication(HttpClient httpClient, ExtensionProvider
extensionProvider) constructor, alongside the existing
ArgumentNullException.ThrowIfNull call, so both dependencies fail fast with a
clear argument error.
In `@src/Beutl/Services/PropertyEditorService.cs`:
- Around line 31-34: `PropertyEditorService.MatchProperty` currently
dereferences `extensionProvider` immediately via
`GetExtensions<PropertyEditorExtension>()`, so add a fail-fast null check at the
start of the method; use `ArgumentNullException.ThrowIfNull(extensionProvider)`
(or equivalent) before any property/extension lookup so future callers get a
clear argument error instead of a null reference.
In `@src/Beutl/ViewModels/EditViewModel.cs`:
- Around line 55-61: Add fail-fast null checks in the EditViewModel constructor
before assigning the injected dependencies. Mirror the existing pattern used in
EditorService by guarding extensionProvider and editorService (and any other
required constructor inputs if needed) with ArgumentNullException.ThrowIfNull so
a null is rejected immediately; locate this in the EditViewModel constructor
where Scene, ExtensionProvider, and EditorService are initialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85cbde9e-b006-4fcc-901d-21ceb4e8e29d
📒 Files selected for processing (81)
src/Beutl.Api/BeutlApiApplication.cssrc/Beutl.Api/Services/ExtensionProvider.cssrc/Beutl.Extensibility/EditorExtension.cssrc/Beutl.Extensibility/IEditorContextServices.cssrc/Beutl.Extensibility/IExtensionProvider.cssrc/Beutl.PackageTools.UI/ViewModels/MainViewModel.cssrc/Beutl/App.axaml.cssrc/Beutl/Pages/SettingsPages/ExtensionsSettingsPage.axamlsrc/Beutl/Services/Adapters/PropertiesEditorFactoryImpl.cssrc/Beutl/Services/Adapters/PropertyEditorFactoryAdapter.cssrc/Beutl/Services/CommandPaletteHandlerProvider.cssrc/Beutl/Services/CommandPaletteService.cssrc/Beutl/Services/EditorContextServices.cssrc/Beutl/Services/EditorService.cssrc/Beutl/Services/OutputService.cssrc/Beutl/Services/PrimitiveImpls/DefaultTutorialExtension.cssrc/Beutl/Services/PrimitiveImpls/ExtensionsToolWindowExtension.cssrc/Beutl/Services/PrimitiveImpls/SceneEditorExtension.cssrc/Beutl/Services/ProjectService.cssrc/Beutl/Services/PropertyEditorService.cssrc/Beutl/Services/StartupTasks/LoadPrimitiveExtensionTask.cssrc/Beutl/Services/StartupTasks/RestoreLastProjectTask.cssrc/Beutl/Services/StartupTasks/Startup.cssrc/Beutl/Services/Tutorials/AnimationEditTutorial.cssrc/Beutl/Services/Tutorials/TimelineBasicsTutorial.cssrc/Beutl/Services/Tutorials/TutorialHelpers.cssrc/Beutl/ViewModels/CommandPaletteViewModel.cssrc/Beutl/ViewModels/Dialogs/AddOutputProfileViewModel.cssrc/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cssrc/Beutl/ViewModels/Dialogs/CreateNewSceneViewModel.cssrc/Beutl/ViewModels/DockHostViewModel.cssrc/Beutl/ViewModels/EditViewModel.cssrc/Beutl/ViewModels/EditorHostViewModel.cssrc/Beutl/ViewModels/Editors/AudioEffectEditorViewModel.cssrc/Beutl/ViewModels/Editors/BaseEditorViewModel.cssrc/Beutl/ViewModels/Editors/BrushEditorViewModel.cssrc/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cssrc/Beutl/ViewModels/Editors/DisplacementMapTransformEditorViewModel.cssrc/Beutl/ViewModels/Editors/FilterEffectEditorViewModel.cssrc/Beutl/ViewModels/Editors/GeometryEditorViewModel.cssrc/Beutl/ViewModels/Editors/GraphModelEditorViewModel.cssrc/Beutl/ViewModels/Editors/GraphModelNodeMemberViewModel.cssrc/Beutl/ViewModels/Editors/ListEditorViewModel.cssrc/Beutl/ViewModels/Editors/PathFigureEditorViewModel.cssrc/Beutl/ViewModels/Editors/PathOperationEditorViewModel.cssrc/Beutl/ViewModels/Editors/PenEditorViewModel.cssrc/Beutl/ViewModels/Editors/PropertiesEditorViewModel.cssrc/Beutl/ViewModels/Editors/TextureSourceEditorViewModel.cssrc/Beutl/ViewModels/Editors/TransformEditorViewModel.cssrc/Beutl/ViewModels/EncoderSettingsViewModel.cssrc/Beutl/ViewModels/ExtensionsPageViewModel.cssrc/Beutl/ViewModels/ExtensionsPages/DiscoverPageViewModel.cssrc/Beutl/ViewModels/ExtensionsPages/DiscoverPages/DataContextFactory.cssrc/Beutl/ViewModels/ExtensionsPages/DiscoverPages/PackageDetailsPageViewModel.cssrc/Beutl/ViewModels/ExtensionsPages/LibraryPageViewModel.cssrc/Beutl/ViewModels/ExtensionsPages/LocalUserPackageViewModel.cssrc/Beutl/ViewModels/ExtensionsPages/PackageOperationHandler.cssrc/Beutl/ViewModels/ExtensionsPages/RemoteUserPackageViewModel.cssrc/Beutl/ViewModels/MainViewModel.cssrc/Beutl/ViewModels/MenuBarViewModel.Files.cssrc/Beutl/ViewModels/MenuBarViewModel.Scene.cssrc/Beutl/ViewModels/MenuBarViewModel.View.cssrc/Beutl/ViewModels/MenuBarViewModel.cssrc/Beutl/ViewModels/SettingsDialogViewModel.cssrc/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cssrc/Beutl/ViewModels/SettingsPages/EditorExtensionPriorityPageViewModel.cssrc/Beutl/ViewModels/SettingsPages/ExtensionsSettingsPageViewModel.cssrc/Beutl/ViewModels/SettingsPages/KeyMapSettingsPageViewModel.cssrc/Beutl/ViewModels/Tools/OutputTabViewModel.cssrc/Beutl/ViewModels/Tools/OutputViewModel.cssrc/Beutl/Views/Dialogs/CreateNewProject.axamlsrc/Beutl/Views/Dialogs/CreateNewScene.axamlsrc/Beutl/Views/MacWindow.axaml.cssrc/Beutl/Views/MainView.axaml.InitializeMenuBar.cssrc/Beutl/Views/MainView.axaml.cssrc/Beutl/Views/Tools/OutputTab.axaml.cssrc/Beutl/Views/Tools/OutputView.axaml.cstests/Beutl.UnitTests/Api/BeutlApiApplicationTests.cstests/Beutl.UnitTests/Api/ExtensionProviderTests.cstests/Beutl.UnitTests/Beutl.UnitTests.csprojtests/PackageSample/SampleEditorExtension.cs
💤 Files with no reviewable changes (4)
- src/Beutl/Pages/SettingsPages/ExtensionsSettingsPage.axaml
- src/Beutl/Views/Dialogs/CreateNewProject.axaml
- src/Beutl/Views/Dialogs/CreateNewScene.axaml
- src/Beutl/Services/ProjectService.cs
…n-scene hosts Address PR review findings on the service-locator removal: - Property editors hosted outside a scene tab (encoder settings, extension settings) no longer received the ExtensionProvider or IPropertyEditorFactory, so nested object/list editors threw or failed to render when expanded. The hosts now expose these via GetService, and BaseEditorViewModel resolves the provider from the service chain instead of only via EditViewModel. - KeyMapSettingsPageViewModel used First() on the extension lookup, which threw when a command definition's ExtensionType had no registered extension; use FirstOrDefault and skip unmatched groups. - PropertyEditorService.MatchProperty now takes IExtensionProvider to depend on the abstraction rather than the concrete API-layer type. - Add fail-fast ArgumentNullException guards (BeutlApiApplication.httpClient, EditViewModel ctor, MatchProperty) with tests for the API constructor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cba4ac1a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var visitor = new Visitor(this); | ||
| var itemAccessor = new ListItemAccessorImpl<TItem?>(index, obj, List.Value!); | ||
| var item = new ListItemEditorViewModel<TItem>(this, itemAccessor); | ||
| var item = new ListItemEditorViewModel<TItem>(this, itemAccessor, this.GetRequiredService<IPropertyEditorFactory>()); |
There was a problem hiding this comment.
When a nested list editor is initialized already expanded (for example the audio/filter/transform/path group editors create child ListEditorViewModels with IsExpanded = true before AcceptChild() accepts the child), the expansion subscription immediately replays the current list and reaches this line before _parentServices has been set by Accept. Because the new factory lookup depends on that service chain, expanding a group that already has items throws instead of rendering its children. This is fresh evidence beyond the settings-host comment: the same diff creates pre-Accept expanded child lists in normal scene editors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cs (1)
57-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winResolve the provider by its abstraction as well.
This service host should return the injected provider when callers request
IExtensionProvider, not just the concreteExtensionProvider.Proposed fix
- if (serviceType == typeof(ExtensionProvider)) + if (serviceType == typeof(ExtensionProvider) || serviceType == typeof(IExtensionProvider)) return _extensionProvider;As per coding guidelines,
**/*.cs: New logic must ship with a NUnit test undertests/in the matching test project.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cs` around lines 57 - 58, The service resolution in AnExtensionSettingsPageViewModel only handles the concrete ExtensionProvider type, so callers asking for IExtensionProvider are not satisfied. Update the provider lookup logic in the service-host implementation to return the injected instance for both the abstraction and the concrete type, and add a matching NUnit test under tests/ to cover resolving IExtensionProvider.Source: Coding guidelines
src/Beutl/ViewModels/EncoderSettingsViewModel.cs (1)
45-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpose the provider through
IExtensionProvidertoo.The new provider abstraction can be requested through the service chain, but this host currently only resolves the concrete
ExtensionProvider.Proposed fix
- if (serviceType == typeof(ExtensionProvider)) + if (serviceType == typeof(ExtensionProvider) || serviceType == typeof(IExtensionProvider)) return _extensionProvider;As per coding guidelines,
**/*.cs: New logic must ship with a NUnit test undertests/in the matching test project.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/ViewModels/EncoderSettingsViewModel.cs` around lines 45 - 46, The service resolution in EncoderSettingsViewModel only returns the concrete ExtensionProvider, so extend the existing GetService logic to also return the same instance when the requested type is IExtensionProvider. Update the service-chain handling in the relevant method so both symbols are supported without changing the provider instance. Add a matching NUnit test under tests/ for the corresponding view model/service resolution path to verify IExtensionProvider is resolved successfully.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Beutl/ViewModels/EncoderSettingsViewModel.cs`:
- Around line 45-46: The service resolution in EncoderSettingsViewModel only
returns the concrete ExtensionProvider, so extend the existing GetService logic
to also return the same instance when the requested type is IExtensionProvider.
Update the service-chain handling in the relevant method so both symbols are
supported without changing the provider instance. Add a matching NUnit test
under tests/ for the corresponding view model/service resolution path to verify
IExtensionProvider is resolved successfully.
In `@src/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cs`:
- Around line 57-58: The service resolution in AnExtensionSettingsPageViewModel
only handles the concrete ExtensionProvider type, so callers asking for
IExtensionProvider are not satisfied. Update the provider lookup logic in the
service-host implementation to return the injected instance for both the
abstraction and the concrete type, and add a matching NUnit test under tests/ to
cover resolving IExtensionProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0569f1a1-9835-45b4-ae50-4e9069280b4d
📒 Files selected for processing (8)
src/Beutl.Api/BeutlApiApplication.cssrc/Beutl/Services/PropertyEditorService.cssrc/Beutl/ViewModels/EditViewModel.cssrc/Beutl/ViewModels/Editors/BaseEditorViewModel.cssrc/Beutl/ViewModels/EncoderSettingsViewModel.cssrc/Beutl/ViewModels/SettingsPages/AnExtensionSettingsPageViewModel.cssrc/Beutl/ViewModels/SettingsPages/KeyMapSettingsPageViewModel.cstests/Beutl.UnitTests/Api/BeutlApiApplicationTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Beutl/Services/PropertyEditorService.cs
- src/Beutl/ViewModels/SettingsPages/KeyMapSettingsPageViewModel.cs
- src/Beutl.Api/BeutlApiApplication.cs
- src/Beutl/ViewModels/Editors/BaseEditorViewModel.cs
- src/Beutl/ViewModels/EditViewModel.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Replaces the global
ProjectService.Current,EditorService.Current, andExtensionProvider.Currentsingletons with constructor injection threaded from the composition roots.MainViewModel(andBeutl.PackageTools.UI'sMainViewModel) own the instances and thread them into child view models, services, views (viaDataContext), startup tasks (Startup->LoadPrimitiveExtensionTask/RestoreLastProjectTask), settings pages, and the per-editor tier (EditViewModel->DockHostViewModel/OutputViewModel/OutputService/ property editors).BeutlApiApplicationreceives itsExtensionProviderfrom the composition root instead of resolvingExtensionProvider.Currentinternally.EditorExtension.TryCreateContextnow receives a typedIEditorContextServices— a new host-services seam inBeutl.ExtensibilityexposingIExtensionProvider.ExtensionProviderimplementsIExtensionProvider.DefaultTutorialExtension's constructor (built byLoadPrimitiveExtensionTask); its staticInstanceis removed.Affected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Breaking changes
EditorExtension.TryCreateContextgains anIEditorContextServicesparameter:TryCreateContext(CoreObject obj, IEditorContextServices services, out IEditorContext? context).Implementations must add the parameter; those that need no host services may ignore it. Use
services.ExtensionProvider(IExtensionProvider) to query other extensions.BeutlApiApplication(HttpClient)was replaced withBeutlApiApplication(HttpClient, ExtensionProvider). Callers must pass the provider from their composition root.ProjectService.Current,EditorService.Current, andExtensionProvider.Currentare removed. Obtain these services via constructor injection /IEditorContext.GetRequiredService<...>instead.Test plan
dotnet build Beutl.slnx-> 0 errors.dotnet test tests/Beutl.UnitTests/Beutl.UnitTests.csproj -f net10.0-> 3670 passed, no regressions. (3AutoSaveServiceTestscases fail only on Windows local:CoreSerializer.StoreToUriauto-creates the target directory, so a save to a "non-existent" path succeeds and emits noSaveError. On the Linux CI runner the root path is not creatable, so they pass. Pre-existing and unrelated to this change.)tests/Beutl.UnitTests/Api/ExtensionProviderTests.cs(new) and extendedBeutlApiApplicationTests.cs.dotnet format Beutl.slnx --verify-no-changesover the changed set -> clean (after restoring UTF-8 BOM on 5 files that lost it during editing).Fixed issues / References
*.Currentuse sites toward composition-root injection" is completed by this PR (all three statics removed).Summary by CodeRabbit
New Features
Bug Fixes
Tests