feat(HikVision): add ChangeWindowNum function #871
Conversation
Reviewer's GuideAdds a ChangeWindowNum viewport-splitting feature to the HikVision web plugin, exposes it through JS interop and C# API, and adjusts initialization and playback guards to support multiple-window layouts. Sequence diagram for the new ChangeWindowNum window layout changesequenceDiagram
actor User
participant HikVisionWebPlugin as HikVisionWebPlugin_CSharp
participant JSRuntime as BlazorJSRuntime
participant HikVisionWebPluginJs as HikVisionWebPlugin_js
participant HikvisionJs as hikvision_js
participant WebVideoCtrl
User->>HikVisionWebPlugin: ChangeWindowNum(iWndType)
activate HikVisionWebPlugin
HikVisionWebPlugin->>HikVisionWebPlugin: IsMultipleWindowType = (iWndType != 1)
HikVisionWebPlugin->>JSRuntime: InvokeAsync changeWndNum(Id, iWndType)
activate JSRuntime
JSRuntime->>HikVisionWebPluginJs: changeWndNum(Id, iWndType)
activate HikVisionWebPluginJs
HikVisionWebPluginJs->>HikvisionJs: changeWndNum(Id, iWndType)
activate HikvisionJs
alt iWndType is 1*2 or 2*1
HikvisionJs->>WebVideoCtrl: I_ArrangeWindow(iWndType)
else other layout type
HikvisionJs->>WebVideoCtrl: I_ChangeWndNum(parseInt(iWndType))
end
WebVideoCtrl-->>HikvisionJs: operation result
HikvisionJs-->>HikVisionWebPluginJs: ret (bool)
deactivate HikvisionJs
HikVisionWebPluginJs-->>JSRuntime: ret (bool)
deactivate HikVisionWebPluginJs
JSRuntime-->>HikVisionWebPlugin: ret (bool)
deactivate JSRuntime
HikVisionWebPlugin-->>User: ret (bool)
deactivate HikVisionWebPlugin
Updated class diagram for HikVisionWebPlugin and JS interopclassDiagram
class HikVisionWebPlugin {
string Id
bool IsLogin
bool IsRealPlaying
bool IsStartRecord
bool IsMultipleWindowType
Task StartRealPlay(int streamType, int channelId)
Task StopRealPlay()
Task~bool~ OpenSound()
Task~bool~ CloseSound()
Task~bool~ SetVolume(int value)
Task~bool~ CapturePictureAndDownload(string fileName, CancellationToken token)
Task~bool~ CapturePicture(string fileName, CancellationToken token)
Task~bool~ StartRecord()
Task~bool~ StopRecord()
Task~bool~ ChangeWindowNum(string iWndType)
}
class HikVisionWebPlugin_js {
+init(id, invoke)
+login(id, ip, port, userName, password, loginType)
+logout(id)
+startRealPlay(id, streamType, channelId)
+stopRealPlay(id)
+openSound(id)
+closeSound(id)
+setVolume(id, value)
+capturePicture(id, fileName)
+capturePictureAndDownload(id, fileName)
+startRecord(id)
+stopRecord(id)
+changeWndNum(id, iWndType)
+dispose(id)
}
class hikvision_js {
+init(id)
+login(id, ip, port, userName, password, loginType)
+logout(id)
+startRealPlay(id, streamType, channelId)
+stopRealPlay(id)
+openSound(id)
+closeSound(id)
+setVolume(id, value)
+capturePicture(id, fileName)
+capturePictureAndDownload(id, fileName)
+startRecord(id)
+stopRecord(id)
+changeWndNum(id, iWndType)
}
HikVisionWebPlugin --> HikVisionWebPlugin_js : JSInterop InvokeAsync
HikVisionWebPlugin_js --> hikvision_js : ES module import/export
class WebVideoCtrl {
+I_InitPlugin(options)
+I_ArrangeWindow(iWndType)
+I_ChangeWndNum(iType)
}
hikvision_js --> WebVideoCtrl : calls layout APIs
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 found 2 issues, and left some high level feedback:
- In
changeWndNumthevisionvariable retrieved fromData.get(id)is never used, so it can be removed to avoid confusion and make it clear that this function is stateless w.r.t. the stored vision data. - The new
IsMultipleWindowTypeflag weakens many guards (e.g.,StartRealPlay,OpenSound,StartRecord) so that operations can run even when not logged in or not currently playing; please double‑check whether login/real‑play preconditions should still be enforced when in multi‑window mode and encapsulate that logic in a single helper if possible. - The XML doc for
ChangeWindowNumdescribes only numericiWndTypevalues (1–4), but the implementation also supports string patterns like"1*2"and"2*1"; consider aligning the parameter type/description with the accepted formats to avoid misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `changeWndNum` the `vision` variable retrieved from `Data.get(id)` is never used, so it can be removed to avoid confusion and make it clear that this function is stateless w.r.t. the stored vision data.
- The new `IsMultipleWindowType` flag weakens many guards (e.g., `StartRealPlay`, `OpenSound`, `StartRecord`) so that operations can run even when not logged in or not currently playing; please double‑check whether login/real‑play preconditions should still be enforced when in multi‑window mode and encapsulate that logic in a single helper if possible.
- The XML doc for `ChangeWindowNum` describes only numeric `iWndType` values (1–4), but the implementation also supports string patterns like `"1*2"` and `"2*1"`; consider aligning the parameter type/description with the accepted formats to avoid misuse.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.HikVision/Components/HikVisionWebPlugin.razor.cs:207-210` </location>
<code_context>
/// <returns></returns>
public async Task StartRealPlay(int streamType, int channelId)
{
- if (IsLogin && !IsRealPlaying)
+ if (IsMultipleWindowType || (IsLogin && !IsRealPlaying))
{
IsRealPlaying = await InvokeAsync<bool?>("startRealPlay", Id, streamType, channelId) ?? false;
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-check whether bypassing login/real-play checks when `IsMultipleWindowType` is true is intentional.
The new condition allows `StartRealPlay` (and similar methods) to run based only on `IsMultipleWindowType`, even when `IsLogin` is false or `IsRealPlaying` is true, changing the previous invariant that guarded against invalid states. If `IsMultipleWindowType` is only about layout, this may trigger real-play operations (e.g., opening sound) while not logged in. If it represents a special mode with different rules, consider encapsulating the conditions in something like `CanStartRealPlay()` so the intended state checks are explicit and consistently reused.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.HikVision/Components/HikVisionWebPlugin.razor.cs:212` </location>
<code_context>
public async Task StartRealPlay(int streamType, int channelId)
{
- if (IsLogin && !IsRealPlaying)
+ if (IsMultipleWindowType || (IsLogin && !IsRealPlaying))
{
IsRealPlaying = await InvokeAsync<bool?>("startRealPlay", Id, streamType, channelId) ?? false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated `IsMultipleWindowType`-related conditions into dedicated helper properties and using those at call sites to centralize the rules and simplify each method.
You can reduce the added complexity by centralizing the new `IsMultipleWindowType` logic into helper properties so the rules live in one place instead of being duplicated across many methods.
For example:
```csharp
private bool CanStartRealPlay => IsMultipleWindowType || (IsLogin && !IsRealPlaying);
private bool CanStopRealPlay => IsMultipleWindowType || IsRealPlaying;
private bool CanControlPlayback => IsMultipleWindowType || (IsLogin && IsRealPlaying);
```
Then use these in the call sites:
```csharp
public async Task StartRealPlay(int streamType, int channelId)
{
if (!CanStartRealPlay)
{
return;
}
IsRealPlaying = await InvokeAsync<bool?>("startRealPlay", Id, streamType, channelId) ?? false;
if (IsRealPlaying)
{
await TriggerStartRealPlay();
}
}
```
```csharp
public async Task StopRealPlay()
{
if (!CanStopRealPlay)
{
return;
}
await InvokeVoidAsync("stopRealPlay", Id);
IsRealPlaying = false;
IsStartRecord = false;
await TriggerStopRealPlay();
}
```
```csharp
public async Task<bool> OpenSound()
{
var ret = false;
if (CanControlPlayback)
{
var code = await InvokeAsync<int>("openSound", Id);
ret = code == 100;
IsOpenSound = ret;
}
return ret;
}
```
Apply `CanControlPlayback` similarly to `CloseSound`, `SetVolume`, `CapturePictureAndDownload`, `CapturePicture`, `StartRecord`, and `StopRecord`.
This keeps all the multi-window vs. login/playback rules in one place, makes future behavior changes safer (only update the properties), and simplifies the public methods without changing any functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public async Task StartRealPlay(int streamType, int channelId) | ||
| { | ||
| if (IsLogin && !IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && !IsRealPlaying)) |
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated IsMultipleWindowType-related conditions into dedicated helper properties and using those at call sites to centralize the rules and simplify each method.
You can reduce the added complexity by centralizing the new IsMultipleWindowType logic into helper properties so the rules live in one place instead of being duplicated across many methods.
For example:
private bool CanStartRealPlay => IsMultipleWindowType || (IsLogin && !IsRealPlaying);
private bool CanStopRealPlay => IsMultipleWindowType || IsRealPlaying;
private bool CanControlPlayback => IsMultipleWindowType || (IsLogin && IsRealPlaying);Then use these in the call sites:
public async Task StartRealPlay(int streamType, int channelId)
{
if (!CanStartRealPlay)
{
return;
}
IsRealPlaying = await InvokeAsync<bool?>("startRealPlay", Id, streamType, channelId) ?? false;
if (IsRealPlaying)
{
await TriggerStartRealPlay();
}
}public async Task StopRealPlay()
{
if (!CanStopRealPlay)
{
return;
}
await InvokeVoidAsync("stopRealPlay", Id);
IsRealPlaying = false;
IsStartRecord = false;
await TriggerStopRealPlay();
}public async Task<bool> OpenSound()
{
var ret = false;
if (CanControlPlayback)
{
var code = await InvokeAsync<int>("openSound", Id);
ret = code == 100;
IsOpenSound = ret;
}
return ret;
}Apply CanControlPlayback similarly to CloseSound, SetVolume, CapturePictureAndDownload, CapturePicture, StartRecord, and StopRecord.
This keeps all the multi-window vs. login/playback rules in one place, makes future behavior changes safer (only update the properties), and simplifies the public methods without changing any functionality.
There was a problem hiding this comment.
Pull request overview
This pull request adds a new ChangeWindowNum function to the HikVision component that allows users to change the video viewport layout (split-screen configurations). The implementation includes JavaScript functions, C# wrapper methods, and introduces a state flag IsMultipleWindowType to track when split-screen mode is active.
Key Changes:
- Added
changeWndNumJavaScript function to support viewport layout changes (1x1, 2x2, 3x3, 4x4, 1x2, 2x1) - Refactored
initWindowto use a callback pattern for window index tracking - Introduced
IsMultipleWindowTypeflag to enable operations in split-screen mode
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| hikvision.js | Added changeWndNum function to handle window layout changes; refactored initWindow to use callback for iWndIndex tracking |
| HikVisionWebPlugin.razor.js | Exported new changeWndNum function and initialized iWndIndex property in data object |
| HikVisionWebPlugin.razor.cs | Added ChangeWindowNum C# wrapper method and IsMultipleWindowType property; updated conditional checks across multiple methods to support split-screen mode |
| BootstrapBlazor.HikVision.csproj | Bumped version from 10.0.9 to 10.0.10 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| var ret = false; | ||
| if (IsLogin && IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && IsRealPlaying)) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsLogin is false or IsRealPlaying is false, potentially allowing stop record operations on an unauthenticated or non-playing session. The condition should be: (IsLogin && IsRealPlaying) || (IsMultipleWindowType && IsLogin && IsRealPlaying) to ensure proper state checking.
| /// <summary> | ||
| /// 更改视口数量方法 | ||
| /// </summary> | ||
| /// <param name="iWndType">画面分割类型 1- 1*1，2- 2*2，3- 3*3，4- 4*4 (最大显示数值为4*4分割，数字超过4返回16分割)</param> |
There was a problem hiding this comment.
The documentation comment is unclear about the behavior when iWndType exceeds 4. The comment states "数字超过4返回16分割" (numbers exceeding 4 return 16 splits), but it's not clear if the function returns false or if it defaults to a 4x4 layout. Consider clarifying the exact behavior and return value in this edge case.
| /// <param name="iWndType">画面分割类型 1- 1*1，2- 2*2，3- 3*3，4- 4*4 (最大显示数值为4*4分割，数字超过4返回16分割)</param> | |
| /// <param name="iWndType">画面分割类型。支持的值：1 = 1×1，2 = 2×2，3 = 3×3，4 = 4×4。传入其它值将原样传递给底层插件，由其决定实际分割效果和返回值。</param> |
| public async Task<bool> ChangeWindowNum(string iWndType) | ||
| { | ||
| IsMultipleWindowType = iWndType != "1"; | ||
| return await InvokeAsync<bool>("changeWndNum", Id, iWndType); | ||
| } |
There was a problem hiding this comment.
Missing input validation for the iWndType parameter. The function should validate that iWndType is one of the expected values ("1", "2", "3", "4", "12", "21") before calling the API. Invalid values could lead to unexpected behavior or errors from the WebVideoCtrl API.
| } | ||
|
|
||
| export async function changeWndNum(id, iWndType) { | ||
| const vision = Data.get(id); |
There was a problem hiding this comment.
The parameter 'vision' is retrieved but never used in the function. Consider removing this unused variable declaration to improve code clarity.
| const vision = Data.get(id); |
| public async Task StartRealPlay(int streamType, int channelId) | ||
| { | ||
| if (IsLogin && !IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && !IsRealPlaying)) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsLogin is false or IsRealPlaying is already true, potentially allowing operations on an unauthenticated or invalid session. The condition should be: (IsLogin && !IsRealPlaying) || (IsMultipleWindowType && IsLogin) to ensure login state is always checked.
| if (IsMultipleWindowType || (IsLogin && !IsRealPlaying)) | |
| if ((IsLogin && !IsRealPlaying) || (IsMultipleWindowType && IsLogin)) |
| }); | ||
| } | ||
|
|
||
| export async function changeWndNum(id, iWndType) { |
There was a problem hiding this comment.
Function naming is inconsistent. The JavaScript function is named 'changeWndNum' (lowercase 'change') while the comment at line 129 uses Chinese characters suggesting "更改视口数量" (Change viewport number). Consider using 'ChangeWndNum' (PascalCase) to match the naming convention of other exported methods like 'StartRealPlay', 'StopRealPlay', etc., or ensure consistency across all layers.
| export async function changeWndNum(id, iWndType) { | |
| export async function ChangeWndNum(id, iWndType) { |
| public async Task StopRealPlay() | ||
| { | ||
| if (IsRealPlaying) | ||
| if (IsMultipleWindowType || IsRealPlaying) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsRealPlaying is false, potentially allowing stop operations when nothing is playing. The condition should be: IsRealPlaying || (IsMultipleWindowType && IsRealPlaying) to ensure proper state checking.
| if (IsMultipleWindowType || IsRealPlaying) | |
| if (IsRealPlaying) |
| { | ||
| var ret = false; | ||
| if (IsLogin && IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && IsRealPlaying)) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsLogin is false or IsRealPlaying is false, potentially allowing sound operations on an unauthenticated or non-playing session. The condition should be: (IsLogin && IsRealPlaying) || (IsMultipleWindowType && IsLogin && IsRealPlaying) to ensure proper state checking.
| if (IsMultipleWindowType || (IsLogin && IsRealPlaying)) | |
| if (IsLogin && IsRealPlaying) |
| { | ||
| var ret = false; | ||
| if (IsLogin && IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && IsRealPlaying)) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsLogin is false or IsRealPlaying is false, potentially allowing sound operations on an unauthenticated or non-playing session. The condition should be: (IsLogin && IsRealPlaying) || (IsMultipleWindowType && IsLogin && IsRealPlaying) to ensure proper state checking.
| { | ||
| var ret = false; | ||
| if (IsLogin && IsRealPlaying) | ||
| if (IsMultipleWindowType || (IsLogin && IsRealPlaying)) |
There was a problem hiding this comment.
The logic for controlling method calls using IsMultipleWindowType is problematic. When IsMultipleWindowType is true, the condition passes even if IsLogin is false or IsRealPlaying is false, potentially allowing record operations on an unauthenticated or non-playing session. The condition should be: (IsLogin && IsRealPlaying) || (IsMultipleWindowType && IsLogin && IsRealPlaying) to ensure proper state checking.
Link issues
fixes #870
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for changing HikVision plugin window layouts and adjust playback controls to work in multi-window mode.
New Features:
Enhancements: