-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate muti-cell paste event to Dash #261
Conversation
…only the latest event to instead include all events emitted within X milliseconds (currently hardcoded to 1)
Have you tested out what this does with row editing and changing all the values? |
@BSd3v what do you mean by "all the values"? I have tried changing multiple values by (1) deleting multiple values and (2) pasting multiple values into the grid. In both cases you get an entry in the list per cell. Are there other ways to change multiple cell values at once? |
Yes, with fullRowEditing. There is a test that uses this functionality, it adds a class to the cell based upon if it was edited. Increase it to editing more than one column before moving on and see if this catches the event properly. |
@BSd3v do you have a link to a test/example that demonstrates/uses this functionality (in Dash)? |
…react-18 compatibility issue)
Here it is: tests/test_row_data_sync.py I thought it was associated with an editing test. But it's just rows data syncing. Just need to see if your fixes capture multiples from this event if more cells are edited before exiting the row editing. |
@BSd3v thanks! For |
Even when you have edited multiple? |
@BSd3v yes. The event 'bundling' mechanism is time-based, so it shouldn't matter that you are in this particular mode. |
Here, run this, I get three when I edit 3 cells and then move down to the next row:
This is what I was wondering about. Here it is when I just pass the stringified object back to the output: |
I'm merging into v31 bump, its possible that this is able to handle correctly in that version with React 18. |
@BSd3v I dont really understand what the issue is with React 18 - but if it's solved, that would be great 😃 |
@emilhe, me neither. You can run app's in React 18 like this:
|
Looks like in React18, the state isnt even updated when it triggers the second time |
@BSd3v Thanks! I'll try that out. Do you know how to clear multiple cells at once from the test (like when you select using click + shift and click delete)? |
You can only do it with range selection, you cant do it without enterprise. 😛 |
Eh, you can do this:
To run in v18. I have a version working now, it just updates the variable instead of the state. |
…Changed # Conflicts: # src/lib/fragments/AgGrid.react.js
Thanks for the update! I have adjusted/cleanup up the code a bit. I also think that I understand why the initial version had problems with React 18; version 18 introduces some performance optimizations, which may introduce race conditions, which could explain the behavior we saw. |
I was thinking that was along the lines of it, haha. Much more performant means things that were in order could not be, haha. |
@BSd3v about the |
CHANGELOG.md
Outdated
### Fixed | ||
- [#262](https://github.com/plotly/dash-ag-grid/issues/262) Fixed all-but-one `cellValueChanged` events suppressed for multi-cell edits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than ### Fixed
this should be a ### Changed
with a very prominent note that it's a breaking change. So it's good that we're about to release a new major version for AG Grid v31 or this kind of change would not be allowed 😅 But it makes sense, I don't see any non-breaking way to support these events.
@BSd3v this makes me wonder, are there any other props that might have similar problems? If so now is the time to convert them into arrays too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If backwards compatibility is a main concern, we could
- Set the type of
cellValueChanged
to union of single/list of events - Add a new prop that represents
CELL_VALUE_CHANGED_DEBOUNCE_MS
, which defaults to -1, in which case we fallback to the old behavior, thus returning a single element. Setting the value it to 0 (or higher) will enable the new behavior, thus returning a list
My initial implementation was actually along these lines, but @BSd3v suggested to target a simpler implementation (the one in this PR) in favor of backwards compatibility since a major release is in the making.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree the simpler API is more important than backward compatibility, given the upcoming major release and the fact it would be annoying to use the prop if sometimes it’s a list and sometimes not. Just make sure the changelog makes the breaking change clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I haven't found anything that operates this way.
The team has also been working on the v31 updates for the docs, and those types of things haven't popped up that I am aware of.
The one thing I don't know about is the grid's state (new feature of the grid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comment, indicating that this is a change. Should I note explicitly that the change is breaking (I am not sure how breaking changes are documented in this project - I didn't see any mentioned of breaking changes previously in the changelog)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the first since switching to version 2 from 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing really to add, just those two things.
CHANGELOG.md
Outdated
### Fixed | ||
- [#262](https://github.com/plotly/dash-ag-grid/issues/262) Fixed all-but-one `cellValueChanged` events suppressed for multi-cell edits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I haven't found anything that operates this way.
The team has also been working on the v31 updates for the docs, and those types of things haven't popped up that I am aware of.
The one thing I don't know about is the grid's state (new feature of the grid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 after the slight modification to the changelog
Co-authored-by: BSd3v <82055130+BSd3v@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃
Will two or more edits not originating from paste events, multi delete or fullRowEditing (or similar) realistically ever be batched? |
What other cases are you thinking about? |
I wonder if there's cases where I can be sure the list will only contain one change, since the logic is simpler to implement (or rather - I have existing logic that is a bit hairy to refactor). In my case don't think it's possible to paste content for instance. Will a user be able to trigger grouped changes by changing multiple cells manually fast? |
No, the user will not be able to trigger the events that quickly. The debounce is set at 1 ms. Maybe a script could do it, but then you have other issues. ;) This is really only helpful for things like paste and fullRowEditing. |
Currently, the
cellValueChanged
events are propagated to Dash one-by-one. This becomes a problem in cases, where events are fires faster than Dash can handle them, e.g. when pasting multiple cells into the grid. For these cases, only the first event triggers a Dash callback, with all other changes lost.This PR changes the event collection mechanism to include all events fired within a specific time window (with the length currently defined by
CELL_VALUE_CHANGED_DEBOUNCE_MS = 1
). By making the window large enough that all events in e.g. a multi cell paste operation can be processed (initial tests indicate that 1 ms is sufficient), all changes can be propagted to Dash. Hence the callback will now receieve a list of change events rather than a single change event.Here is a small example that demonstrates the usage,
Copy-pasting (the whole) row 0 to row 4 will yield the following output with the main branch,
With the updates propose in the PR, the output will be,
Currently, the time window size is hardcoded, but it could also be changed to be a property of the grid so that users get more fine-grained control. Please let me know, if this approach is preferred or not.