Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #2189 requires the Buttons view to reflect an attached Stream Deck +, enable rotary encoder options by default, and allow manual selection when no device is attached. The PR implements manual su… |
Please add or document automatic selection for an attached Stream Deck + and enable its rotary encoder options by default, with automated tests. For #3205, add the requested preset customization and factory-default reset, or confirm that th… |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: adding the ability to view the button grid as a surface. Out of Scope Changes check The surface model IPC, layout resolution, appearance rendering, grid controls, zoom behavior, dropdown disabled-state support, documentation, and related tests support the view-as-surface feature and …
Full details: Linked Issues check
-
Explanation
Issue
#2189requires the Buttons view to reflect an attached Stream Deck +, enable rotary encoder options by default, and allow manual selection when no device is attached. The PR implements manual surface selection throughGridViewAsControlanduseGridViewAs. The stored state defaults to disabled with no selection, so automatic attached-device selection is not established. The change summary shows no rotary encoder default change or test. Issue#3205requires quick layout switching with non-destructive hiding. The PR provides a surface selector, preserves off-view controls, and supports grid growth. It does not provide the requested configurable name/row/column presets or a factory-default reset.Resolution
Please add or document automatic selection for an attached Stream Deck + and enable its rotary encoder options by default, with automated tests. For
#3205, add the requested preset customization and factory-default reset, or confirm that those requirements are intentionally superseded by the surface-model selector.
- Fix all pre-merge checks with AI
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A surface finds its place in view
Each control takes its shape anew
Colors glow and LEDs gleam
Zoom fits the panel to the scene
Models arrive through updates shared
A grid grows where controls are paired
Buttons take a surface’s view
Comment @coderabbitai help to get the list of available commands.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
So is it just that required to do the "col span" type behaviour on the Neo to get the "long" button at the bottom to align with the four normal buttons above it, or is there some additional work/future enhancement required first? I assume the pain of stuff like the Shuttle with a row of four buttons, then a row of five below will always be a pain/you won't necessarily support different column spacing (I guess like the SD + XL), although in theory with some clever col spans, it ought to be possible to represent. |
|
The current screenshots show the basic 'auto' mode of displaying a surface. Our best guess at how to arrange things with the limited information provided. The part I am still working on is detailed drawing, relying on the surface module providing more details. This is currently achieving the below but it needs some more work (Largely background images, and figuring out some edge cases): |
That already looks pretty awesome for those ones.
Are those edge cases likely to go as far as the complexities of Shuttle which is effectively a doughnut and then the whole as separate controls in terms of the jog shuttle? |
d8a91b5 to
abf4777
Compare
e2e8c4a to
fba3ccd
Compare
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
dd7612af-9b83-4b27-afc6-14d7cb665afb
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (66)
companion/lib/Instance/Surface/ChildHandler.tscompanion/lib/Instance/Surface/IpcTypes.tscompanion/lib/Instance/Surface/Thread/Entrypoint.tscompanion/lib/Preview/Graphics.tscompanion/lib/Surface/Config.tscompanion/lib/Surface/Controller.tscompanion/lib/Surface/Handler.tscompanion/lib/Surface/IP/ElgatoEmulator.tscompanion/lib/Surface/IP/Satellite.tscompanion/lib/Surface/LayoutSummary.tscompanion/lib/Surface/PluginPanel.tscompanion/lib/Surface/PluginSurfaceModels.tscompanion/lib/Surface/Types.tscompanion/test/Preview/Graphics.test.tscompanion/test/Surface/ControllerLayouts.test.tscompanion/test/Surface/ControllerModels.test.tscompanion/test/Surface/LayoutSummary.test.tscompanion/test/Surface/PluginSurfaceModels.test.tsdocs/user-guide/3_config/buttons/index.mdpackage.jsonshared-lib/lib/Model/Common.tsshared-lib/lib/Model/Surfaces.tsshared-lib/lib/ModuleApiVersionCheck.tsshared-lib/lib/SurfaceLayout.tsshared-lib/lib/SurfaceLayoutEstimate.tsshared-lib/lib/__tests__/SurfaceLayout.test.tsshared-lib/lib/__tests__/SurfaceLayoutEstimate.test.tswebui/src/Buttons/ButtonGridPanel.csswebui/src/Buttons/ButtonGridPanel.tsxwebui/src/Buttons/ButtonGridZoomControl.stories.tsxwebui/src/Buttons/ButtonGridZoomControl.tsxwebui/src/Buttons/ButtonInfiniteGrid.tsxwebui/src/Buttons/GridButtonCell.tsxwebui/src/Buttons/GridButtonPreview.tsxwebui/src/Buttons/GridViewAs.csswebui/src/Buttons/GridViewAs.tswebui/src/Buttons/GridViewAsBanner.stories.tsxwebui/src/Buttons/GridViewAsBanner.tsxwebui/src/Buttons/GridViewAsControl.tsxwebui/src/Buttons/GridViewGrowPanel.tsxwebui/src/Buttons/GridZoom.tsxwebui/src/Buttons/SurfaceView/SurfaceCanvas.csswebui/src/Buttons/SurfaceView/SurfaceCanvas.tsxwebui/src/Buttons/SurfaceView/SurfaceLeds.csswebui/src/Buttons/SurfaceView/SurfaceLeds.tsxwebui/src/Buttons/SurfaceView/__tests__/ledLayout.test.tswebui/src/Buttons/SurfaceView/__tests__/surfaceGeometry.test.tswebui/src/Buttons/SurfaceView/ledLayout.tswebui/src/Buttons/SurfaceView/surfaceGeometry.tswebui/src/Buttons/__tests__/ButtonGridPanel.test.tsxwebui/src/Buttons/__tests__/ButtonsPage.test.tsxwebui/src/Buttons/__tests__/GridButtonDragOverlay.test.tsxwebui/src/Buttons/__tests__/GridButtonIcons.test.tsxwebui/src/Buttons/__tests__/GridButtonPreview.test.tsxwebui/src/Buttons/__tests__/GridViewAs.test.tswebui/src/Buttons/__tests__/GridZoom.test.tsxwebui/src/Buttons/index.tsxwebui/src/Buttons/useGridViewAs.tswebui/src/Components/DropdownChoices.tsxwebui/src/Components/DropdownInputField/Popup.tsxwebui/src/Components/DropdownInputField/useFuzzyChoices.tswebui/src/Components/DropdownInputFieldSimple.tsxwebui/src/Components/__tests__/DropdownInputField.test.tsxwebui/src/Hooks/__tests__/useButtonImageForLocation.test.tsxwebui/src/Hooks/useButtonImageForLocation.tswebui/src/ImportExport/Import/Page.tsx
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
|
I am happy with this now, and think it is ready to merge and update other modules to properly support. @peternewman check out the last 3 screenshots in the description. Thats what I've been aiming for, and will hopefully translate to other surface modules just fine. |
fba3ccd to
a775b9b
Compare
|
This looks great; nice work! |
a775b9b to
79ee3b9
Compare
79ee3b9 to
13352bf
Compare
A choice can say it is disabled, and both the simple and the searchable dropdown then show it without letting it be chosen. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The surface api for declaring the models a plugin supports, describing the appearance of a surface, and saying which declared model a surface is, is not yet in a release. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The grid is larger than any one surface, and a surface is not always a plain rectangle of square buttons. Viewing the grid as a surface - one which has been connected, or a model of one a surface module declares - draws the grid as the face of that surface: each control where it sits and at the shape it is drawn, with the cell coordinates still the grid's own. Controls beyond the configured grid are drawn but cannot be configured, and offer to grow the grid to fit. The choice is made per browser, as a way of looking at the grid rather than part of the config. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A surface can describe its face: where each control sits and its shape, the colour of its body, and artwork for it. That is kept with the surface's config alongside its layout, so it is known while the surface is unplugged, and the view draws it exactly rather than estimating positions from the layout's rows and columns. Artwork is drawn without the body colour under it, so the shape of the device shows, and a control is only rounded when given a radius. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…in ones A fixed few pixels taken off every side of a control was nothing on a key but squashed a thin one, like the Neo's info bar, out of the shape its image is drawn at. Controls are now drawn exactly where the surface has them, as the face already leaves gaps between them. A control thinner than a fingertip is given a hit area grown around it, cut back to the middle of the gap wherever it would meet another's, so even a dense face never has two overlap. One too small to hold its location leaves it out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A style with colours and no bitmap, like the Neo's touch buttons, is lit rather than drawn on. Such a control is drawn as a swatch of the button's background, which the preview stream now carries, rather than its image squeezed into a sliver it was never going to be shown in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A control's style can declare leds, as a ring around an encoder or a strip. The preview stream now carries the gauge a button drives them with, and the view draws them around or along the control, each segment lit with the same sampling the surface is driven with. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The dark ground the grid uses fights with a face of any colour but its own, so a face the surface describes is drawn on the page instead, with a hairline and soft shadow tracing its outline, and scrollbars to match. Press mode keeps the armed ground, as marking out what is live matters more there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The panel's min-height was only ever set by the grid, so a surface face kept whatever the grid had last asked for, and zooming out left the panel far taller than the face. The face now asks for its own: the whole face when it is short, or two rows of keys when it is tall, so it scrolls within the panel rather than stretching the page. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…by default A Neo and a Studio want very different zoom levels, so one shared between them is always wrong for one. Viewing as a surface has a zoom of its own, remembered for its model so a plugged in surface shares it with its model. One not zoomed by hand fits to the panel's width, following it as it changes, within 60-100% so a very wide surface stays readable and a small one is not blown up. The zoom control offers going back to fitting. Steps work from the previous setting, so a burst of them - a fast ctrl+wheel - all count, and the controller only changes with the kind of surface, so the wheel listener holding it is not redone at every step. The grid's own zoom now starts at 100% when storage cannot be read, rather than failing the buttons page where storage is blocked. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A surface can say which of its module's declared models it is. That is kept with its config, like its appearance, so it is still known while the surface is unplugged, and the layout summary qualifies it with the module, as the declared models are. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Matching by the model id a surface reports, before falling back to its name, which a module is free to word differently from the model's. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A display bigger than the others in its row or column, like an info bar under a row of keys, now spills into the empty cells beside it rather than widening its own track, so the keys around it stay evenly spaced. A cell two would both spill into is left to neither. A control with no display fills the cell the displays around it leave, rather than being drawn a fixed square, while one with a ring of leds stays round. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
13352bf to
dbfc79b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
dea920ca-11db-4422-99bd-cd2ee2ff0ed8
⛔ Files ignored due to path filters: 1
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing: 19
companion/lib/Instance/Surface/ChildHandler.tscompanion/lib/Surface/IP/Satellite.tsdocs/user-guide/3_config/buttons/index.mdpackage.jsonshared-lib/lib/ModuleApiVersionCheck.tsshared-lib/lib/SurfaceLayout.tsshared-lib/lib/__tests__/SurfaceLayout.test.tswebui/src/Buttons/ButtonGridPanel.csswebui/src/Buttons/ButtonGridPanel.tsxwebui/src/Buttons/ButtonInfiniteGrid.tsxwebui/src/Buttons/GridViewAsBanner.tsxwebui/src/Buttons/GridViewAsControl.tsxwebui/src/Buttons/SurfaceView/SurfaceCanvas.tsxwebui/src/Buttons/__tests__/ButtonGridPanel.test.tsxwebui/src/Buttons/__tests__/ButtonsPage.test.tsxwebui/src/Buttons/index.tsxwebui/src/Components/DropdownInputFieldSimple.tsxwebui/src/Components/__tests__/DropdownInputField.test.tsxwebui/src/ImportExport/Import/Page.tsx
🚧 Files skipped from review as they are similar to previous changes: 1
- webui/src/Buttons/ButtonGridPanel.css
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
|
|
||
| ## Viewing the grid as a surface | ||
|
|
||
| The grid is larger than any one surface, and a surface is not always a plain rectangle of square |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that a surface can extend beyond the grid.
Line 111 says the grid is larger than every surface, but Lines 122-124 say a surface can reach beyond the grid. Please describe the grid and surface as having different sizes so users do not expect every surface to fit without growing the grid.
This branch has not been deployed
3 participants
Related: #1352
Closes: #2189
closes #3205
Replaces #2945
This is intended to solve 2 workflow problems:
x-xx-xx-xpattern of encoder/lcd alignment.For now this is gated behind a flag in the advanced settings page, so that this can be merged before being complete (to avoid large merge conflicts)
To achieve this, there is a new button+dropdown in the button grid, which allows for choosing a surface to see the grid as. This has 2 modes, you can pick a surface that your companion currently or has previously had connected (once it has been connected recently enough for us to grab and persist the layout info), or any model reported as compatible by an enabled surface module.
The grid below then gets reconfigured to match the selected surface. For modules which aren't providing a new 'surfaceAppearance' definition, we make our best attempt based upon the technical layout of the controls it requests. These are usually messy, but at least filter down the controls and provide the correct resolutions to the controls: (Some CSS polish still needed)
Surfaces can now also provide a full appearance description to drive this drawing properly. For example:
Note that these are not 100% faithful to the original, but they should be accurate enough for any view and alignment a user may want to do. Primarily as they are svgs measured from either photos or manufacturer pngs. Intentionally using svgs here to avoid the bloat of high res pngs which will need to come in over satellite.
Summary by CodeRabbit
Summary by CodeRabbit