Fixes `.change()` in `Video`, `Audio`, `Image` #4793

abidlabs · 2023-07-05T15:02:22Z

We had multiple separate issues:

The change event would get triggered twice when a file would be uploaded to Video or Image

Test with:

import gradio as gr

def test(x):
    print("change called")
    return x

with gr.Blocks() as demo:
    a = gr.Video()
    b = gr.Video()
    
    a.change(test, a, b)
    
demo.launch()

And:

import gradio as gr

def test(x):
    print("change called")
    return x

with gr.Blocks() as demo:
    a = gr.Image()
    b = gr.Image()
    
    a.change(test, a, b)
    
demo.launch()

or

import gradio as gr

def testa():
    print("a change called")
    return 

def testb():
    print("b change called")
    return 


with gr.Blocks() as demo:
    a = gr.Image(source="canvas", tool="color-sketch")  # try different tools here
    b = gr.Image()
    c = gr.Image()
    
    a.change(lambda x:x, a, b)
    a.change(testa, None, None)
    b.change(lambda x:x, b, c)
    b.change(testb, None, None)
    
demo.launch()

The change event would not get triggered for Audio if its value was changed programmatically

Try:

import gradio as gr

with gr.Blocks() as demo:
    a = gr.Audio()
    b = gr.Audio()
    c = gr.Audio()
    
    a.change(lambda x:x, a, b)
    a.change(lambda x:print("a change_called"), a, None)
    b.change(lambda x:x, b, c)
    b.change(lambda x:print("b change_called"), b, None)
    
demo.launch()

This PR fixes these issues above

Closes: #4378, Closes: #4589

Adds unit tests and updates a demo (change_vs_input) that can be used to test the changes as well

vercel · 2023-07-05T15:02:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
gradio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 10, 2023 8:47pm

gradio-pr-bot · 2023-07-05T15:05:45Z

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4793-all-demos

abidlabs · 2023-07-05T15:06:02Z

js/app/src/components/Video/Video.svelte

@@ -57,8 +57,6 @@
 		} else {
 			value = null;
 		}
-
-		dispatch("change");


Not needed because we already dispatch change if the value changes in the subsequent lines

gradio-pr-bot · 2023-07-05T15:30:37Z

Chromatic build successful 🎉

gradio-pr-bot · 2023-07-05T21:16:05Z

🎉 The demo notebooks match the run.py files! 🎉

pngwn · 2023-07-10T09:05:48Z

CHANGELOG.md

