feat(DockView): remove EditorRequired attribute#985
Merged
Conversation
Reviewer's GuideRefactors DockViewV2 local storage configuration handling by removing the EditorRequired requirement from Name, adding runtime validation when local storage is enabled, and consolidating option construction into a clearer DockView configuration method. Class diagram for updated DockViewV2 local storage configurationclassDiagram
class DockViewV2 {
+string~Nullable~ Name
+bool~Nullable~ EnableLocalStorage
+string~Nullable~ Version
+string~Nullable~ LocalStoragePrefix
-bool IsEnableLocalStorage
-string~Nullable~ LocalStorageKey
+void OnInitialized()
+void OnParametersSet()
+Task OnAfterRenderAsync(bool firstRender)
+Task InvokeInitAsync()
-DockViewConfig GetDockViewConfig()
-string GetVersion()
-string GetPrefixKey()
+Task Reset(string~Nullable~ layoutConfig)
}
class DockViewConfig {
+bool EnableLocalStorage
+string~Nullable~ LocalStorageKey
+bool IsLock
+bool ShowLock
+bool IsFloating
+string~Nullable~ LayoutConfig
+string LoadTabs
}
DockViewV2 --> DockViewConfig : builds
Flow diagram for DockViewV2 local storage validation and configurationflowchart TD
A[OnParametersSet called] --> B{IsEnableLocalStorage?}
B -- No --> C[Skip Name validation]
B -- Yes --> D{Name is null or empty?}
D -- Yes --> E[Throw InvalidOperationException]
D -- No --> F[Continue initialization]
subgraph ConfigConstruction[GetDockViewConfig]
G[Compute IsEnableLocalStorage from EnableLocalStorage or options] --> H{IsEnableLocalStorage?}
H -- No --> I[Set LocalStorageKey to null]
H -- Yes --> J[Build LocalStorageKey using GetPrefixKey, Name, GetVersion]
I --> K[Populate DockViewConfig with flags and LocalStorageKey]
J --> K
end
C --> G
F --> G
K --> L[InvokeInitAsync uses GetDockViewConfig]
K --> M[OnAfterRenderAsync update uses GetDockViewConfig]
K --> N[Reset uses GetDockViewConfig]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new InvalidOperationException message is only in English, whereas the surrounding XML comments are bilingual; consider aligning the error message with your localization approach (e.g., bilingual or resource-based).
- The Chinese comment above the OnParametersSet validation contains a likely typo ('本体存储'); it should probably be '本地存储' to match the feature description.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new InvalidOperationException message is only in English, whereas the surrounding XML comments are bilingual; consider aligning the error message with your localization approach (e.g., bilingual or resource-based).
- The Chinese comment above the OnParametersSet validation contains a likely typo ('本体存储'); it should probably be '本地存储' to match the feature description.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Removes the EditorRequired requirement from DockViewV2.Name and replaces the design-time requirement with runtime validation when local-storage persistence is enabled.
Changes:
- Removes
[EditorRequired]from theNameparameter and repositions the parameter definition. - Adds
OnParametersSetvalidation to throw when local storage is enabled butNameis missing. - Refactors option-building logic (
GetOptions→GetDockViewConfig) and centralizes local-storage derived values (IsEnableLocalStorage,LocalStorageKey).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <para lang="en">Gets or sets the DockView name. Default is null and it is used for local storage identification</para> | ||
| /// </summary> | ||
| [Parameter] | ||
| [NotNull] |
There was a problem hiding this comment.
Name is declared as nullable (string?) and is intentionally optional now (only required when local storage is enabled), but it is still annotated with [NotNull]. That attribute communicates to the nullability analyzer that the property getter never returns null, which is no longer true and can lead to incorrect flow analysis. Consider removing [NotNull] (or making Name non-nullable again if it must always be supplied).
Suggested change
| [NotNull] |
| { | ||
| base.OnParametersSet(); | ||
|
|
||
| // 开启本体存储未提供 Name 时抛出异常提示 |
There was a problem hiding this comment.
Typo in the comment: it says “本体存储” but the code and docs refer to “本地存储”. This should be corrected to avoid confusion.
Suggested change
| // 开启本体存储未提供 Name 时抛出异常提示 | |
| // 开启本地存储未提供 Name 时抛出异常提示 |
| // 开启本体存储未提供 Name 时抛出异常提示 | ||
| if (IsEnableLocalStorage && string.IsNullOrEmpty(Name)) | ||
| { | ||
| throw new InvalidOperationException("Name must be provided when local storage is enabled."); |
There was a problem hiding this comment.
The thrown InvalidOperationException message could be more actionable for consumers. Consider including the component/parameter names via nameof(Name)/nameof(EnableLocalStorage) and clarifying that local storage may be enabled either via the parameter or global DockViewOptions, so users know what to change.
Suggested change
| throw new InvalidOperationException("Name must be provided when local storage is enabled."); | |
| throw new InvalidOperationException($"{nameof(Name)} must be provided when local storage is enabled. Local storage can be enabled either by the {nameof(EnableLocalStorage)} parameter or by global {nameof(DockViewOptions)} configuration."); |
|
|
||
| private bool IsEnableLocalStorage => EnableLocalStorage ?? _options.EnableLocalStorage ?? false; | ||
|
|
||
| private string? LocalStorageKey => IsEnableLocalStorage ? $"{GetPrefixKey()}-{Name}-{GetVersion()}" : null; |
There was a problem hiding this comment.
LocalStorageKey is now null when local storage is disabled, but some JS code (e.g. dockview-utils.js) reads options.localStorageKey outside the enableLocalStorage guard. This can cause unexpected access to localStorage keys like null-panels/undefined-panels, and potentially JSON parse errors if those keys exist. Consider always providing a stable non-null key, or updating the JS side to only access localStorageKey when enableLocalStorage is true.
Suggested change
| private string? LocalStorageKey => IsEnableLocalStorage ? $"{GetPrefixKey()}-{Name}-{GetVersion()}" : null; | |
| private string LocalStorageKey => $"{GetPrefixKey()}-{Name}-{GetVersion()}"; |
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.
Link issues
fixes #984
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Relax DockViewV2 parameter requirements while enforcing correct configuration when local storage is enabled.
Bug Fixes:
Enhancements: