feat: add signal bind methods to grid component#8742
Conversation
|
|
||
| boolean[] fromSignal = { false }; | ||
|
|
||
| Registration effectReg = ElementEffect.bind(getElement(), |
There was a problem hiding this comment.
How is this cleaned up when you switch to a different selection model?
There was a problem hiding this comment.
Good catch, it wasn't. I've added a removeBinding(VALUE) call in the remove() method of both selection models, so the signal binding (effect + listener) is properly cleaned up when the selection model is replaced via setSelectionMode(). Added also tests to verify that after switching modes, the old signal no longer affects the grid and a new binding can be created.
| SignalBindingFeature feature = getElement().getNode() | ||
| .getFeature(SignalBindingFeature.class); | ||
|
|
||
| if (feature.hasBinding(SignalBindingFeature.VALUE)) { |
There was a problem hiding this comment.
It seems wrong to bind the "value" of a Grid element to the selection?
It also feels like there is some missing infrastructure here if implementing a single bind method in a component requires 40 lines of code.
There was a problem hiding this comment.
It seems wrong to bind the "value" of a Grid element to the selection?
It's the standard key used by HasValue.bindValue implementations, and from the asSingleSelect()/asMultiSelect() perspective the selection is the grid's value. The key is just an identifier in SignalBindingFeature to track the binding.
It also feels like there is some missing infrastructure here if implementing a single bind method in a component requires 40 lines of code.
I've extracted the shared logic into a package-private GridSelectionSignalHelper.bindValue() that both selection models delegate to, which also fixes the SonarQube duplication warning. The implementation follow what's done in AbstractFieldSupport, but yes, a default re-usable framework implementation would be better.
…lication Address review comments: add signal binding cleanup in selection model remove() so bindings are properly released when switching selection modes, and extract shared bindValue logic into GridSelectionSignalHelper to eliminate code duplication between single and multi selection models. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add getSelectedItemSignal() and getSelectedItemsSignal() convenience methods that create a ValueSignal internally, bind it to the selection, and return it — so users can directly get a signal representing the grid's selection state without managing their own signal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Also added |
I'm not sure that's a pattern we want to introduce without considering how it relates to similar cases with the value in e.g. a text field. I did some initial exploration of that concept a while ago and I ended up rejecting it because it became weird. Unfortunately, I don't remember exactly what the problem was. |
| protected void remove() { | ||
| super.remove(); | ||
| getGrid().getElement().getNode().getFeature(SignalBindingFeature.class) | ||
| .removeBinding(SignalBindingFeature.VALUE); |
There was a problem hiding this comment.
multiSelect_bindValue_switchToSingle_bindingCleanedUp still passes when I remove this.
There was a problem hiding this comment.
Right, tests pass trivially because after mode switch, the old signal writes to the disconnected old model, which never affects the new model regardless of cleanup. Tests now switch back to the original mode and verify the old effect doesn't fire selection events.
| protected void remove() { | ||
| super.remove(); | ||
| getGrid().getElement().getNode().getFeature(SignalBindingFeature.class) | ||
| .removeBinding(SignalBindingFeature.VALUE); |
There was a problem hiding this comment.
singleSelect_bindValue_switchToMulti_bindingCleanedUp still passes when I remove this.
| throw new BindingActiveException(); | ||
| } | ||
|
|
||
| boolean[] fromSignal = { false }; |
There was a problem hiding this comment.
Tests still pass if I remove this.
There was a problem hiding this comment.
Correct, the signal's own equality check prevented an actual infinite loop, so existing tests didn't cover the guard. Added now tests that explicitly count calls.
| * @see #setColumns(String...) | ||
| * @since 25.1 | ||
| */ | ||
| public void bindColumns(Signal<List<String>> signal) { |
There was a problem hiding this comment.
I can still use all of these (and many more) to get the signal state out of sync with the grid:
addColumnaddColumnsremoveColumnremoveAllColumns
Same for the selection, header and footer text.
What is the plan here? Should a component really prevent any imperative API from working if a binding is active? Is it a user error and we ignore? Is setColumns not working just accidental due to using SignalPropertySupport?
There was a problem hiding this comment.
As discussed in Slack, I'm going to revert the bindColumns method for now: once a signal is bound, that's the source of truth and imperative methods shouldn't work.
There was a problem hiding this comment.
There's more:
column.setHeader(new Span("foo"));
grid.getHeaderRows().get(0).getCell(column).setText("foo");
grid.getHeaderRows().get(0).getCell(column).setComponent(new Span("foo"));
column.setFooter(new Span("foo"));
grid.getFooterRows().get(0).getCell(column).setText("foo");
grid.getFooterRows().get(0).getCell(column).setComponent(new Span("foo"));To be honest this all feels a bit rushed. I'd be in favor of dropping everything except the selection maybe.
The bindColumns/setColumns signal binding is inconsistent with how other column methods (addColumn, removeColumn, etc.) work, as they bypass the binding guard entirely and silently desync the signal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The existing _bindingCleanedUp tests passed trivially because after a single mode switch the old signal writes to the disconnected old model, which never affects the new model regardless of cleanup. Strengthen by switching back to the same mode and verifying the old effect does not fire spurious selection events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that the fromSignal guard in GridSelectionSignalHelper prevents the write callback from being invoked when the value change originates from the signal itself, while still allowing it for client-originated changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
4 participants
No description provided.