+* The `.change()` event is fixed in `Video`, `Image`, `Audio` by [@abidlabs](https://github.com/abidlabs) in [PR 4793](https://github.com/gradio-app/gradio/pull/4793)



Can we add a more descriptive changelog entry here explaining exactly what was fixed?

pngwn · 2023-07-10T09:06:56Z

js/app/test/change_vs_input.spec.ts

I think these should be unit tests as events can easily be tested without spinning up a browser.

Makes sense, I'll look into it!

abidlabs · 2023-07-10T19:20:27Z

Thanks @pngwn for the suggestion. I've replaced the e2e test with unit tests for each of the affected components, following the approach in #4811.

Ready for a re-review!

abidlabs · 2023-07-10T19:26:39Z

Hmmm CI is complaining about this error:

⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯
ReferenceError: CustomEvent is not defined
 ❯ Module.custom_event node_modules/.pnpm/svelte@4.0.0/node_modules/svelte/src/runtime/internal/dom.js:1072:2
    1070|   * @returns {void}
    1071|   */
    1072|  c(html) {
       |  ^
    1073|   this.h(html);
    1074|  }
 ❯ node_modules/.pnpm/svelte@4.0.0/node_modules/svelte/src/runtime/internal/lifecycle.js:111:40
 ❯ dispatch_blob js/audio/src/Audio.svelte:919:3
 ❯ js/audio/src/Audio.svelte:972:5
 ❯ Object.<anonymous> js/app/src/components/Audio/Audio.test.ts:135:11

This error originated in "js/app/src/components/Audio/Audio.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
This error was caught after test environment was torn down. Make sure to cancel any running tasks before test finishes:

But I don't get this error when running locally.

Edit: its no longer complaining about it. Not sure what could be responsible for the flake.

freddyaboulton

This looks good to me @abidlabs ! Gonna let @hannahblair @pngwn @dawoodkhan82 give the final approval.

freddyaboulton · 2023-07-11T14:35:45Z

js/app/src/components/Audio/Audio.svelte

@@ -41,6 +44,13 @@
 	let _value: null | FileData;
 	$: _value = normalise_file(value, root, root_url);

+	$: {


I think we're trying to move away from JSON.stringify. I think the Label component has the "preferred" way but gonna defer to @pngwn

Ah good to know! Will take a look later

freddyaboulton · 2023-07-11T14:38:40Z

js/app/src/components/Video/Video.test.ts

@@ -125,7 +125,7 @@ describe("Video", () => {
 			root_url: null,
 			streaming: false,
 			pending: false,
-			source: "microphone",


Why do we need to change the source?

Because Video doesn't have a "microphone" source :)

pngwn

Looks good to me! Thanks for fixing this @abidlabs!

video change

959e959

abidlabs commented Jul 5, 2023

View reviewed changes

abidlabs changed the title ~~Fixes .change() in Video and Audio~~ Fixes .change() in Video and Audio and Image Jul 5, 2023

abidlabs changed the title ~~Fixes .change() in Video and Audio and Image~~ Fixes .change() in Video, Audio, Image, File Jul 5, 2023

changes

f40a6b9

vercel bot deployed to Preview July 5, 2023 19:07 View deployment

image

50789b1

vercel bot deployed to Preview July 5, 2023 20:40 View deployment

fix image

6d1af3b

vercel bot deployed to Preview July 5, 2023 20:48 View deployment

abidlabs changed the title ~~Fixes .change() in Video, Audio, Image, File~~ Fixes .change() in Video, Audio, Image Jul 5, 2023

test

368a0cd

vercel bot deployed to Preview July 5, 2023 21:16 View deployment

tests

d4ddb6a

vercel bot deployed to Preview July 7, 2023 15:27 View deployment

Merge branch 'main' into audio-change

baf2113

vercel bot deployed to Preview July 7, 2023 15:29 View deployment

Merge branch 'main' into audio-change

2ad9c57

vercel bot deployed to Preview July 7, 2023 16:39 View deployment

changelog, demos

bce629a

vercel bot deployed to Preview July 7, 2023 16:40 View deployment

fixed name

d9017e9

vercel bot deployed to Preview July 7, 2023 16:44 View deployment

abidlabs added 2 commits July 7, 2023 12:41

change vs input

c6d1a9c

Merge branch 'main' into audio-change

8938d28

vercel bot deployed to Preview July 7, 2023 17:46 View deployment

abidlabs marked this pull request as ready for review July 7, 2023 20:57

abidlabs requested review from aliabid94, dawoodkhan82, pngwn and freddyaboulton July 7, 2023 23:53

pngwn reviewed Jul 10, 2023

View reviewed changes

Merge branch 'main' into audio-change

9222984

vercel bot deployed to Preview July 10, 2023 18:02 View deployment

changelog

59ccbe9

vercel bot deployed to Preview July 10, 2023 18:18 View deployment

video unit test

1522f98

vercel bot deployed to Preview July 10, 2023 18:56 View deployment

remove e2e test

da182da

vercel bot deployed to Preview July 10, 2023 19:09 View deployment

abidlabs added 2 commits July 10, 2023 14:09

revert website

928797c

revert website

dcc5a17

vercel bot deployed to Preview July 10, 2023 19:11 View deployment

image unit test

a63bf6e

vercel bot deployed to Preview July 10, 2023 19:17 View deployment

formatting

1876a4e

vercel bot deployed to Preview July 10, 2023 19:18 View deployment

Merge branch 'main' into audio-change

74cc008

vercel bot deployed to Preview July 10, 2023 20:47 View deployment

freddyaboulton requested a review from pngwn July 11, 2023 14:32

freddyaboulton reviewed Jul 11, 2023

View reviewed changes

pngwn approved these changes Jul 11, 2023

View reviewed changes

pngwn merged commit abd0ced into main Jul 11, 2023
11 checks passed

pngwn deleted the audio-change branch July 11, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes `.change()` in `Video`, `Audio`, `Image` #4793

Fixes `.change()` in `Video`, `Audio`, `Image` #4793

abidlabs commented Jul 5, 2023 •

edited

vercel bot commented Jul 5, 2023 •

edited

gradio-pr-bot commented Jul 5, 2023

abidlabs Jul 5, 2023

gradio-pr-bot commented Jul 5, 2023 •

edited

gradio-pr-bot commented Jul 5, 2023 •

edited

pngwn Jul 10, 2023

pngwn Jul 10, 2023

abidlabs Jul 10, 2023

abidlabs commented Jul 10, 2023

abidlabs commented Jul 10, 2023 •

edited

freddyaboulton left a comment

freddyaboulton Jul 11, 2023

abidlabs Jul 11, 2023

freddyaboulton Jul 11, 2023

abidlabs Jul 11, 2023

freddyaboulton Jul 11, 2023

pngwn left a comment

		* The `.change()` event is fixed in `Video`, `Image`, `Audio` by [@abidlabs](https://github.com/abidlabs) in [PR 4793](https://github.com/gradio-app/gradio/pull/4793)

Fixes .change() in Video, Audio, Image #4793

Fixes .change() in Video, Audio, Image #4793

Conversation

abidlabs commented Jul 5, 2023 • edited

vercel bot commented Jul 5, 2023 • edited

gradio-pr-bot commented Jul 5, 2023

Choose a reason for hiding this comment

gradio-pr-bot commented Jul 5, 2023 • edited

gradio-pr-bot commented Jul 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs commented Jul 10, 2023

abidlabs commented Jul 10, 2023 • edited

freddyaboulton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pngwn left a comment

Choose a reason for hiding this comment

Fixes `.change()` in `Video`, `Audio`, `Image` #4793

Fixes `.change()` in `Video`, `Audio`, `Image` #4793

abidlabs commented Jul 5, 2023 •

edited

vercel bot commented Jul 5, 2023 •

edited

gradio-pr-bot commented Jul 5, 2023 •

edited

gradio-pr-bot commented Jul 5, 2023 •

edited

abidlabs commented Jul 10, 2023 •

edited