Skip to content
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

feat: implement the FileInput API for extra.FileInput #288

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

egormkn
Copy link

@egormkn egormkn commented Nov 19, 2023

Hi!
I noticed that the FileInput from ipyvuetify.extra doesn't support all properties of v-file-input. In this PR I tried to implement most of the FileInput API while maintaining backward compatibility where possible. I suppose it closes #265.

I haven't found a beautiful way to pass v-file-input props to component without hardcoding the property names. I tried to filter keys of this.$data, but that will lead to possible issues with components derived from FileInput, such as solara.FileDrop. So I think this implementation will do for now, and the further task will be to combine it with the generated FileInput.

Please let me know your thoughts about this.

@egormkn
Copy link
Author

egormkn commented Nov 19, 2023

Also I made a reacton component for testing this code based on solara.FileDrop. If this PR gets merged, I can send a PR with something similar to solara repository.

import threading
from typing import Callable, TypedDict, BinaryIO, cast

import reacton
import solara

from ipyvuetify.extra.file_input import FileInput


class FileInfo(TypedDict):
    name: str
    size: int
    lastModified: int
    type: str
    file_obj: BinaryIO
    data: bytes | None


@reacton.component
def Upload(on_upload: Callable[[list[FileInfo]], None] = lambda _: None, lazy: bool = False, **kwargs):
    file_info, set_file_info = reacton.use_state(None)
    wired_files, set_wired_files = reacton.use_state(cast(list[FileInfo] | None, None))

    file_input = FileInput.element(
        outlined=True,
        counter=True,
        multiple=True,
        show_size=True,
        label="Files",
        placeholder="Select files...",
        chips=True,
        color="purple",
        on_v_model=set_file_info,
        disabled=bool(file_info),
        loading=bool(file_info),
        prepend_icon="mdi-upload",
        attributes={"accept": "image/*"},
        **kwargs
    )

    def wire_files():
        if not file_info:
            return

        real = cast(FileInput, reacton.get_widget(file_input))

        # workaround for @observe being cleared
        real.version += 1
        real.reset_stats()

        set_wired_files(cast(list[FileInfo], real.get_files()))

    reacton.use_effect(wire_files, [file_info])

    def handle_file(cancel: threading.Event):
        if not wired_files:
            return
        if on_upload:
            for wired_file in wired_files:
                wired_file["data"] = None if lazy else wired_file["file_obj"].read()
            on_upload(wired_files)
        set_file_info(None)

    result: solara.Result = solara.hooks.use_thread(handle_file, [wired_files])
    if result.error:
        raise result.error

    return file_input


if __name__ == "__main__":

    @reacton.component
    def Page():
        Upload(on_upload=print)

Copy link
Collaborator

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really ❤️ warming to see you spending effort on being backwards compatible!

I really love this PR. I do wonder if the deprecation warnings should be emitted now, as that can be seen as a breaking change? Maybe be silent now, and for ipyvue v3 we do warn the user?

What do you think Mario?

del kwargs["v_model"]

# Maintain backwards compatibility for the file_info
dlink((self, "v_model"), (self, "file_info"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better done on the python side. If we have file_info not have sync=True, and observe v_model, and set file_info as a result of that, we don't have to transfer the data twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already removed sync tag for file_info. It seems that dlink does what you described in this case: it updates the target value once on initialization and adds the observer, which modifies the target value on change to the source value.

@mariobuikhuizen
Copy link
Collaborator

Yeah, this is great! I didn't know you could override a slot by adding it again. I also overlooked there was a slot for progress.

I agree with @maartenbreddels that we should not show warnings for the changed properties. In the notebook these will show up for the end-user. That end-user may not be the author and may not know how to fix it.

Looks like only show_progress is not backward compatible. It would be great if we could make that backward compatible as-well. Then this feature would have no breaking changes.

Note: I will force push to this branch to remove the erroneous commit that was temporarily on master.

@mariobuikhuizen
Copy link
Collaborator

I see there are lint-errors. This can be prevented by using pre-commit. I've just updated the readme with these instructions:

$ pip install -e ".[dev]"
$ pre-commit install

This will check and enforce code style when committing.

To update the existing code you can run:

$ pre-commit run --all-files

@egormkn
Copy link
Author

egormkn commented Nov 25, 2023

I've removed warnings and fixed code style issues.
Also, I returned the sync tag to file_info and used get_files() to set v_model value on file_info change. Now it is possible to remove the effect from the component above and use just on_v_model=set_wired_files.

I have a question: why this workaround is needed for solara.FileDrop? How observe can be cleared?

real.version += 1
real.reset_stats()

UPD: show_progress is still not backward compatible because it was True by default, and I'm not sure about doing the same for loading.

@egormkn
Copy link
Author

egormkn commented Nov 26, 2023

I added a computed property as a default loading state. If neither loading nor show_progress are set, the progress bar will appear automatically, but it is also easy to control the state manually.

I also thought about transforming v_model values into objects, that use pythonic key names (last_modified instead of lastModified) and maybe even allow attribute access with dot notation (namedtuple?). It should be easier to do now, before this PR is merged. What do you think?

UPD: I've almost finished adding drag & drop 😃

@mariobuikhuizen
Copy link
Collaborator

Sorry for the late response, I missed the notifications for the changes on this PR.

I added a computed property as a default loading state. If neither loading nor show_progress are set, the progress bar will appear automatically, but it is also easy to control the state manually.

Wouldn't it be better to leave out loading from the API, since it's controlled by the component internally?

The drag & drop functionality is really nice!, but it shouldn't be part of this PR. Are you able to move those commits to a new PR (that can be based on this PR)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

extra FileInput **kwargs have no effect
3 participants