Fix Upload widget counter value

[jtpio]

Fixes #2480.

It looks like the _counter value was used to detect a change of the value.

Instead we could use data to achieve the same result, since its value should change when new files are being uploaded.

Breaking changes

• Rename data to _data
• Rename metadata to _metadata
• Make _counter, _metadata and _data read-only
• value is a list of dicts. Example:

[{ 'metadata': {}, 'content': <memoryview>}]

[pbugnion]

Thanks for this.

I agree that it's a little strange that we're using _counter both to decide whether there's been an upload event and to update the description.

I'm not sure I agree that switching to observing the data trait solves our problems though. For instance, what happens if someone uploads:

1. A.txt with content hello
2. B.txt with content hello

I don't think data changes in this case? (I haven't checked this, so I may well be wrong)

An alternative could be to keep _counter, but not use it in the button label? We could have a new model property (that doesn't necessarily make its way all the way to the Python layer) called fileCount that measures the current number of files?

[jtpio]

I don't think data changes in this case? (I haven't checked this, so I may well be wrong)

Thanks for raising that point. We should be able to check that behavior.

It looks it's possible for data to not change, but for metadata to change. In that case value would indeed have to be recalculated.

An alternative could be to keep _counter, but not use it in the button label? We could have a new model property (that doesn't necessarily make its way all the way to the Python layer) called fileCount that measures the current number of files?

Right, with the current change _counter is only used in the button label and doesn't need to live in the kernel. We would still need a way to detect a state change to be able to set value accordingly.

Another way could be to compute value on the frontend, and push it back to the kernel? So the _data_changed method would not be needed anymore.

[pbugnion]

It looks it's possible for data to not change, but for metadata to change. In that case value would indeed have to be recalculated.

Thanks for checking.

Right, with the current change _counter is only used in the button label and doesn't need to live in the kernel.

Agreed.

Another way could be to compute value on the frontend, and push it back to the kernel? So the _data_changed method would not be needed anymore.

To me, that sounds like the right call, except data and metadata are annoyingly exposed as public traits in the Python-side. Somewhat weirdly, data and metadata are not read-only. I'm not sure what would happen if someone sets them server-side.

We could break backwards compatibility and just keep value as a read-only trait?

[jtpio]

We could break backwards compatibility and just keep value as a read-only trait?

It's possible that data and metadata were kept that way to leverage the binary serialization for data, so that the value dict can be computed on the Python side.

[pbugnion]

It's possible that data and metadata were kept that way to leverage the binary serialization for data

Ah yeah, that makes sense. They should probably still be read-only.

[jtpio]

Pushed some changes to:

• make data and metadata private and read only
• use a client side _file_count for the Upload button view
• keep _counter as a way to trigger the value update

[pbugnion]

I think this looks good. I think it'd be useful to discuss with people explicitly removing data and metadata from the public API.

[jtpio]

Thanks @pbugnion for adding the comment in 656e2ed.

For reference and to add a bit more context: without this serializer, we indeed hit the default serializer, which performs a deep copy of the value via a JSON.parse / JSON.stringify:

ipywidgets/packages/base/src/widget.ts

Line 443 in 44fb6f8

state[k] = JSON.parse(JSON.stringify(state[k]));

This would result in the ArrayBuffer (value of content) being serialized to an empty object ({} ).