feat(HikVision): add HidePlugin function#799
Merged
Conversation
Reviewer's GuideAdds visibility-aware behavior to the HikVision video plugin by observing DOM visibility and conditionally resizing, showing, or hiding the plugin, while cleaning up observers on dispose. Sequence diagram for HikVision plugin visibility-aware resize and showsequenceDiagram
participant Page
participant HikVisionModule
participant IntersectionObserver
participant JSVideoPlugin
participant WebVideoCtrl
Page->>HikVisionModule: init(id)
HikVisionModule->>HikVisionModule: create DOM element el
HikVisionModule->>IntersectionObserver: new IntersectionObserver(callback)
IntersectionObserver->>IntersectionObserver: observe(el)
HikVisionModule-->>Page: true
loop when_intersection_changes
IntersectionObserver->>HikVisionModule: callback(el)
HikVisionModule->>HikVisionModule: checkVisibility(el)
alt element_visible
HikVisionModule->>WebVideoCtrl: I_Resize(el.offsetWidth, el.offsetHeight)
else element_not_visible
HikVisionModule-->>IntersectionObserver: do_nothing
end
end
JSVideoPlugin->>JSVideoPlugin: JS_Resize(e, t)
JSVideoPlugin->>HikVisionModule: checkVisibility(el)
alt visible
JSVideoPlugin->>JSVideoPlugin: originalResize(e, t)
else not_visible
JSVideoPlugin->>WebVideoCtrl: I_HidPlugin()
end
JSVideoPlugin->>JSVideoPlugin: JS_ShowWnd()
JSVideoPlugin->>HikVisionModule: checkVisibility(el)
alt visible
JSVideoPlugin->>JSVideoPlugin: originalShowWnd()
else not_visible
JSVideoPlugin-->>JSVideoPlugin: return_resolved_promise()
end
Page->>HikVisionModule: dispose(id)
HikVisionModule->>IntersectionObserver: observer.disconnect()
Class diagram for JSVideoPlugin and HikVision visibility helpersclassDiagram
class JSVideoPlugin {
oOptions
JS_Resize(e, t)
JS_ShowWnd()
}
class WebVideoCtrl {
I_Resize(width, height)
I_HidPlugin()
}
class HikVisionModule {
init(id)
dispose(id)
hackJSResize()
hackJSShowWnd()
hackJSDestroyPlugin()
checkVisibility(el)
isVisible(element)
}
class DataStore {
get(id)
remove(id)
}
HikVisionModule --> JSVideoPlugin : patches_prototype
HikVisionModule --> WebVideoCtrl : calls
HikVisionModule --> DataStore : uses
JSVideoPlugin --> WebVideoCtrl : calls
HikVisionModule --> JSVideoPlugin : uses_oOptions_szId
HikVisionModule --> IntersectionObserver : creates
class IntersectionObserver {
observe(element)
disconnect()
}
Flow diagram for checkVisibility and isVisible logicflowchart TD
A[checkVisibility el] --> B{el.checkVisibility exists}
B -->|yes| C[call el.checkVisibility]
B -->|no| D[isVisible element]
subgraph isVisible_flow
D --> E{element exists}
E -->|no| F[return false]
E -->|yes| G[style = getComputedStyle element]
G --> H{style.display == none
or style.visibility == hidden
or opacity < 0.01}
H -->|yes| F
H -->|no| I[rect = element.getBoundingClientRect]
I --> J{rect.width == 0
or rect.height == 0}
J -->|yes| F
J -->|no| K[parent = element.parentElement]
K --> L{parent exists}
L -->|no| M[return true]
L -->|yes| N[parentStyle = getComputedStyle parent]
N --> O{parentStyle.display == none
or parentStyle.visibility == hidden}
O -->|yes| F
O -->|no| P[set parent = parent.parentElement]
P --> L
end
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 there - I've reviewed your changes - here's some feedback:
- Consider making
hackJSResizeandhackJSShowWndidempotent (e.g., by guarding against multiple patching of the prototype), since repeated calls toinitWindowwill currently re-wrap the same methods and could lead to unexpected nested behavior. - In
hackJSShowWnd, when the target element is not found you now silently do nothing instead of delegating to the original implementation; please verify whether this change is intentional and, if not, either fall back tooriginalShowWndor explicitly handle the missing element case. - The
JS_ShowWndoverride returnsnew Promise(resolve => resolve())when the element is not visible; if the original method is synchronous or has a different return type, you may want to align the no-op branch’s return value (e.g.,Promise.resolve()orundefined) with the original contract to avoid surprising callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `hackJSResize` and `hackJSShowWnd` idempotent (e.g., by guarding against multiple patching of the prototype), since repeated calls to `initWindow` will currently re-wrap the same methods and could lead to unexpected nested behavior.
- In `hackJSShowWnd`, when the target element is not found you now silently do nothing instead of delegating to the original implementation; please verify whether this change is intentional and, if not, either fall back to `originalShowWnd` or explicitly handle the missing element case.
- The `JS_ShowWnd` override returns `new Promise(resolve => resolve())` when the element is not visible; if the original method is synchronous or has a different return type, you may want to align the no-op branch’s return value (e.g., `Promise.resolve()` or `undefined`) with the original contract to avoid surprising callers.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:372` </location>
<code_context>
return xmlDoc.getElementsByTagName(tagName);
}
+
+const checkVisibility = el => {
+ if (el.checkVisibility) {
+ return el.checkVisibility();
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the visibility checks into a single helper and simplifying the fallbacks so the plugin’s visibility behavior is easier to reason about and maintain.
You can keep the new behavior while simplifying and centralizing the visibility logic.
### 1. Centralize visibility state (avoid re-checking everywhere)
Right now visibility is checked in three places (`IntersectionObserver`, `JS_Resize`, `JS_ShowWnd`) and `IntersectionObserver` callback even calls `checkVisibility(el)` again.
Introduce a small helper that is the *single* visibility source of truth for a given plugin element:
```js
// keep existing checkVisibility / isVisible, but use only here
const isPluginVisible = (szId) => {
const el = document.getElementById(szId);
if (!el) return false;
return checkVisibility(el);
};
```
Then simplify all call sites to use this helper:
```js
// IntersectionObserver in initWindow
const observer = new IntersectionObserver((entries) => {
const entry = entries[0];
if (!entry.isIntersecting) return;
const { szId } = vision; // or however szId is stored for this id
if (!isPluginVisible(szId)) return;
WebVideoCtrl.I_Resize(el.offsetWidth, el.offsetHeight);
});
```
```js
// JS_Resize override
JSVideoPlugin.prototype.JS_Resize = function (e, t) {
const { szId } = this.oOptions;
if (isPluginVisible(szId)) {
return originalResize.call(this, e, t);
}
WebVideoCtrl.I_HidPlugin();
};
```
```js
// JS_ShowWnd override
JSVideoPlugin.prototype.JS_ShowWnd = function () {
const { szId } = this.oOptions;
if (!isPluginVisible(szId)) {
return Promise.resolve(); // no-op but consistent Promise contract
}
return originalShowWnd.call(this);
};
```
This keeps the behavior but removes duplicated visibility logic and makes it easier to reason about.
### 2. Simplify `JS_ShowWnd` return shape
You don’t need to manually construct a promise:
```js
JSVideoPlugin.prototype.JS_ShowWnd = function () {
const { szId } = this.oOptions;
if (!isPluginVisible(szId)) {
// consistent and explicit: always return a Promise
return Promise.resolve();
}
return originalShowWnd.call(this);
};
```
This avoids the “three code paths with ambiguous return value” issue and keeps the function promise-based.
### 3. Lighten the `isVisible` fallback a bit
Given you already use `IntersectionObserver`, the fallback doesn’t need to be as heavy as a full parent-walk plus several style checks. For example:
```js
const isVisible = (element) => {
if (!element) return false;
const style = window.getComputedStyle(element);
if (style.display === 'none' || style.visibility === 'hidden' || style.opacity === '0') {
return false;
}
const rect = element.getBoundingClientRect();
return rect.width > 0 && rect.height > 0;
};
```
This still covers the common cases without a full parent traversal, and combined with `IntersectionObserver` should be sufficient.
These changes keep all the new functionality (hide on not-visible, observer-driven resize, cleanup via `observer.disconnect()`) while reducing the complexity and scattering of visibility logic.
</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 functionality to handle hiding the HikVision video plugin when elements are not visible in the viewport, addressing issue #798. The implementation includes visibility checking, IntersectionObserver integration, and proper cleanup on disposal.
- Adds an IntersectionObserver to automatically resize the video control when element visibility changes
- Implements visibility checks in
hackJSResizeand newhackJSShowWndfunctions to hide the plugin when elements are not visible - Adds helper functions
checkVisibilityandisVisiblefor comprehensive visibility detection with fallback support
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return originalResize.call(this, e, t); | ||
| } | ||
| else { | ||
| WebVideoCtrl.I_HidPlugin(); |
There was a problem hiding this comment.
There's a spelling error in the function name. The function is called I_HidPlugin() but it should likely be I_HidePlugin() based on the PR title and standard English conventions. Please verify the correct API method name from the WebVideoCtrl documentation.
Suggested change
| WebVideoCtrl.I_HidPlugin(); | |
| WebVideoCtrl.I_HidePlugin(); |
| } | ||
| else { | ||
| return new Promise((resolve, reject) => { | ||
| resolve(); |
There was a problem hiding this comment.
The reject parameter is declared but never used. When returning an empty resolved promise for a non-visible element, this could mask errors. Consider whether this is the intended behavior or if the promise should be rejected instead to signal that the window cannot be shown.
Suggested change
| resolve(); | |
| reject(new Error("Element is not visible, cannot show window.")); |
| return originalResize.call(this, e, t); | ||
| } | ||
| else { | ||
| WebVideoCtrl.I_HidPlugin(); |
There was a problem hiding this comment.
The JS_Resize function should return a value when the element is not visible. Currently, when visible is false and I_HidPlugin() is called, the function doesn't return anything (implicit undefined). This could break calling code that expects a return value. Consider adding an explicit return statement.
Suggested change
| WebVideoCtrl.I_HidPlugin(); | |
| WebVideoCtrl.I_HidPlugin(); | |
| return false; |
Comment on lines
+64
to
+66
| return new Promise((resolve, reject) => { | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
This empty promise can be simplified using Promise.resolve() instead of constructing a new Promise with an executor function. This is more concise and clearer in intent.
Suggested change
| return new Promise((resolve, reject) => { | |
| resolve(); | |
| }); | |
| return Promise.resolve(); |
| if (!element) return false; | ||
|
|
||
| const style = window.getComputedStyle(element); | ||
| if (style.display === 'none' || style.visibility === 'hidden' || parseFloat(style.opacity) < 0.01) { |
There was a problem hiding this comment.
Using a magic number 0.01 without explanation makes the code harder to understand. Consider adding a comment explaining why this specific threshold is chosen for opacity checks, or define it as a named constant like MIN_VISIBLE_OPACITY.
Comment on lines
+25
to
+26
| const observer = new IntersectionObserver(() => { | ||
| if (checkVisibility(el)) { |
There was a problem hiding this comment.
The IntersectionObserver callback doesn't use the entries parameter to check intersection state. The observer fires for both entering and leaving viewport, but the current implementation only checks visibility without considering the intersection state. This could cause unnecessary resize calls. Consider checking entries[0].isIntersecting to only resize when the element is actually intersecting the viewport.
Suggested change
| const observer = new IntersectionObserver(() => { | |
| if (checkVisibility(el)) { | |
| const observer = new IntersectionObserver((entries) => { | |
| if (entries[0].isIntersecting && checkVisibility(el)) { |
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 #798
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Guard HikVision video plugin operations behind element visibility checks to avoid resizing or showing the plugin when its container is hidden, and clean up observers on disposal.
Enhancements: