VM edit make RAM values human readable#2331
Conversation
WalkthroughCSS file shows a no-op replacement. The VM Custom form PHP updates memory dropdown option labels: it computes sizes per step, displays MB for values <1024 MB and GB (one decimal) for ≥1024 MB, applied to both Initial and Max memory. No public interfaces changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant B as Browser (UI)
participant S as PHP Template (Custom.form.php)
U->>B: Open VM Custom form
B->>S: Request form HTML
Note right of S: Generate memory options<br/>- Compute sizeMB = i*512<br/>- value = sizeMB*1024 (KiB)<br/>- Label: MB if <1024, else GB (1 decimal)
S-->>B: HTML with Initial/Max memory dropdowns (new labels)
B-->>U: Render dropdowns with MB/GB labels
U->>B: Select memory values (behavior unchanged)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (3)
368-374: Consider enhancingformatMemoryDisplayfor fractional GB values.The function displays clean integer GB values but could be improved to handle fractional GB values (e.g., 1.5 GB) for better precision when needed.
Consider this enhancement:
function formatMemoryDisplay($value) { if ($value >= 1024 * 1024) { - return ($value / (1024 * 1024)) . ' GB'; + $gb = $value / (1024 * 1024); + // Display as integer if whole number, otherwise show one decimal place + return (floor($gb) == $gb ? floor($gb) : number_format($gb, 1)) . ' GB'; } else { return ($value / 1024) . ' MB'; } }
807-857: Complex slider handle override may cause maintenance issues.The custom
_mouseCaptureoverride for handling equal slider values is complex and could break with jQuery UI updates. Consider documenting this thoroughly or finding a simpler approach.Consider these alternatives:
- Add comprehensive comments explaining why this override is necessary
- Consider using a different slider library that natively supports this behavior
- Add version checking for jQuery UI to ensure compatibility
At minimum, add documentation:
+ // IMPORTANT: Custom override to handle slider behavior when both handles have equal values + // Default jQuery UI behavior makes it difficult to select specific handles when values overlap + // This override ensures the clicked handle is always selected, not the closest by value var slider = $("#memory-slider-range").slider("instance");
699-722: Consider extracting magic numbers into named constants.The
updateSelectOptionsfunction has implicit dependencies on the slider configuration that could be made clearer.Consider defining constants for clarity:
const SLIDER_CONFIG = { MIN_INDEX: 0, MAX_INDEX: <?=($maxmem*2)+1?>, INITIAL_MEM_HANDLE: 0, MAX_MEM_HANDLE: 1 };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.vm.manager/styles/dynamix.vm.manager.css(1 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(2 hunks)
🔇 Additional comments (4)
emhttp/plugins/dynamix.vm.manager/styles/dynamix.vm.manager.css (1)
683-732: LGTM! Clean CSS implementation for the memory slider.The CSS implementation for the new memory slider UI is well-structured. The styles appropriately handle the dual-handle slider with rounded edges on the appropriate handles and proper positioning using transforms. The flexbox layout for the memory selects ensures proper spacing and alignment.
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (3)
865-868: Good implementation of disabled state for running VMs.The slider is properly disabled when the VM is running, preventing memory changes while the VM is active. This is a good safety measure.
639-663: Clean HTML structure for the memory slider UI.The table structure properly integrates the new memory slider while maintaining the existing XML preview functionality. The hidden inputs ensure backward compatibility with the form submission.
683-695: Missing null/undefined checks inupdateDisplayfunction.The function doesn't verify that the DOM elements exist before attempting to update them, which could cause JavaScript errors if the elements are missing.
Add safety checks:
function updateDisplay(field, value) { var hidden = document.getElementById('domain_' + field + '_hidden'); - if (hidden) hidden.value = value; + if (hidden) { + hidden.value = value; + } var select = document.getElementById(field + '_select'); if (select) { for (var i = 0; i < window.memoryValues.length; i++) { if (window.memoryValues[i] == value) { select.selectedIndex = i; break; } } + } else { + console.warn('Select element not found:', field + '_select'); } }Likely an incorrect or invalid review comment.
| function getSliderPosition($currentValue) { | ||
| global $maxmem; | ||
| $values = [128 * 1024, 256 * 1024]; | ||
| for ($i = 1; $i <= ($maxmem*2); $i++) { | ||
| $values[] = $i * 512 * 1024; | ||
| } | ||
|
|
||
| $position = 0; | ||
| foreach ($values as $index => $value) { | ||
| if ($currentValue <= $value) { | ||
| $position = $index; | ||
| break; | ||
| } | ||
| } | ||
| return $position; | ||
| } |
There was a problem hiding this comment.
Fix potential infinite loop in getSliderPosition function.
The function lacks a safeguard if $currentValue exceeds all values in the array. The loop will complete without setting $position, leaving it at 0, which may not be the intended behavior for values larger than the maximum.
Apply this fix:
function getSliderPosition($currentValue) {
global $maxmem;
$values = [128 * 1024, 256 * 1024];
for ($i = 1; $i <= ($maxmem*2); $i++) {
$values[] = $i * 512 * 1024;
}
$position = 0;
foreach ($values as $index => $value) {
if ($currentValue <= $value) {
$position = $index;
break;
}
}
+ // If currentValue exceeds all values, set to maximum position
+ if ($position === 0 && $currentValue > $values[0]) {
+ $position = count($values) - 1;
+ }
return $position;
}🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php around lines 351
to 366, getSliderPosition builds a values array then searches for the first
value >= $currentValue but if $currentValue exceeds all entries the loop leaves
$position at 0; change the logic so after the foreach if no match was found you
set $position to the last index (e.g. count($values) - 1) to clamp to the
maximum position; ensure $position is an integer and return that clamped value.
| var sliderValue = parseInt($(this).val()); | ||
| var currentValues = $("#memory-slider-range").slider("values"); | ||
|
|
||
| if (sliderValue <= currentValues[1]) { | ||
| $("#memory-slider-range").slider("values", 0, sliderValue); | ||
| var memValue = window.memoryValues[sliderValue] || window.memoryValues[0]; | ||
| updateDisplay('mem', memValue); | ||
| updateSelectOptions(); // Update disabled options | ||
| } else { | ||
| // Reset select to current value | ||
| var currentIndex = <?=getSliderPosition($arrConfig['domain']['mem'])?>; | ||
| $(this).val(currentIndex); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential for inconsistent state when select change validation fails.
When a user selects an invalid value in the dropdowns (lines 786-788 and 802-804), the select reverts to the previous value, but there's no user feedback about why their selection was rejected.
Add user feedback for invalid selections:
} else {
// Reset select to current value
var currentIndex = <?=getSliderPosition($arrConfig['domain']['mem'])?>;
$(this).val(currentIndex);
+ // Provide feedback to user
+ $(this).addClass('invalid-selection');
+ setTimeout(() => $(this).removeClass('invalid-selection'), 500);
}And add corresponding CSS:
.invalid-selection {
border-color: #ff6b6b !important;
animation: shake 0.3s;
}
@keyframes shake {
0%, 100% { transform: translateX(0); }
25% { transform: translateX(-2px); }
75% { transform: translateX(2px); }
}Also applies to: 791-805
ljm42
force-pushed
the
vm-memory-slider
branch
from
16bdf41 to
2c09c21
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (3)
351-366: Clamp slider index when current value exceeds the last supported stepIf
$currentValueis larger than the largest entry, position stays 0, desynchronizing the slider from hidden/select values. Clamp to the last index.function getSliderPosition($currentValue) { global $maxmem; $values = [128 * 1024, 256 * 1024]; for ($i = 1; $i <= ($maxmem*2); $i++) { $values[] = $i * 512 * 1024; } - - $position = 0; + $position = 0; foreach ($values as $index => $value) { if ($currentValue <= $value) { $position = $index; break; } } - return $position; + // Clamp to the maximum position if value exceeds the last step + $lastIndex = count($values) - 1; + if ($currentValue > $values[$lastIndex]) { + $position = $lastIndex; + } + return (int)$position; }
368-374: Unify memory formatting with JS (show one decimal for GB)Current PHP returns raw float text (potentially many decimals) while JS uses one decimal for GB. Align PHP to 1 decimal and integer MB to match JS and avoid UI inconsistencies.
function formatMemoryDisplay($value) { - if ($value >= 1024 * 1024) { - return ($value / (1024 * 1024)) . ' GB'; - } else { - return ($value / 1024) . ' MB'; - } + if ($value >= 1024 * 1024) { + $gb = $value / (1024 * 1024); + $text = (fmod($gb, 1.0) === 0.0) ? (string)intval($gb) : number_format($gb, 1); + return $text . ' GB'; + } + return intval($value / 1024) . ' MB'; }
774-806: Resetting to the original server index is incorrect; use the current slider index and add lightweight feedbackOn invalid dropdown selection, it reverts to the initial PHP-calculated index, not the slider’s current value. This becomes wrong after any user change.
$('#mem_select').change(function() { var sliderValue = parseInt($(this).val()); var currentValues = $("#memory-slider-range").slider("values"); if (sliderValue <= currentValues[1]) { $("#memory-slider-range").slider("values", 0, sliderValue); var memValue = window.memoryValues[sliderValue] || window.memoryValues[0]; updateDisplay('mem', memValue); updateSelectOptions(); // Update disabled options } else { - // Reset select to current value - var currentIndex = <?=getSliderPosition($arrConfig['domain']['mem'])?>; - $(this).val(currentIndex); + // Reset to current slider value and provide subtle feedback + $(this).val(currentValues[0]).addClass('invalid-selection'); + setTimeout(() => $(this).removeClass('invalid-selection'), 500); } }); $('#maxmem_select').change(function() { var sliderValue = parseInt($(this).val())); var currentValues = $("#memory-slider-range").slider("values"); if (sliderValue >= currentValues[0]) { $("#memory-slider-range").slider("values", 1, sliderValue); var maxmemValue = window.memoryValues[sliderValue] || window.memoryValues[0]; updateDisplay('maxmem', maxmemValue); updateSelectOptions(); // Update disabled options } else { - // Reset select to current value - var currentIndex = <?=getSliderPosition($arrConfig['domain']['maxmem'])?>; - $(this).val(currentIndex); + $(this).val(currentValues[1]).addClass('invalid-selection'); + setTimeout(() => $(this).removeClass('invalid-selection'), 500); } });Optionally add a small CSS rule in the VM manager CSS to highlight the invalid moment:
.invalid-selection { outline: 2px solid #ff6b6b; }
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (2)
376-383: Single source of truth for memory steps (avoid duplicating the generator in PHP and JS)Build
$memoryValuesonce in PHP and JSON‑embed it for JS instead of echoing a PHPforloop in JS. Keeps the arrays in lockstep and simplifies maintenance.// PHP $memoryValues = [ 128 * 1024, 256 * 1024, ]; for ($i = 1; $i <= ($maxmem * 2); $i++) { $memoryValues[] = $i * 512 * 1024; } // JS -window.memoryValues = [ - 128 * 1024, // 128 MB - 256 * 1024, // 256 MB - <?php for ($i = 1; $i <= ($maxmem*2); $i++) { ?> - <?=$i * 512 * 1024?>, // <?=$i * 512?> MB - <?php } ?> -]; +window.memoryValues = <?= json_encode($memoryValues) ?>;Also applies to: 666-675
638-659: Make new labels translatable and accessibleAdd translations and proper label associations for a11y.
- <span class="advanced">Initial/Max </span>Memory: + <span class="advanced">_(Initial/Max)_ </span>_(Memory)_: ... - <div> - <span>Initial:</span> - <select id="mem_select"></select> - </div> + <div> + <label for="mem_select">_(Initial)_:</label> + <select id="mem_select" aria-label="<?=_('Initial memory')?>"></select> + </div> ... - <div> - <span>Maximum:</span> - <select id="maxmem_select"></select> - </div> + <div> + <label for="maxmem_select">_(Maximum)_:</label> + <select id="maxmem_select" aria-label="<?=_('Maximum memory')?>"></select> + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.vm.manager/styles/dynamix.vm.manager.css(1 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/styles/dynamix.vm.manager.css
🔇 Additional comments (4)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (4)
351-356: Ensure $maxmem is defined and in the expected units before useMultiple places assume
$maxmemexists and represents GiB (e.g., to build steps and slider max). If undefined/mis-scaled, PHP notices or mismatched arrays vs. slider bounds will occur. Add a defensive init near the first usage.+// Ensure $maxmem (GiB) is defined; derive from current config if missing +if (!isset($maxmem)) { + // domain[maxmem] is in KiB; convert to GiB and clamp >= 1 + $maxmem = max(1, (int) floor($arrConfig['domain']['maxmem'] / (1024 * 1024))); +}Would you confirm where
$maxmemis initialized upstream (includes/templates)? If it’s guaranteed, we can skip the fallback.Also applies to: 381-383, 758-759, 671-673
807-858: Overriding jQuery UI’s private _mouseCapture can be brittle across upgradesThis works, but it relies on an internal API. If jQuery UI is updated, behavior may change. Consider gating the override with a version check or falling back to a “start” handler strategy that uses
ui.handleto pick the active handle.Would you confirm the bundled jQuery UI version and whether it’s pinned? If not pinned, we should add a guard:
if (slider && typeof slider._mouseCapture === 'function') { /* override */ }
683-697: LGTM: hidden inputs + select sync is correctHidden values are kept authoritative; selects reflect indices; lookup by exact step avoids float issues.
698-723: LGTM: constraints are enforced via disabled optionsSimple, predictable UX and consistent with the slider.
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
|
Thanks for this. I think updating the dropdown with with more user friendly values makes a lot of sense. But at this point we don't really want to introduce new controls to the legacy UI. If you would be willing to update the PR to only include the more friendly dropdown settings we should be able to get it in. Thanks! |
ljm42
changed the title
|
thank you! |
3 participants
This PR modifies the memory settings in the VM page in a few ways in an attempt to be more user friendly. Below is a list of changes and why I think they should be adopted, either in part or in whole.
The inline xml view still works and the slider is disabled if the VM is running, just as other elements are. No extra libraries are needed as this uses the existing jQuery version. Here are some screenshots to illustrate the changes:
Summary by CodeRabbit