feat(HikVision): add IsOpenSound parameter #867
Merged
Conversation
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
Reviewer's GuideAdds client- and server-side support for starting and stopping HikVision video recording, wires new JS interop calls through the Razor component, and slightly improves error handling and documentation for capture operations. Sequence diagram for starting and stopping HikVision recording via JS interopsequenceDiagram
participant CSharp_HikVisionWebPlugin
participant JS_HikVisionWebPlugin as JS_HikVisionWebPlugin_razor_js
participant JS_Hikvision as hikvision_js
participant WebVideoCtrl
CSharp_HikVisionWebPlugin->>CSharp_HikVisionWebPlugin: StartRecord()
alt IsLogin_and_IsRealPlaying_true
CSharp_HikVisionWebPlugin->>JS_HikVisionWebPlugin: InvokeAsync startRecord Id
JS_HikVisionWebPlugin->>JS_Hikvision: startRecord Id
JS_Hikvision->>JS_Hikvision: Data.get Id
JS_Hikvision->>JS_Hikvision: check realPlaying
alt realPlaying_true
JS_Hikvision->>WebVideoCtrl: I_StartRecord record_timestamp
WebVideoCtrl-->>JS_Hikvision: success_or_error_callback
JS_Hikvision->>JS_Hikvision: resolve_or_reject_Promise
JS_Hikvision-->>JS_HikVisionWebPlugin: bool_result
JS_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: bool_result
else realPlaying_not_true
JS_Hikvision-->>JS_HikVisionWebPlugin: false
JS_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: false
end
else not_logged_in_or_not_playing
CSharp_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: return_false
end
CSharp_HikVisionWebPlugin->>CSharp_HikVisionWebPlugin: StopRecord()
alt IsLogin_and_IsRealPlaying_true
CSharp_HikVisionWebPlugin->>JS_HikVisionWebPlugin: InvokeAsync stopRecord Id
JS_HikVisionWebPlugin->>JS_Hikvision: stopRecord Id
JS_Hikvision->>JS_Hikvision: Data.get Id
JS_Hikvision->>JS_Hikvision: check realPlaying
alt realPlaying_true
JS_Hikvision->>WebVideoCtrl: I_StopRecord
WebVideoCtrl-->>JS_Hikvision: success_or_error_callback
JS_Hikvision->>JS_Hikvision: resolve_or_reject_Promise
JS_Hikvision-->>JS_HikVisionWebPlugin: bool_result
JS_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: bool_result
else realPlaying_not_true
JS_Hikvision-->>JS_HikVisionWebPlugin: false
JS_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: false
end
else not_logged_in_or_not_playing
CSharp_HikVisionWebPlugin-->>CSharp_HikVisionWebPlugin: return_false
end
Class diagram for updated HikVisionWebPlugin recording and capture methodsclassDiagram
class HikVisionWebPlugin {
bool IsLogin
bool IsRealPlaying
string Id
Task CapturePictureAndDownload()
Task~IJSStreamReference?~ CapturePicture(CancellationToken token)
Task TriggerReceivePictureStream(IJSStreamReference stream)
Task~bool~ StartRecord()
Task~bool~ StopRecord()
}
class Hikvision_razor_js {
+init(id, invoke)
+login(id, ip, port, userName, password, loginType)
+logout(id)
+startRealPlay(id)
+stopRealPlay(id)
+openSound(id)
+closeSound(id)
+setVolume(id, value)
+capturePicture(id)
+capturePictureAndDownload(id)
+startRecord(id)
+stopRecord(id)
+dispose(id)
}
class hikvision_js {
+startRecord(id)
+stopRecord(id)
+capturePicture(id)
+capturePictureAndDownload(id)
+dispose(id)
}
HikVisionWebPlugin --> Hikvision_razor_js : uses_JS_interop
Hikvision_razor_js --> hikvision_js : wraps_calls
hikvision_js --> WebVideoCtrl : calls_recording_APIs
class WebVideoCtrl {
+I_StartRecord(recordName, options)
+I_StopRecord(options)
}
File-Level Changes
Assessment against 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 3 issues, and left some high level feedback:
- In
startRecord/stopRecordthe polling handle is created withsetIntervalbut cleared withclearTimeout; this should beclearIntervalto avoid leaking the interval timer. startRecord/stopRecordreturn a Promise that never settles ifWebVideoCtrl.I_StartRecord/I_StopRecordthrow before settingcompleted, so consider rejecting the Promise in thecatchblock or short‑circuiting to a resolved value there.- The
stopRecordcatch block is empty while other operations log exceptions withconsole.log, so it would be more consistent and aid debugging to log or otherwise handle the error there as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `startRecord`/`stopRecord` the polling handle is created with `setInterval` but cleared with `clearTimeout`; this should be `clearInterval` to avoid leaking the interval timer.
- `startRecord`/`stopRecord` return a Promise that never settles if `WebVideoCtrl.I_StartRecord`/`I_StopRecord` throw before setting `completed`, so consider rejecting the Promise in the `catch` block or short‑circuiting to a resolved value there.
- The `stopRecord` catch block is empty while other operations log exceptions with `console.log`, so it would be more consistent and aid debugging to log or otherwise handle the error there as well.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:496-498` </location>
<code_context>
}
+
+ return new Promise((resolve, reject) => {
+ const handler = setInterval(() => {
+ if (completed) {
+ clearTimeout(handler);
+ if (error === null) {
+ resolve(true);
</code_context>
<issue_to_address>
**issue (bug_risk):** Use `clearInterval` instead of `clearTimeout` for an interval handle.
`setInterval` must be paired with `clearInterval` for the returned handle. Using `clearTimeout` here is incorrect and risks the interval not being cleared, causing ongoing executions and potential leaks after the Promise has settled.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:495-504` </location>
<code_context>
+ console.log(ex);
}
+
+ return new Promise((resolve, reject) => {
+ const handler = setInterval(() => {
+ if (completed) {
+ clearTimeout(handler);
+ if (error === null) {
+ resolve(true);
+ }
+ else {
+ reject(error);
+ }
+ }
+ }, 16);
+ });
+}
</code_context>
<issue_to_address>
**suggestion:** Consider resolving/rejecting directly from the success/error callbacks instead of polling with `setInterval`.
Because `I_StartRecord`/`I_StopRecord` already take `success` and `error` callbacks, you can wrap them in a Promise and call `resolve(true)` / `reject(oError)` directly in those callbacks. This removes the `completed`/`error` state and the 16ms `setInterval`, simplifying the flow and avoiding an extra timer altogether.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:470` </location>
<code_context>
+ }
+}
+
+export async function startRecord(id) {
+ const vision = Data.get(id);
+ const { realPlaying } = vision;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new startRecord/stopRecord implementations by resolving the Promise directly in the SDK callbacks (optionally via a shared helper) instead of using polling flags and intervals.
You can simplify `startRecord`/`stopRecord` by removing the polling/flags and resolving the promise directly in the callbacks. This keeps all behavior while reducing complexity and duplication, and also fixes the `clearTimeout`/`setInterval` mismatch.
```js
export function startRecord(id) {
const vision = Data.get(id);
const { realPlaying } = vision;
if (realPlaying !== true) {
return Promise.resolve(false);
}
return new Promise((resolve, reject) => {
try {
WebVideoCtrl.I_StartRecord(`record_${Date.now()}`, {
success: () => resolve(true),
error: (oError) => reject(oError)
});
} catch (ex) {
console.log(ex);
reject(ex);
}
});
}
```
```js
export function stopRecord(id) {
const vision = Data.get(id);
const { realPlaying } = vision;
if (realPlaying !== true) {
return Promise.resolve(false);
}
return new Promise((resolve, reject) => {
try {
WebVideoCtrl.I_StopRecord({
success: () => resolve(true),
error: (oError) => reject(oError)
});
} catch (ex) {
console.log(ex);
reject(ex);
}
});
}
```
If you want to further reduce duplication, you can extract a tiny helper:
```js
function execRecord(fn) {
return new Promise((resolve, reject) => {
try {
fn({
success: () => resolve(true),
error: (oError) => reject(oError)
});
} catch (ex) {
console.log(ex);
reject(ex);
}
});
}
// usage
export function startRecord(id) {
const vision = Data.get(id);
if (vision.realPlaying !== true) return Promise.resolve(false);
return execRecord((opts) => WebVideoCtrl.I_StartRecord(`record_${Date.now()}`, opts));
}
export function stopRecord(id) {
const vision = Data.get(id);
if (vision.realPlaying !== true) return Promise.resolve(false);
return execRecord((opts) => WebVideoCtrl.I_StopRecord(opts));
}
```
This removes the interval-based busy-wait, unifies error handling (always logs and rejects on exceptions/errors), and keeps the external behavior (`true` on success, rejection on failure, `false` when not playing) intact.
</issue_to_address>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
This PR adds video recording functionality to the HikVision component, despite the misleading title which mentions "IsOpenSound parameter". The changes introduce StartRecord and StopRecord methods across JavaScript and C# layers.
Key changes:
- Added
startRecordandstopRecordfunctions in JavaScript to interface with WebVideoCtrl API - Exposed these functions through C# wrapper methods for Blazor integration
- Updated documentation comments for capture methods
- Bumped package version from 10.0.6 to 10.0.7
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| hikvision.js | Added startRecord and stopRecord async functions with callback-to-promise conversion, plus console logging in catch blocks |
| HikVisionWebPlugin.razor.js | Added imports and exports for new startRecord and stopRecord functions |
| HikVisionWebPlugin.razor.cs | Implemented C# wrapper methods StartRecord and StopRecord, updated documentation comments |
| BootstrapBlazor.HikVision.csproj | Incremented version from 10.0.6 to 10.0.7 |
Comments suppressed due to low confidence (4)
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:1
- Unused import registerBootstrapBlazorModule.
import { addScript, registerBootstrapBlazorModule } from '../BootstrapBlazor/modules/utility.js';
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:76
- Unused variable originalDestroy.
const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin;
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:190
- Unused variable logined.
const { szDeviceIdentify, logined } = vision;
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:390
- Unused variable iWndIndex.
const { iWndIndex, realPlaying } = vision;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| } | ||
| catch (ex) { | ||
|
|
There was a problem hiding this comment.
The catch block is empty and doesn't log the exception like other catch blocks in this file. For consistency with the error handling pattern used in lines 428, 466, and 492, this should log the exception.
Suggested change
| console.error('Error while stopping record:', ex); |
| return new Promise((resolve, reject) => { | ||
| const handler = setInterval(() => { | ||
| if (completed) { | ||
| clearTimeout(handler); |
There was a problem hiding this comment.
Using clearTimeout with a setInterval handler is incorrect. The handler is created by setInterval on line 496, so it should be cleared using clearInterval, not clearTimeout.
| return new Promise((resolve, reject) => { | ||
| const handler = setInterval(() => { | ||
| if (completed) { | ||
| clearTimeout(handler); |
There was a problem hiding this comment.
Using clearTimeout with a setInterval handler is incorrect. The handler is created by setInterval on line 536, so it should be cleared using clearInterval, not clearTimeout.
Suggested change
| clearTimeout(handler); | |
| clearInterval(handler); |
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 #866
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add client-side support for starting and stopping HikVision video recording and expose it through the Blazor component API.
New Features:
Enhancements: