Feat: Added <select> to control enabling players on Monitor page#4428
Feat: Added <select> to control enabling players on Monitor page#4428IgorA100 wants to merge 23 commits intoZoneMinder:masterfrom
Conversation
|
This PR is not needed? |
|
@connortechnology |
|
It is not clear to me why we would want to disable ZMS playback. The .disabled stuff makes sense. |
This PR does NOT disable ZMS playback! At first, I thought about disabling all players, including ZMS, but I later realized that wasn't necessary and locked the code. This PR changes the player selection style. I plan to remove the checkboxes (which are quite confusing if there are more than two players) and switch to multi-select. I think it will be more convenient and look better. |
|
@connortechnology It's been almost six months, and I can't figure out what to do with this PR... |
|
It is still not clear to me why we need it. Why would we want to disable zms? |
This PR will allow users to select available players using multi-select, rather than the current checkboxes. The checkboxes themselves will remain visible for now, but they are disabled for editing. In the future, the checkboxes could be completely removed by revising the page layout.
We are NOT disabling ZMS in this PR!!!
|
|
@connortechnology |
|
@connortechnology |
There was a problem hiding this comment.
Pull request overview
This PR adds a multi-select dropdown (<select multiple>) to the Monitor configuration page's "viewing" tab, allowing users to enable/disable streaming players (Go2RTC, RTSP2Web, Janus) from a single control instead of individual checkboxes. It also introduces a CSS .disabled class mechanism for inputs that visually disables them while still including their values in form submissions. The ZMS MJPEG player is shown as always-selected and disabled in the dropdown. Much of the ZMS toggle functionality is commented out, pending a future database schema change.
Changes:
- Added a multi-select player control (
SelectPlayers) in the monitor viewing tab that syncs with existing player-enabled checkboxes, with supporting JavaScript functions for bidirectional state management. - Introduced a
.disabledCSS class for checkbox/radio inputs that mimics the nativedisabledattribute visually but still submits form values. - Added
ZMSEnabledfield references in the monitor action handler and model (commented out in the model), as groundwork for future ZMS toggle support.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
web/skins/classic/views/monitor.php |
Adds SelectPlayers multi-select dropdown and related PHP logic for building player options and initial selection state |
web/skins/classic/views/js/monitor.js |
Adds JavaScript functions to manage the multi-select ↔ checkbox synchronization and disable checkbox manual interaction |
web/skins/classic/js/skin.js |
Adds click prevention handler for input.disabled class in the global page initialization |
web/skins/classic/css/base/skin.css |
Extends existing :disabled CSS rules to also apply to .disabled class for checkboxes and radios |
web/includes/actions/monitor.php |
Adds ZMSEnabled to the checkbox defaults $types array |
web/includes/Monitor.php |
Adds commented-out ZMSEnabled field to the Monitor model defaults |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web/includes/actions/monitor.php
Outdated
| 'Enabled' => 0, | ||
| 'Deleted' => 0, | ||
| 'DecodingEnabled' => 0, | ||
| 'ZMSEnabled' => 0, |
There was a problem hiding this comment.
ZMSEnabled is added here as a checkbox default type, but ZMSEnabled is commented out in Monitor.php's $defaults array (line 234). The Object::changes() method only includes fields that exist in $this->defaults (see web/includes/Object.php:316), so this field will never actually be persisted to the database. Additionally, the ZMSEnabled column doesn't exist in the database schema (it requires a manual ALTER TABLE as mentioned in the PR description). This means the form will submit ZMSEnabled=0 on every save, and the $monitor->changes() call could potentially produce unexpected behavior when it encounters a field that doesn't exist in the monitor object.
Either remove this line (keeping it consistent with the commented-out model definition), or uncomment the model definition and add the corresponding database migration.
| 'ZMSEnabled' => 0, |
There was a problem hiding this comment.
Let's leave the line commented out, in case we need to disable ZMS someday.
web/skins/classic/views/monitor.php
Outdated
| @@ -1258,6 +1277,14 @@ class="nav-link<?php echo ($tab == $name ? ' active' : '') . ' ' . (($name == 'z | |||
| <label><?php echo translate('RTSPStreamName'); echo makeHelpLink('OPTIONS_RTSPSTREAMNAME') ?></label> | |||
| <input type="text" name="newMonitor[RTSPStreamName]" value="<?php echo validHtmlStr($monitor->RTSPStreamName()) ?>"/> | |||
| </li> | |||
| <li id="SelectPlayers" class="SelectPlayers"> | |||
| <label><?php echo translate('Select players'); echo makeHelpLink('OPTIONS_SELECTPLAYERS') ?> </label> | |||
| <?php echo htmlSelect('SelectPlayers', $selectPlayers, $selectedPlayers, ['class'=>'chosen chosen-full-width', 'multiple'=>'', 'data-on-change'=>'selectPlayers', 'none-exists'=>$noneExists]); ?> | |||
| </li> | |||
| <!--<li id="FunctionZMSEnabled" class='hidden-shift'> | |||
| <label><?php //echo translate('ZMS MJPEG') ?></label> | |||
| <input type="checkbox" name="newMonitor[ZMSEnabled]" value="1"<?php //echo $monitor->ZMSEnabled() ? ' checked="checked"' : '' ?> class="hidden-shift0"/> | |||
| </li>--> | |||
There was a problem hiding this comment.
There is a significant amount of commented-out code throughout this PR (lines 1257, 1259-1266, 1284-1287). This makes the code harder to read and maintain. If this code represents planned future functionality (as the PR description suggests), it would be better to either:
- Not include the commented-out code and implement it fully in a future PR when the DB schema change is ready.
- At minimum, add clear
TODOorFIXMEcomments explaining what these blocks are for and what prerequisite (DB migration) is needed.
The commented-out HTML block on lines 1284-1287 still contains PHP code inside an HTML comment (<!--...-->), which means the PHP code will execute server-side even though the output is hidden from the DOM. The // inside the <?php ?> tags prevents execution, but this pattern is fragile.
…s/js/monitor.js As recommended by Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I'll analyze Copilot's recommendations. |
…sic/js/skin.js) As recommended by Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…kins/classic/views/monitor.php) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…MSEnabled'] = 0;" (monitor.php)
|
@connortechnology For $enableDisableZMS = false, the DB can be left unchanged. Actually, at the moment, even with $enableDisableZMS = true and the DB change, ZMS will still work. In other words, ZMS functionality doesn't change at all. Only the UI changes. If you have any questions, ask them, and I'll answer them. |
The code implements support for disabling ALL players, including "ZMS MJPEG", but the UI does not currently use disabling all players. Because at the moment I am not sure that it is necessary to disable all players.
To implement enabling/disabling "ZMS MJPEG" you need to uncomment some lines of code and add a field to the DB
ALTER TABLEMonitorsADD COLUMNZMSEnabledBOOLEAN NOT NULL default true AFTERDecoding``It looks like this: #4393 (comment)
Also added the ability for
<input>to use ".disabled", because if you use the "disabled" attribute for<input>, then the value of<input>will not be used in the form. And now, if you add the "disabled" class, then the value of<input>will be used in the form, but you cannot change its value manually in the UI.If this PR is approved, then in the future you can remove the checkboxes for controlling player enablement from the UI and visually combine the options into the Go2RTC, RTSP2Web & Janus groups
I'm not sure about the feasibility of this PR, but I had some ideas and decided to publish them. Probably, choosing to enable/disable players in one place (in "multi select") is better than many checkboxes.