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: Support for dropping extra callback args #271

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

jnumainville
Copy link
Collaborator

@jnumainville jnumainville commented Feb 8, 2024

Fixes #260

This snippet now works:

import deephaven.ui as ui
from deephaven.ui import use_state


@ui.component
def counter():
    count, set_count = use_state(0)
    return [
      # Pass in a function that accepts the event argument
      ui.action_button(f"Foo {count}", on_press=lambda e: set_count(count + 1)),
      # Pass in a function that does not have any positional arguments
      ui.action_button(f"Bar {count}", on_press=lambda: set_count(count + 1))
    ]


c = counter()

This implementation is more robust than that though. It supports:

  1. Dropping any number of positional args. For example, if the callback supports 3 positional args but only two are defined, the third will be not be passed in.
  2. Dropping any unused keyword args.
  3. If *args or **kwargs are used, all of the relevant args are passed in.

@jnumainville jnumainville changed the title feat: upport feat: Support for dropping callback args Feb 8, 2024
@jnumainville jnumainville requested a review from mofojed February 8, 2024 17:03
@jnumainville jnumainville changed the title feat: Support for dropping callback args feat: Support for dropping extra callback args Feb 8, 2024
@jnumainville jnumainville self-assigned this Feb 8, 2024
@jnumainville jnumainville marked this pull request as ready for review February 8, 2024 17:16
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Sweet. As a side note, the kwargs was unnecessary - JS will only be allowed to call positional arguments anyways. But it does make the solution more general/complete so I like that.

plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
@jnumainville jnumainville requested a review from mofojed February 8, 2024 19:33
@jnumainville jnumainville merged commit 34ddfcd into deephaven:main Feb 8, 2024
11 checks passed
mofojed added a commit to mofojed/deephaven-plugins that referenced this pull request Feb 22, 2024
- We are wrapping callables now because of deephaven#271, but did not account for `signature` raising an error: https://docs.python.org/3/library/inspect.html
- Instead of not working at all, just return the original Callable. They'll need to have provided the correct number of arguments, which isn't the worst thing in the world
- Tested with the `print` button example:
```
import deephaven.ui as ui

@ui.component
def button_event_printer(*children, id="My Button"):
    return ui.action_button(
        *children,
        on_key_down=print,
        on_key_up=print,
        on_press=print,
        on_press_start=print,
        on_press_end=print,
        on_press_up=print,
        on_focus=print,
        on_blur=print,
    )

@ui.component
def button_events():
    return ui.panel([
        button_event_printer("1", id="My Button 1"),
        button_event_printer("2", id="My Button 2"),
    ])

be = button_events()
```
mofojed added a commit that referenced this pull request Feb 22, 2024
- We are wrapping callables now because of #271, but did not account for
`signature` raising an error:
https://docs.python.org/3/library/inspect.html
- Instead of not working at all, just return the original Callable.
They'll need to have provided the correct number of arguments, which
isn't the worst thing in the world
- Tested with the `print` button example:
```
import deephaven.ui as ui

@ui.component
def button_event_printer(*children, id="My Button"):
    return ui.action_button(
        *children,
        on_key_down=print,
        on_key_up=print,
        on_press=print,
        on_press_start=print,
        on_press_end=print,
        on_press_up=print,
        on_focus=print,
        on_blur=print,
    )

@ui.component
def button_events():
    return ui.panel([
        button_event_printer("1", id="My Button 1"),
        button_event_printer("2", id="My Button 2"),
    ])

be = button_events()
```
jnumainville added a commit that referenced this pull request Mar 13, 2024
Adds spec for `list_view`

There is one major additions on top of what has been discussed in the
picker spec.

I see it as an oversight (or at least not ideal) to some extent that
there's no simple way for embedded buttons to access their parent's keys
easily, as far as I can tell. I suppose it's simple enough to bind item
keys to button events if you're doing this in React, but I think we'll
want a way to do that easily, especially with tables, because it doesn't
make sense to pass a button for each item in a ticking table. The least
intrusive method here is returning extra args for the item key and
section key that a button is embedded in that can be ignored (using the
changes implemented in #271).
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.

Allow functions that don't accept parameters as event callbacks
2 participants