Conversation
… summary tab. For feature #256.
WalkthroughThis pull request introduces comprehensive enhancements to slider functionality across the Minsky project. The changes span multiple files and components, focusing on improving slider management, initialization, and interaction. Key modifications include adding new methods for slider control, refactoring how slider bounds are set and managed, and updating various components to support more robust slider interactions. The changes aim to provide more flexible and consistent handling of slider properties across the application's backend and frontend. Changes
Sequence DiagramsequenceDiagram
participant User
participant Variable
participant VariableValue
participant Slider
User->>Variable: Adjust Slider
Variable->>VariableValue: vValue()
VariableValue->>Slider: adjustSliderBounds()
Slider-->>VariableValue: Bounds Adjusted
VariableValue-->>Variable: Updated Value
Variable-->>User: Slider Reflects Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
engine/variableSummary.h (1)
36-36: Initialize new slider fields or ensure they are always set.Since
sliderStep,sliderMin, andsliderMaxare newly added double members, be mindful of when and how they get initialized. In certain edge cases (e.g. default construction), these could remain uninitialized and lead to unexpected behaviors.engine/variableValue.h (1)
190-196: New slider operations
The additional methodssliderSet,incrSlider,initSliderBounds, andadjustSliderBoundsprovide a clear abstraction for slider behavior. Ensure consistent usage across the codebase and adequate test coverage for edge cases (e.g., negative boundaries, zero or negative steps).model/variable.h (1)
89-89: Removal ofSliderfrom inheritance
VariableBaseno longer inherits fromSlider. This decouples slider logic from the base variable, promoting a cleaner separation of concerns. Verify that existing features relying on a variable’s direct slider interface are updated accordingly.engine/mdlReader.cc (1)
371-386: Slider specification parsing
This block robustly parses the[min,max,step]text format for slider bounds. Consider adding validation or error handling for malformed inputs (e.g., non-numeric values, empty strings).engine/variableValue.cc (3)
222-230: Ensure robust error handling withinsliderSet.Currently the function clamps the input to
[sliderMin, sliderMax]and then re-initializes the value withinit(to_string(x)). This approach is fine, but ifto_string(x)fails parsing elsewhere, potential issues may arise. Verify that downstream code can gracefully handle the string-based initialization for numeric values.
236-255: Thread-safety and repeated calls.
initSliderBounds()sets default bounds ifvalue()==0(±1) or ±10×value otherwise. Consider verifying this is only invoked in single-threaded contexts or properly synchronized, since repeated calls from multiple threads could cause race conditions onsliderBoundsSet.
Line range hint
351-395: Large function complexity.
resetValue(VariableValue& v)is quite lengthy. Consider splitting out the dimension reset and initialization logic into smaller helper functions for readability and maintainability.model/variable.cc (1)
613-620: Prioritize user interaction clarity.
sliderSetis called when the user drags the mouse horizontally. Validate thatsliderSetcalls remain consistent with the bounding logic ininitSliderBounds()andadjustSliderBounds().gui-js/libs/shared/src/lib/backend/minsky.ts (1)
233-233: Expand type safety for arguments toenableSlider.
Currently the function signature uses(...args: any[]). For clarity and maintainability, consider using a more precise type or an interface to document the expected parameters.gui-js/libs/ui-components/src/lib/summary/summary.component.html (1)
109-137: Consider extracting a reusable template for slider property cells.The slider min and max cells share identical structure. Consider creating a reusable template to reduce code duplication.
Example approach:
+ <!-- slider-property-cell.template.html --> + <ng-template #sliderPropertyCell let-property="property" let-value="value"> + <div *ngIf="type=='parameter' && !editing(variable, property)" + [title]="value" + (click)="edit($event,variable,property)" + class="data-cell numeric" latex + equation="{{truncateValue(value)}}"> + </div> + <div *ngIf="type=='parameter' && editing(variable,property)" + class="data-cell"> + <input + (keydown.Enter)="finishEditing()" + (click)="$event.stopImmediatePropagation()" + type="text" [(ngModel)]="editCellContents"/> + </div> + </ng-template> + + <!-- Usage --> - <div *ngIf="type=='parameter' && !editing(variable,'sliderMin')" - [title]="variable.sliderMin" - (click)="edit($event,variable,'sliderMin')" - class="data-cell numeric" latex - equation="{{truncateValue(variable.sliderMin)}}"> - </div> - <div *ngIf="type=='parameter' && editing(variable,'sliderMin')" - class="data-cell"> - <input - (keydown.Enter)="finishEditing()" - (click)="$event.stopImmediatePropagation()" - type="text" [(ngModel)]="editCellContents"/> - </div> + <ng-container *ngTemplateOutlet="sliderPropertyCell; + context: { property: 'sliderMin', value: variable.sliderMin }"> + </ng-container>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
engine/equations.cc(1 hunks)engine/mdlReader.cc(1 hunks)engine/slider.h(1 hunks)engine/variableSummary.h(1 hunks)engine/variableValue.cc(6 hunks)engine/variableValue.h(3 hunks)gui-js/libs/shared/src/lib/backend/minsky.ts(4 hunks)gui-js/libs/ui-components/src/lib/summary/summary.component.html(3 hunks)gui-js/libs/ui-components/src/lib/summary/summary.component.ts(2 hunks)gui-js/libs/ui-components/src/lib/wiring/wiring.component.ts(1 hunks)model/cairoItems.cc(1 hunks)model/minsky.cc(1 hunks)model/variable.cc(4 hunks)model/variable.h(2 hunks)model/variablePane.cc(0 hunks)schema/schema3.cc(1 hunks)schema/schema3.h(1 hunks)test/00/RESTService.sh(1 hunks)test/00/editItems.sh(0 hunks)test/00/t0030a.sh(0 hunks)test/00/textInput.sh(0 hunks)test/testModel.cc(3 hunks)test/testPubTab.cc(2 hunks)
💤 Files with no reviewable changes (4)
- model/variablePane.cc
- test/00/editItems.sh
- test/00/textInput.sh
- test/00/t0030a.sh
🔇 Additional comments (44)
gui-js/libs/ui-components/src/lib/summary/summary.component.ts (1)
86-89: Verify broader implications of nullifying document-wide key events.
Setting document.body.onkeydown = null; and document.body.onkeyup = null; can cause unexpected side effects if other parts of the application rely on those handlers. Verify that there's no conflict with the rest of your app.
test/testPubTab.cc (2)
136-137: Check for uninitialized slider bounds.
Explicitly setting sliderBoundsSet = false and then calling initSliderBounds() might mask deeper initialization issues. Verify that your variable's vValue() is always non-null and properly initialized before usage.
149-149: Confirm test alignment with slider logic.
The test checks that var->value() matches var->sliderMax() after a drag event. Ensure this is the intended behavior for your slider logic—particularly if sliderStep or fractional increments are introduced.
engine/variableValue.h (2)
29-29: Include for slider functionality
The addition of #include "slider.h" is essential for the newly introduced slider inheritance, ensuring the Slider-related methods and members are accessible.
45-45: Inheritance from Slider
Inheriting from Slider centralizes slider-related operations in VariableValueData. Ensure you are not breaking encapsulation. Consider confirming that Slider methods are logically consistent with ITensorVal.
schema/schema3.h (1)
146-146: Unconditional slider initialization
Resetting the slider unconditionally in the Item constructor ensures consistent slider behavior. Verify that scenarios calling this constructor actually rely on the new Slider initialization, and handle default or missing slider parameters gracefully.
model/variable.h (1)
246-260: Introduction of slider accessor methods
The newly added methods (initSliderBounds, sliderMin, sliderMax, sliderStep, sliderStepRel, and enableSlider) clarify slider property management. Verify that any legacy slider references have been removed or migrated. Also, confirm that bounds checking is in place for sliderMin <= sliderMax and that step sizes are not zero or negative.
engine/mdlReader.cc (2)
369-369: Null check for variable value
Wrapping v->vValue() in an if (auto vv=...) block correctly prevents null pointer dereferences. This ensures that slider parameters are only applied to valid VariableValue objects.
388-388: Default to initSliderBounds
Fallback to initSliderBounds() when no slider specification is provided preserves older behavior. Ensure that default settings make sense (e.g., do not produce zero or negative bounds) for diverse usage scenarios.
engine/variableValue.cc (6)
32-32: Good addition for slider functionality integration.
Including "slider.rcd" makes the slider definitions accessible within this file. No issues found with this inclusion.
218-218: Confirm consistent usage of tensorInit.
The check if (tensorInit.rank()>0 && (!isFlowVar() || m_type==parameter || !cminsky().definingVar(valueId()))) aligns well with existing logic. Ensure that all external or flow-bound calls also respect this condition consistently.
231-234: Avoid potential step mismatch.
incrSlider applies step*(sliderStepRel? value(): 1)*sliderStep and then calls sliderSet. This might cause unintended big jumps if value() is large and sliderStepRel is true. Consider adding a safety clamp if steps become unbounded.
257-268: Check for edge cases with negative values.
adjustSliderBounds() sets the slider bounds to ±10×value if sliderMax<value() or sliderMin>value(). This is logical, but might lead to surprising expansions if the user is working at or near zero in some exotic scenario. Confirm you truly want 10× expansions.
296-296: Validate dimension expansion logic.
dims.push_back(tmp) appends the parsed dimension to the container. Make sure you handle any invalid or zero values in tmp to avoid dimension mismatch or memory overhead from large dimensions.
584-584: Include new slider fields in summary logic.
Appending sliderStep, sliderMin, sliderMax to the Summary struct is consistent with the new slider functionality. Great for ensuring UI reflection.
model/variable.cc (16)
547-547: Conditional slider visibility check.
return enableSlider() && ...; ensures we only show the slider if explicitly enabled. This is clear and consistent with the encapsulated enableSlider() logic.
550-550: Clarify combined conditions.
(type()==parameter || vv->enableSlider) might cause a hidden parameter variable to skip if enableSlider is off. Verify that’s intended if a user sets a parameter but disables the slider.
553-553: Centralized slider bounds initialization.
initSliderBounds() delegates bounds logic to the underlying VariableValue. This reduces duplication, which is a positive design decision.
556-556: Null check is present.
if (auto vv=vValue()) vv->initSliderBounds(); uses a safe guard. Looks correct and prevents null pointer access.
564-566: Handle arrow up or right.
Incrementing the slider on key press is consistent. Checking !minsky().running before calling requestReset() is also a logical step ensuring environment is paused.
568-570: Handle arrow left or down.
Similar logic to lines 564–566 for decrement. Consistency is good.
630-635: Getter uses correct safe return.
Returns 0 if vValue() is null. Approved approach to avoid crashes.
638-643: Setter ensures safe assignment.
sliderMin(double x) const ... vv->sliderMin=x ensures we do not attempt to write to a null pointer. Good design.
645-650: Safe retrieval for sliderMax.
Same pattern as sliderMin. Straightforward.
652-657: Safe assignment for sliderMax.
Identical logic to sliderMin. No issues.
659-664: Getter for sliderStep.
Returns 0 if vValue() is null. No issues flagged.
666-671: Setter for sliderStep.
Consistent approach with other slider properties. No concerns.
673-678: Boolean getter for sliderStepRel.
No issues. Well-structured.
680-685: Boolean setter for sliderStepRel.
Same pattern. Implementation is consistent.
687-692: Getter for enableSlider.
No specific concerns.
694-699: Setter for enableSlider.
Updates vv->enableSlider if available. Straightforward.
engine/equations.cc (1)
538-538: Validate null checks before adjusting slider bounds.
if (auto vv=iv->vValue()) vv->adjustSliderBounds(); is a good safety check. Ensure we do not skip necessary steps if the vValue() happens to be null. Possibly log or handle fallback if it's unexpectedly null.
test/testModel.cc (4)
502-504: Test correctness of slider property setters.
Confirm the test covers cases for default values, boundary conditions, and ensures the updated slider bounds are reflected in the variable's actual range.
522-522: Clamping behavior verification.
CHECK_EQUAL(cv->sliderMax(), cv->value()); confirms the slider is capped at the maximum. No issues.
526-526: Ensuring lower bound clamp.
CHECK_EQUAL(cv->sliderMin(), cv->value()); confirms the slider is clamped at the minimum. Good boundary test.
922-924: Arrow key increments tested thoroughly.
sliderMin(0), sliderMax(2000), sliderStep(100) sets a coarser step. Arrows verify stepping in large increments. The test ensures correct clamping at 0 and 2000.
model/minsky.cc (1)
913-913: Ensure mini-plot reinitialization is necessary.
It’s good that resetMiniPlot() is called here to ensure that the mini-plot displays the updated state. However, if resetMiniPlot() is also being invoked in other parts of the reset logic, verify that this additional call is truly needed and doesn’t cause duplicated effort.
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
294-297: Maintain consistency in slider attribute accessors.
All of these methods (sliderMax, sliderMin, sliderStep, sliderStepRel) share similar signatures returning or modifying slider-related properties. Ensure that any validation or type-checking is consistent across all these methods since subtle differences can introduce bugs.
test/00/RESTService.sh (1)
12-12: Validate new slider attributes in test output.
The test now checks for additional fields: enableSlider, sliderBoundsSet, sliderMax, sliderMin, sliderStep, and sliderStepRel. Ensure all these attributes are indeed set in the expected contexts so that the test remains robust and meaningful.
model/cairoItems.cc (1)
99-103: Handle potential edge cases when accessing sliderStep.
You are correctly checking whether sliderStep is too small or NaN before initializing slider bounds. This helps prevent invalid slider steps that can arise from uninitialized or corrupted data. Carefully confirm that there is no concurrent modification of vv->sliderStep in another thread that could lead to race conditions here.
gui-js/libs/ui-components/src/lib/summary/summary.component.html (3)
19-22: LGTM: Table header structure for slider properties.
The conditional display of slider-specific columns (Step, Min, Max) for parameter type variables is well-structured and follows a logical order.
63-77: LGTM: Units cell implementation.
The editable units cell is well-implemented with proper event handling and LaTeX support.
94-102: LGTM: Slider Step cell implementation.
The slider step cell is properly implemented with value truncation and follows consistent patterns.
schema/schema3.cc (1)
529-533: LGTM: Improved encapsulation for slider properties.
The change from direct assignment to setter methods improves encapsulation and aligns with object-oriented principles.
| case 'sliderMin': | ||
| this.electronService.minsky.variableValues.elem(this.editRow.valueId).sliderMin(+this.editCellContents); | ||
| break; | ||
| case 'sliderMax': | ||
| this.electronService.minsky.variableValues.elem(this.editRow.valueId).sliderMax(+this.editCellContents); | ||
| break; | ||
| case 'sliderStep': | ||
| this.electronService.minsky.variableValues.elem(this.editRow.valueId).sliderStep(+this.editCellContents); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add user input validation for slider parameters.
You're directly casting strings to numbers with +this.editCellContents. If the input is invalid (e.g., non-numeric), the call might silently produce NaN. Consider adding explicit checks and error handling or defaulting to a safe value.
Example approach:
case 'sliderStep':
- this.electronService.minsky.variableValues.elem(this.editRow.valueId).sliderStep(+this.editCellContents);
+ const parsed = parseFloat(this.editCellContents);
+ if (!isNaN(parsed)) {
+ this.electronService.minsky.variableValues.elem(this.editRow.valueId).sliderStep(parsed);
+ } else {
+ console.warn("Invalid sliderStep input:", this.editCellContents);
+ }
break;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
NaN is not a problem here
There was a problem hiding this comment.
@highperformancecoder, understood. We’ll skip that suggestion and assume the current approach with NaN results is acceptable for your use case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engine/variableValue.cc (1)
231-234: Consider validating step parameterThe step parameter should be validated to prevent potential issues with extreme values.
void VariableValue::incrSlider(double step) { + if (std::isnan(step) || std::isinf(step)) return; sliderSet(value()+step*(sliderStepRel? value(): 1)*sliderStep); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engine/slider.h(1 hunks)engine/variableValue.cc(6 hunks)model/cairoItems.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/slider.h
- model/cairoItems.cc
🔇 Additional comments (4)
engine/variableValue.cc (4)
32-32: LGTM: Include statement properly placed
The new include for slider functionality is appropriately placed among other similar RCD includes.
257-268: LGTM: Robust bounds adjustment implementation
The method properly handles size and NaN checks before adjusting bounds.
584-584: LGTM: Proper inclusion of slider properties in summary
The slider properties are correctly added to the Summary struct, maintaining consistency with the new slider functionality.
222-229: Validate slider bounds initialization
The method assumes sliderMin and sliderMax are properly initialized. Consider adding validation or initialization check.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/variableValue.cc(6 hunks)
🔇 Additional comments (3)
engine/variableValue.cc (3)
584-587: LGTM: Summary struct updated correctly
The slider-related fields are properly added to the Summary struct.
236-255: Protect against potential division by zero in maxSliderSteps
The method calls maxSliderSteps() which might involve division operations. Ensure it's protected against division by zero when sliderMax == sliderMin.
257-268: Consider thread safety for sliderBoundsSet flag
The sliderBoundsSet flag is modified without synchronization, which could lead to race conditions if accessed from multiple threads.
CodeRabbit suggestion Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
engine/variableValue.cc (2)
234-237: Add input validation for the step parameter.The
stepparameter should be validated to ensure it's finite to maintain consistency withsliderSet.void VariableValue::incrSlider(double step) { + if (!std::isfinite(step)) { + step = 0; + } sliderSet(value()+step*(sliderStepRel? value(): 1)*sliderStep); }
260-271: Optimize the adjustSliderBounds method.The
sliderBoundsSet=trueassignment at line 269 is redundant as it's already set to true when bounds are initialized. Consider removing it to improve code clarity.void VariableValue::adjustSliderBounds() const { if (size()==1 && !isnan(value())) // make sure sliderBoundsSet is defined. for tickets 1258/1263 { if (sliderMax<value()) sliderMax=value()? 10*value():1; if (sliderMin>=value()) sliderMin=value()? -10*value():-1; sliderStep=maxSliderSteps(); - sliderBoundsSet=true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/variableValue.cc(6 hunks)
🧰 Additional context used
📓 Learnings (1)
engine/variableValue.cc (1)
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
🔇 Additional comments (3)
engine/variableValue.cc (3)
32-32: LGTM!
The inclusion of slider.rcd is correct and necessary for the slider functionality.
354-374: Add error handling for dimension lookup failure.
The dimension lookup at line 360 should handle the case where the dimension is not found.
587-587: LGTM!
The slider properties are correctly included in the Summary struct.
… Remove sliderBoundsSet.
…oder/minsky into feature-256-summary-tab
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
model/cairoItems.cc (1)
101-103: Consider extracting the slider calculation into a helper method.The calculation logic is complex and could benefit from being extracted into a well-named helper method for better readability and reusability.
Here's a suggested refactoring:
double RenderVariable::handlePos() const { if (auto vv=var.vValue()) { vv->adjustSliderBounds(); assert(vv->sliderMin<vv->sliderMax); - return (w<0.5*var.iWidth()? 0.5*var.iWidth() : w)*(vv->value()-0.5*(vv->sliderMin+vv->sliderMax))/(vv->sliderMax-vv->sliderMin); + return calculateNormalizedSliderPosition(vv); } else return 0; } + +double RenderVariable::calculateNormalizedSliderPosition(const VariableValue* vv) const +{ + double effectiveWidth = w < 0.5*var.iWidth() ? 0.5*var.iWidth() : w; + double normalizedValue = (vv->value() - 0.5*(vv->sliderMin + vv->sliderMax)) / (vv->sliderMax - vv->sliderMin); + return effectiveWidth * normalizedValue; +}engine/slider.h (1)
33-34: Double-check initialization for completenessInitializing
sliderMintostd::numeric_limits<double>::max()andsliderMaxto-sliderMineffectively setssliderMin>sliderMax. While it may be an intentional sentinel, any usage or comparison before adjusting them should be handled carefully.engine/variableValue.h (2)
45-45: Inheritance fromSlider
VariableValueDatanow inherits from bothITensorValandSlider. Confirm thatSliderfunctionality is indeed required at this lower-level data structure rather than a composition approach.
194-194: Ensure consistent usage ofadjustSliderBounds()
adjustSliderBounds()is declared asconst, yet it adjusts the slider fields. Verify that these changes are safe and consistent with the const correctness of your code. Consider removingconstor marking the relevant fields as mutable if they must be changed inside this method.model/variable.h (1)
247-247: Method name clarity
void adjustSliderBounds();is short and sweet, but consider a more descriptive name likesyncSliderBoundsWithValue()to reflect its purpose more explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
engine/mdlReader.cc(1 hunks)engine/slider.h(1 hunks)engine/variableValue.cc(6 hunks)engine/variableValue.h(3 hunks)gui-js/libs/shared/src/lib/backend/minsky.ts(4 hunks)gui-js/libs/ui-components/src/lib/create-variable/create-variable.component.ts(1 hunks)model/cairoItems.cc(1 hunks)model/variable.cc(4 hunks)model/variable.h(2 hunks)test/00/RESTService.sh(1 hunks)test/testPubTab.cc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/00/RESTService.sh
- test/testPubTab.cc
- engine/mdlReader.cc
- engine/variableValue.cc
🔇 Additional comments (15)
model/cairoItems.cc (1)
99-106: LGTM! Robust null pointer and bounds handling.
The changes effectively address potential issues by:
- Adding null pointer check before accessing the variable value
- Ensuring slider bounds are properly adjusted
- Using assertion to prevent division by zero
- Providing appropriate fallback value
gui-js/libs/ui-components/src/lib/create-variable/create-variable.component.ts (1)
187-187: Ensure Proper Error Handling or Defaults
The new item.adjustSliderBounds() call looks good for initializing slider boundaries. However, consider handling unexpected cases such as invalid or missing item references. Ensure you have a fallback or relevant error handling if item is undefined or if slider parameters are out of bounds.
engine/slider.h (2)
30-30: Keep an eye on the mutable keyword usage
mutable bool sliderStepRel=false; is declared as mutable. If this member is accessed concurrently, ensure thread-safety or confirm it is only ever manipulated from a single thread.
37-41: Consider boundary checks for sliderStep
The maxSliderSteps() function prevents more than 10,000 steps. Check if sliderStep can be zero or negative after initialization or user input, which might lead to infinite or undefined values for (sliderMax - sliderMin) / sliderStep. You might reuse or confirm the fix from previous comments ensuring sliderStep > 0.
engine/variableValue.h (2)
29-29: Ensure unambiguous inclusion hierarchy
Including "slider.h" here couples VariableValue directly to Slider. Verify that it won’t cause circular dependencies in other parts of the code base.
[approve]
188-193: Evaluate potential concurrency constraints
Methods sliderSet(double x) and incrSlider(double step) can mutate the underlying data. If multiple threads access or modify VariableValue concurrently, ensure proper synchronization or a lock to avoid data races on sliderMin, sliderMax, and sliderStep.
model/variable.h (1)
89-89: Slider inheritance removal
VariableBase no longer inherits from Slider. This is a good step towards separation of concerns. Confirm that all slider functionalities are fully migrated to VariableValue and that any old calls are updated or removed.
model/variable.cc (7)
547-550: LGTM! Improved encapsulation of slider visibility logic
The implementation correctly determines slider visibility by checking enableSlider() and appropriate type/value conditions. The change from direct property access to method call improves encapsulation.
553-556: LGTM! Clean delegation to VariableValue
The method properly checks for null before delegating to the VariableValue instance.
564-570: LGTM! Improved slider increment handling
The implementation properly handles arrow key navigation by delegating to incrSlider() and correctly manages simulation state.
613-620: LGTM! Robust slider position calculation
The implementation handles complex slider position calculations correctly, taking into account:
- Zoom factor and rotation adjustments
- Proper bounds calculation
- Snapping to hatch marks using sliderStep
630-643: LGTM! Clean slider minimum value accessors
The getter and setter implementations properly handle null checks and follow a consistent pattern.
776-782: LGTM! Robust slider value formatting
The implementation correctly handles both relative and absolute slider steps, with appropriate scale calculations using log10.
645-699: LGTM! Consistent slider property accessors
All slider property accessors (max, step, stepRel, enable) follow a consistent implementation pattern with proper null checks.
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
Line range hint 2281-2317: LGTM! Complete TypeScript interface for slider functionality
The TypeScript definitions correctly map all new slider-related methods from the C++ implementation with proper typing.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engine/slider.h(1 hunks)engine/variableValue.cc(6 hunks)engine/variableValue.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/slider.h
🧰 Additional context used
📓 Learnings (1)
engine/variableValue.cc (3)
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in `variableValue.cc` rather than throwing an exception if a dimension is not found.
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: model/variable.h:249-260
Timestamp: 2024-12-30T09:44:42.105Z
Learning: The user clarified that the newly introduced slider parameter methods in VariableBase (sliderMin, sliderMax, sliderStep, etc.) are required for delegation from Variable to VariableValue, maintaining JS compatibility. Negative or zero sliderStep values are addressed in other parts of the codebase.
🔇 Additional comments (5)
engine/variableValue.h (3)
189-190: LGTM: Method declaration for setting slider value.
The method declaration is clear and follows the class's naming conventions.
191-192: LGTM: Method declaration for incrementing slider value.
The method declaration is clear and follows the class's naming conventions.
194-195: LGTM: Method declaration for adjusting slider bounds.
The method declaration is clear and follows the class's naming conventions.
engine/variableValue.cc (2)
331-351: LGTM: Silent failure for dimension lookup.
The implementation correctly follows the user's preference to fail silently when a dimension is not found, as indicated in the retrieved learnings.
222-230: 🛠️ Refactor suggestion
Add bounds validation in sliderSet.
The implementation needs additional validation:
- The bounds themselves should be validated before use.
- The order of operations could be optimized.
Apply this diff to improve the implementation:
void VariableValue::sliderSet(double x)
{
if (!isfinite(x)) return;
+ if (!isfinite(sliderMin) || !isfinite(sliderMax) || sliderMin >= sliderMax) {
+ sliderMin = -1;
+ sliderMax = 1;
+ }
if (x<sliderMin) x=sliderMin;
if (x>sliderMax) x=sliderMax;
- sliderStep=maxSliderSteps();
init(to_string(x));
setValue(x);
+ sliderStep=maxSliderSteps();
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engine/variableValue.cc (1)
237-248: Consider adding documentation for the slider bounds logic.The implementation is correct, particularly:
- Using abs() for symmetric bounds
- Maintaining a 2-unit range (-1 to 1) when value is zero
Consider adding a comment explaining the bounds calculation logic:
void VariableValue::adjustSliderBounds() { + // Maintains symmetric bounds around the current value + // When value is zero, uses -1 to 1 range + // Otherwise, uses ±10 times the absolute value if (size()==1 && !isnan(value()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/variableValue.cc(6 hunks)
🧰 Additional context used
📓 Learnings (1)
engine/variableValue.cc (4)
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:237-248
Timestamp: 2024-12-30T10:10:13.811Z
Learning: When the variable value is zero, the slider range is from -1 to 1 (ensuring a 2-unit range), so the assertion (sliderMin < sliderMax) never fails.
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in `variableValue.cc` rather than throwing an exception if a dimension is not found.
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: engine/variableValue.cc:32-32
Timestamp: 2024-12-29T09:15:41.817Z
Learning: .rcd files in the Minsky build are generated from corresponding .h files rather than from .cd files.
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#538
File: model/variable.h:249-260
Timestamp: 2024-12-30T09:44:42.105Z
Learning: The user clarified that the newly introduced slider parameter methods in VariableBase (sliderMin, sliderMax, sliderStep, etc.) are required for delegation from Variable to VariableValue, maintaining JS compatibility. Negative or zero sliderStep values are addressed in other parts of the codebase.
🔇 Additional comments (5)
engine/variableValue.cc (5)
32-32: LGTM: Required include for slider functionality.
The inclusion of slider.rcd is correct as per the build system requirements.
222-230: LGTM: Robust slider value setting implementation.
The method correctly handles:
- Input validation
- Bounds checking
- Value propagation
232-235: LGTM: Efficient slider increment implementation.
The method correctly delegates validation to sliderSet and handles both relative and absolute step modes.
331-351: LGTM: Robust dimension handling implementation.
The implementation correctly:
- Handles tensor initialization
- Silently handles missing dimensions
- Validates tensor rank before assignment
564-564: LGTM: Summary includes slider properties.
Correctly adds slider properties (step, min, max) to the variable summary.
This change is
Summary by CodeRabbit
Release Notes
New Features
sliderSet,incrSlider, andadjustSliderBounds.sliderMin,sliderMax, andsliderStep.Improvements
Bug Fixes
UI Changes
These changes focus on providing more flexible and safer slider controls across the application, with particular emphasis on parameter management and user interaction.