-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix value synchronisation issue in gr.Dataframe #10918
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-pypi-previews.s3.amazonaws.com/29573e2d179daef030326a140afabf49899bf83b/gradio-5.23.3-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@29573e2d179daef030326a140afabf49899bf83b#subdirectory=client/python" Install Gradio JS Client from this PR npm install https://gradio-npm-previews.s3.amazonaws.com/29573e2d179daef030326a140afabf49899bf83b/gradio-client-1.14.1.tgz Use Lite from this PR <script type="module" src="https://gradio-lite-previews.s3.amazonaws.com/29573e2d179daef030326a140afabf49899bf83b/dist/lite.js""></script> |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
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.
Pull Request Overview
This pull request addresses a value synchronization issue in gr.Dataframe by removing redundant logic that may be causing input inconsistencies.
- Removed the clear_on_focus property from the selection context and its associated assignments.
- Updated the changeset to document the minor fix.
Reviewed Changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
js/dataframe/shared/context/selection_context.ts | Removed the clear_on_focus property and its assignment to prevent redundant value clearing behavior. |
.changeset/wet-rockets-bow.md | Updated the changeset to reflect a minor fix in gr.Dataframe value synchronization. |
Files not reviewed (3)
- js/dataframe/shared/EditableCell.svelte: Language not supported
- js/dataframe/shared/Table.svelte: Language not supported
- js/dataframe/shared/TableCell.svelte: Language not supported
I tested with Screen.Recording.2025-04-01.at.7.15.35.AM.mov |
@abidlabs hmm good catch |
Translation: I'm also encountering this issue. How can I apply the patched version of Gradio to start using it right away? |
Not fixed yet @JPlin but if you have a repro we can test with and/or more details about your system and when you experience this issue, that'll be helpful |
@JPlin I have a fix running locally - just doing some testing and will get it out as soon as possible! |
- fix cell value saving logic
- add story and tests
So (apologies), this turned out to be a bit of a refactor, just because I found a few opportunities for cleanup in the process of fixing a couple issues. I have consolidated the three context files into one because in hindsight it just means a bit more cognitive overhead separating them. That is really the bulk of the changes (plus some redundant code removal), nothing major has changed here. |
value = currentTarget.value; | ||
dispatch("blur"); | ||
function handle_blur(event: FocusEvent): void { | ||
dispatch("blur", { |
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.
dispatch blur event with coords
of the cell to update the value of that cell
if (edit) { | ||
value = _value; | ||
dispatch("blur"); | ||
} else if (!header) { |
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.
don't need this logic, blur is handled in handle_keydown
@@ -0,0 +1,551 @@ | |||
import { getContext, setContext } from "svelte"; |
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.
new consolidated dataframe context file, nothing new here
@@ -88,28 +88,6 @@ export function handle_delete_key( | |||
return new_data; | |||
} | |||
|
|||
export function handle_editing_state( |
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.
wasnt used
@@ -1,40 +1,82 @@ | |||
import { dequal } from "dequal/lite"; |
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.
all changes here are just using new context file
@@ -1,5 +1,9 @@ | |||
import { describe, test, expect } from "vitest"; |
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.
moved to a test dir
@@ -21,7 +21,6 @@ | |||
right: string; |
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.
removing clear_on_focus
and adding blur event
@@ -454,98 +468,34 @@ | |||
selected = result.selected; | |||
} | |||
|
|||
$: sort_data(data, display_value, styling); |
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.
redundant
@@ -738,6 +687,23 @@ | |||
$: handle_mouse_down = drag_handlers?.handle_mouse_down || (() => {}); | |||
$: handle_mouse_move = drag_handlers?.handle_mouse_move || (() => {}); | |||
$: handle_mouse_up = drag_handlers?.handle_mouse_up || (() => {}); | |||
|
|||
function get_cell_display_value(row: number, col: number): string { |
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.
fixes display_value issue
@@ -60,6 +65,7 @@ | |||
export let handle_select_column: (col: number) => void; | |||
export let handle_select_row: (row: number) => void; | |||
export let is_dragging: boolean; | |||
export let display_value: string | undefined; |
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.
use display_value
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.
Tested and works! Great PR @hannahblair
Description
clear_on_focus
since that was never being usedsave_cell_value
function to the contexthandle_blur
functiondisplay_values
display_values
Closes: #10937