feat: gauge element #4141#4228
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a ChangesGauge Element Graphics System
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a0a783-d01a-445f-a7de-a7045a799b8d
⛔ Files ignored due to path filters (30)
companion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_empty_thresholds_-_nothing_drawn.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveAmount_0_-_inactive_portions_invisible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveAmount_100_-_inactive_same_as_active_colour.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveStyle_dimmed_-_inactive_portions_darkened.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_multiSegment_false_-_single_colour_for_entire_active_region.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_orientation_vertical_reverse_false_-_fills_from_bottom.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_orientation_vertical_reverse_true_-_fills_from_top.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_reverse_true_-_fills_from_right.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_in_non-square_element_-_stays_circular.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_multiSegment_false_value_75_-_single_colour_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_reverse_true_value_75_-_counter-clockwise.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_roundedEnds_false_value_75_-_flat_ends.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_thick_thickness_40.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_thin_thickness_8.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_0_-_inactive_arc_only_dark_bg_makes_it_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_100_-_fully_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_33_-_one_colour_within_first_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_50_-_midway_through_first_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_66_-_exactly_at_first_threshold_boundary.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_75_-_crossing_into_yellow_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_75_dimmed_inactive_-_both_halves_clearly_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_90_-_crossing_into_red_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_single_threshold_-_full_bar_one_colour.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_unsorted_thresholds_-_sorted_before_rendering.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_0_-_only_inactive_background_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_100_-_all_segments_active_no_inactive.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_50_-_first_segment_partially_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_75_-_two_segments_active_green_yellow_red_inactive.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/Image_drawing_arcStroke_snapshot_-_full_circle_and_quarter_arcs.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/Image_drawing_arcStroke_snapshot_-_thick_vs_thin_arc.pngis excluded by!**/*.png
📒 Files selected for processing (17)
companion/lib/Controls/ControlTypes/Button/LayerDefaults.tscompanion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/Graphics/ConvertGraphicsElements/Helper.tscompanion/test/Graphics/Image.test.tscompanion/test/Graphics/LayeredRenderer.test.tsshared-lib/lib/Graphics/ElementPropertiesSchemas.tsshared-lib/lib/Graphics/ImageBase.tsshared-lib/lib/Graphics/LayeredRenderer.tsshared-lib/lib/Model/Options.tsshared-lib/lib/Model/StyleLayersModel.tsshared-lib/lib/ValidateInputValue.tsshared-lib/lib/__tests__/validate-input-value.test.tstools/generate_graphics_types.mtswebui/src/Buttons/EditButton/LayeredButtonEditor/Buttons.tsxwebui/src/Components/TableInputField.stories.tsxwebui/src/Components/TableInputField.tsxwebui/src/Controls/OptionsInputField.tsx
|
@thedist I feel you may have some input on the design/functionality here. |
Julusian
force-pushed
the
feat/gauge-element
branch
from
0a0cb1c to
10587b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
webui/src/Components/ListInputField.tsx (1)
113-142: ⚡ Quick winConsider using functional updates for
setValueto prevent stale closure reads.The current callbacks (
addRow,removeRow,moveRow,updateCell) close overrowsand callsetValue([...rows, ...]). IfsetValueis triggered multiple times before re-render completes (e.g., rapid user clicks), both calls might read the same stalerowsvalue, potentially losing updates.Using functional updates makes the callbacks more stable and prevents this edge case:
♻️ Suggested refactor using functional updates
const addRow = useCallback(() => { - setValue([...rows, newRow(definition.fields)]) -}, [rows, definition.fields, setValue]) + setValue((prevRows) => [...prevRows, newRow(definition.fields)]) +}, [definition.fields, setValue]) const removeRow = useCallback( (rowIndex: number) => { - setValue(rows.filter((_, i) => i !== rowIndex)) + setValue((prevRows) => prevRows.filter((_, i) => i !== rowIndex)) }, - [rows, setValue] + [setValue] ) const moveRow = useCallback( (rowIndex: number, direction: -1 | 1) => { + setValue((prevRows) => { + const rows = prevRows const next = rowIndex + direction if (next < 0 || next >= rows.length) return const updated = [...rows] ;[updated[rowIndex], updated[next]] = [updated[next], updated[rowIndex]] - setValue(updated) + return updated + }) }, - [rows, setValue] + [setValue] ) const updateCell = useCallback( (rowIndex: number, fieldId: string, cellValue: ExpressionOrValue<JsonValue | undefined>) => { - setValue( - rows.map((row, i) => (i === rowIndex ? { ...row, [fieldId]: cellValue as ExpressionOrValue<JsonValue> } : row)) - ) + setValue((prevRows) => + prevRows.map((row, i) => (i === rowIndex ? { ...row, [fieldId]: cellValue as ExpressionOrValue<JsonValue> } : row)) + ) }, - [rows, setValue] + [setValue] )This also makes the callbacks more stable (recreated less often) since they no longer depend on
rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c5e00cd-7c97-43af-9cc8-9a67602cafc2
⛔ Files ignored due to path filters (32)
companion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_box_properties_box_with_opacity_-_semi-transparent_over_another_element.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_empty_thresholds_-_nothing_drawn.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveAmount_0_-_inactive_portions_invisible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveAmount_100_-_inactive_same_as_active_colour.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_inactiveStyle_dimmed_-_inactive_portions_darkened.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_multiSegment_false_-_single_colour_for_entire_active_region.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_orientation_vertical_reverse_false_-_fills_from_bottom.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_orientation_vertical_reverse_true_-_fills_from_top.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_reverse_true_-_fills_from_right.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_in_non-square_element_-_stays_circular.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_multiSegment_false_value_75_-_single_colour_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_reverse_true_value_75_-_counter-clockwise.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_roundedEnds_false_value_75_-_flat_ends.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_thick_thickness_40.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_thin_thickness_8.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_0_-_inactive_arc_only_dark_bg_makes_it_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_100_-_fully_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_33_-_one_colour_within_first_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_50_-_midway_through_first_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_66_-_exactly_at_first_threshold_boundary.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_75_-_crossing_into_yellow_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_75_dimmed_inactive_-_both_halves_clearly_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_90_-_crossing_into_red_segment.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_single_threshold_-_full_bar_one_colour.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_unsorted_thresholds_-_sorted_before_rendering.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_0_-_only_inactive_background_visible.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_100_-_all_segments_active_no_inactive.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_50_-_first_segment_partially_active.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_value_75_-_two_segments_active_green_yellow_red_inactive.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_group_properties_group_with_rotation.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/Image_drawing_arcStroke_snapshot_-_full_circle_and_quarter_arcs.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/Image_drawing_arcStroke_snapshot_-_thick_vs_thin_arc.pngis excluded by!**/*.png
📒 Files selected for processing (23)
companion/lib/Controls/ControlTypes/Button/LayerDefaults.tscompanion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/Graphics/ConvertGraphicsElements/Helper.tscompanion/test/Graphics/ConvertGraphicsElements.test.tscompanion/test/Graphics/Image.test.tscompanion/test/Graphics/LayeredRenderer.test.tsshared-lib/lib/Graphics/ElementPropertiesSchemas.tsshared-lib/lib/Graphics/ImageBase.tsshared-lib/lib/Graphics/LayeredRenderer.tsshared-lib/lib/Model/Options.tsshared-lib/lib/Model/StyleLayersModel.tsshared-lib/lib/ValidateInputValue.tsshared-lib/lib/__tests__/validate-input-value.test.tstools/generate_graphics_types.mtswebui/src/Buttons/EditButton/LayeredButtonEditor/Buttons.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/ElementPropertiesEditor.tsxwebui/src/Components/ListInputField.stories.tsxwebui/src/Components/ListInputField.tsxwebui/src/Components/TableInputField.stories.tsxwebui/src/Components/TableInputField.tsxwebui/src/Components/__tests__/ListInputField.test.tsxwebui/src/Components/__tests__/TableInputField.test.tsxwebui/src/Controls/OptionsInputField.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- webui/src/Buttons/EditButton/LayeredButtonEditor/Buttons.tsx
- tools/generate_graphics_types.mts
- companion/lib/Controls/ControlTypes/Button/LayerDefaults.ts
- companion/lib/Graphics/ConvertGraphicsElements/Helper.ts
- shared-lib/lib/Model/StyleLayersModel.ts
- companion/test/Graphics/Image.test.ts
- shared-lib/lib/Graphics/ImageBase.ts
- webui/src/Components/TableInputField.tsx
- webui/src/Components/TableInputField.stories.tsx
- companion/lib/Graphics/ConvertGraphicsElements.ts
- shared-lib/lib/Model/Options.ts
- shared-lib/lib/Graphics/LayeredRenderer.ts
- shared-lib/lib/ValidateInputValue.ts
|
What's the best way to give this a test? I was going to check it out with the Voicemeeter module as that's a good one to test the performance of lots of meters (and with the imagebuffers not working with the 5.0 graphics system is one of the main modules I need ready for 5.0's release) but was expecting the 'Guage' element to be similar to the group/text/image/line/circle/etc... elements but it's not showing in that list. so I'm wondering if it's an element somewhere else and I'm misunderstanding where it's located? |
|
yeah this is just another element type: only in this branch. not available in presets yet either |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
webui/src/Controls/OptionsInputField.tsx (1)
150-150: 💤 Low valueMinor: Consider using the imported constant for consistency.
You're using an inline object
{ variables: true, local: true }here, butInputFeatures.tsxexportsExpressionModeFeatureswith the same values. Using the constant would be more maintainable if these values ever need to change together.💡 Optional consistency improvement
+import { ExpressionModeFeatures, getInputFeatures, InputFeatureIcons, type InputFeatureIconsProps } from './InputFeatures.js' -import { getInputFeatures, InputFeatureIcons, type InputFeatureIconsProps } from './InputFeatures.js' ... - <OptionLabel option={option} features={isInExpressionMode ? { variables: true, local: true } : features} /> + <OptionLabel option={option} features={isInExpressionMode ? ExpressionModeFeatures : features} />webui/src/Components/PropertyFieldRow.tsx (1)
79-80: Document/guard expression semantics whendisableAutoExpressionis trueIn
PropertyFieldRow, thedisableAutoExpressionbranch bypassesFieldOrExpressionand passeschildren({ value: value.value }). SinceExpressionOrValue<T>allows{ isExpression: true; value: string }, any existing expression payload would be treated as a literal JSON string, and subsequent edits viasetInnerValuewill force{ isExpression: false }. Add a short invariant/comment (or upstream normalization) clarifying that this is intentional (i.e.,disableAutoExpressionimpliesvalue.isExpression === false, or that preserving the literal string is desired).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: feeca19d-1446-4b1b-83c3-ad0a84ecb3eb
⛔ Files ignored due to path filters (4)
companion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_empty_segments_-_nothing_drawn.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_ring_value_66_-_exactly_at_first_segment_boundary.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_single_segment_-_full_bar_one_colour.pngis excluded by!**/*.pngcompanion/test/Graphics/__snapshots__/GraphicsLayeredButtonRenderer_gauge_element_unsorted_segments_-_sorted_before_rendering.pngis excluded by!**/*.png
📒 Files selected for processing (15)
companion/lib/Controls/ControlTypes/Button/LayerDefaults.tscompanion/lib/Graphics/ConvertGraphicsElements.tscompanion/test/Graphics/ConvertGraphicsElements.test.tscompanion/test/Graphics/LayeredRenderer.test.tsshared-lib/lib/ValidateInputValue.tswebui/src/Buttons/EditButton/LayeredButtonEditor/ControlOptionsEditor.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/ElementPropertiesEditor.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/ElementPropertiesUtil.tsxwebui/src/Components/ListInputField.tsxwebui/src/Components/PropertyFieldRow.tsxwebui/src/Components/TableInputField.tsxwebui/src/Controls/Components/AddElementPickerModal.tsxwebui/src/Controls/InputFeatures.tsxwebui/src/Controls/OptionsInputField.tsxwebui/src/Surfaces/EditPanelConfigField.tsx
✅ Files skipped from review due to trivial changes (2)
- webui/src/Surfaces/EditPanelConfigField.tsx
- webui/src/Buttons/EditButton/LayeredButtonEditor/ControlOptionsEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- companion/lib/Controls/ControlTypes/Button/LayerDefaults.ts
- webui/src/Components/TableInputField.tsx
- webui/src/Buttons/EditButton/LayeredButtonEditor/ElementPropertiesEditor.tsx
- shared-lib/lib/ValidateInputValue.ts
- webui/src/Components/ListInputField.tsx
- companion/test/Graphics/LayeredRenderer.test.ts
- companion/lib/Graphics/ConvertGraphicsElements.ts
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc3d289a-75e8-4642-aaf0-7af34af4571d
📒 Files selected for processing (3)
companion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/Graphics/ConvertGraphicsElements/Helper.tsshared-lib/lib/Graphics/ElementPropertiesSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- shared-lib/lib/Graphics/ElementPropertiesSchemas.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Graphics/ConvertGraphicsElements/Helper.ts (1)
78-81:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
#getValuereturnsundefinedfor missing properties, causing crashes in partial row data.This issue was raised in a previous review but remains unaddressed. When
forRownormalizes an incomplete row (e.g.,{ color: 0xff0000 }missing thevaluekey),#getValue('value')returnsundefinedinstead of anExpressionOrValueobject. Subsequent calls likegetNumber('value', 0)then accessundefined.isExpression, throwing aTypeError.From the gauge converter (context snippet 2):
const rowHelper = helper.forRow(row) return { value: rowHelper.getNumber('value', 0), // crashes if row missing 'value' color: rowHelper.getNumber('color', 0), }This is a runtime regression from previous behavior that tolerated missing table cells by defaulting them.
🐛 Required fix
`#getValue`(propertyName: keyof T): ExpressionOrValue<JsonValue | undefined> { const override = this.#elementOverrides?.get(String(propertyName)) - return override ? override : (this.#element as any)[propertyName] + return override ?? (this.#element as any)[propertyName] ?? { isExpression: false, value: undefined } }This ensures
#getValuealways returns a validExpressionOrValueobject, even when the property doesn't exist, maintaining backward compatibility with the previous tolerant parsing behavior.
🧹 Nitpick comments (1)
companion/lib/Graphics/ConvertGraphicsElements/Helper.ts (1)
164-170: ⚡ Quick winConsider guarding against empty
trimmedstring.If
rawisnull,undefined, or an empty string,trimmedbecomes''andtrimmed[0]isundefined. While the current code handles this (JavaScript convertsundefinedto"undefined"instartsWith, no match occurs, anddefaultValueis returned), it relies on type coercion and performs an unnecessary string comparison.For clarity and efficiency, consider adding an early return:
Suggested improvement
getTolerantEnum<TVal extends string>(propertyName: keyof T, values: readonly TVal[], defaultValue: TVal): TVal { const raw = this.getString(propertyName, defaultValue) const trimmed = String(raw ?? '') .trim() .toLowerCase() + if (!trimmed) return defaultValue return values.find((v) => v.toLowerCase().startsWith(trimmed[0])) ?? defaultValue }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29a57ac4-d2aa-4f22-a1d1-4fa203cc37a2
📒 Files selected for processing (3)
companion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/Graphics/ConvertGraphicsElements/Helper.tsshared-lib/lib/Graphics/ElementPropertiesSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- shared-lib/lib/Graphics/ElementPropertiesSchemas.ts
- companion/lib/Graphics/ConvertGraphicsElements.ts
|
Hey, I don't have time for testing it currently, but here are my thoughts:
|
There is the usual size position and rotation. (and enabled and opacity), I just left them out of the screenshot.
I think that will be challenging; which is a general problem with the current rotation. Because the width+height are a percentage, in a non-square button things get a little odd.
That controls the colour of the value. Whether it shows each colour or gets filled with a single colour.
The pan part of this makes sense, I'm not really sure what the others are asking for.
No real objection here, just that reusing the ring thickness for a veritcal/horisontal something will vastly change the behaviour of the drawing relative to the bounding box, or be something the user is forced to change when switching between the styles.
No objection to that. maybe this is really asking for a start/end angle field. This would have overlap with rotation, but that might be the best way to allow this dead range
I'm ok with a min and max value fields, with the collapsing sections having a wall of properties doesnt bother me too much.
Yes and no. It can be done with basic elements, but not at all simple to do. Not too bad for the single colour versions, but if multiple colours then thats an element per colour
For sure future scope. I dont see this as necessary for a first iteration. |
|
I did some testing after some annoying git issues (solved by deleting the branch locally and running the same pull command again lol). So far my main issues are lack of presets as it is a fair amount of work to get them up and running so I definitely expect module presets to be needed (I know there are some people who use modules JUST for the meters feedback they have that lets users put in variables so they use those meters with sources unrelated to the module, perhaps we should consider some generic presets built in to Companion with some pre-made meters to smooth this transition and provide better UX than needing to use separate modules or hand make meters). Secondly, we need to consider what's the best user experience for handling logarithmic values. Should this be handled entirely by the user (in which case modules may need to do more to add appropriate variables), which means we would need more Expression Functions as I don't think there's currently an equivalent of As an example, vMix give audio metre data as a logarithmic amplitude from 0 to 1. This isn't much value to end users (at least it wasn't so far) so I don't expose that as a variable, but instead turn it in to a dB value. Such as an amplitude of 0.07890493, is roughly -22 dB, which is ~50 on a 0 to 100 linear gauge. Without a So personally I think modules should do a lot of the leg work where they can to straight up give users a 0 to 100 value variable that users can paste right into a gauge, but we're also going to need any missing Math functions that are going to be needed so users can do it themselves for modules that don't update.
I agree here. For a lot of circular gauges it's quite common graphically to have a gap in the gauge at the bottom for text, either a label or the textual value that's controlling the gauge.
I can see this as being tough tom implement, as so many different modules handle things differently, like vMix returns the current amplitude of the moment of the request, where as Voicemeeter returns a value that already takes in to account fading so if there was a loud sound a second ago and then silence the returned value would actually be fading from that loud sound and not 0. Add in to this things like update frequency (I generally limit my modules with audio metres to 10 fps and any faster is problematic) and there's already some margin of error with how accurate the gauges would be compared to a 'real' audio metre device. |
Could you do something similar by overlapping two meters, or more for the metering where it shows a peak level as well as the current level.
I assume for a linear meter, you'd use normal sizing to potentially leave space above/below it?
At a bare minimum, the colour thresholds should accept expressions, as then you can use the same expression throughout all value fields (e.g. log(-18) or whatever) rather than having to pre-calculate those, but being able to use a function for the main values.
Consider also a circular audio pan meter might have the gap at the bottom, but e.g. a camera tilt display or stopwatch could have the gap at the side/top.
Do you think anything returns both values already, current and averaged? Actually at least some Shure radio mics return both peak and RMS values. |
|
ok, I've done another pass of this, and expanded out the properties quite a bit (see description for new screenshots) I've renamed various properties, so any existing test configs wont translate across cleanly |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared-lib/lib/Graphics/ImageBase.ts (1)
233-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent
lineCapstate from leaking across subsequent draw calls.At Line 236 and Line 411,
lineCapis written onto the shared canvas context but never restored. If a rounded marker sets'round', later stroke operations that don’t setlineCap(for example open arc/path strokes) can render with unintended caps depending on draw order.Suggested fix
line(x1: number, y1: number, x2: number, y2: number, style: LineStyle): void { + const oldLineCap = this.context2d.lineCap this.context2d.lineWidth = style.width ?? 1 this.context2d.strokeStyle = style.color this.context2d.lineCap = style.cap ?? 'butt' - this.context2d.beginPath() - this.context2d.moveTo(x1, y1) - this.context2d.lineTo(x2, y2) - this.context2d.closePath() - this.context2d.stroke() + try { + this.context2d.beginPath() + this.context2d.moveTo(x1, y1) + this.context2d.lineTo(x2, y2) + this.context2d.closePath() + this.context2d.stroke() + } finally { + this.context2d.lineCap = oldLineCap + } } @@ arcStroke( x: number, y: number, radius: number, @@ ): void { if (radius <= 0 || lineStyle.width <= 0) return + const oldLineCap = this.context2d.lineCap this.context2d.beginPath() this.context2d.arc(x, y, radius, startAngle, endAngle, anticlockwise) this.context2d.strokeStyle = lineStyle.color this.context2d.lineWidth = lineStyle.width this.context2d.lineCap = lineStyle.cap ?? 'butt' - this.context2d.stroke() + try { + this.context2d.stroke() + } finally { + this.context2d.lineCap = oldLineCap + } }Also applies to: 397-413
🧹 Nitpick comments (1)
shared-lib/lib/Model/StyleLayersModel.ts (1)
231-231: ⚡ Quick winUse a dedicated stop-row type for stronger gauge contracts.
stopsis currently modeled asRecord<string, JsonValue>[], but downstream code expects a fixed{ value, color, gradient }shape. Tightening the draw-model type would improve safety and catch drift earlier.Also applies to: 256-256
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 039bec80-c5e4-4efd-af1a-5fa2ce9b77f7
📒 Files selected for processing (7)
companion/lib/Controls/ControlTypes/Button/LayerDefaults.tscompanion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/Graphics/ConvertGraphicsElements/Helper.tsshared-lib/lib/Graphics/ElementPropertiesSchemas.tsshared-lib/lib/Graphics/ImageBase.tsshared-lib/lib/Graphics/LayeredRenderer.tsshared-lib/lib/Model/StyleLayersModel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- shared-lib/lib/Graphics/LayeredRenderer.ts
4 participants
Just another element.
Screencast.From.2026-06-06.21-19-29.mp4
current properties:
Summary by CodeRabbit
Summary of changes