-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add Kino.Screen #489
Add Kino.Screen #489
Conversation
Co-authored-by: Hugo Baraúna <[email protected]>
lib/kino/screen.ex
Outdated
end | ||
|
||
def new(module, state) when is_atom(module) do | ||
{:ok, state} = module.init(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.
@jonatanklosko, this may be a bit confusing.
- We call
module.init/1
in the caller process, so we don't fall into the trap of starting a frame inside a process - The first
render/1
happens in the watcher process - All following render happens in the user-specific process
We are exposing three processes to users. Given when module.init/1
is called today, I am thinking we should just get rid of it and you pass the state as argument to Kino.Screen.new directly. We may add it back in the future to customize the user specific process instead.
@josevalim as we talked, I like this direction. I built this dynamic form based on this PR: CleanShot.2025-02-11.at.17.10.19.mp4Here's the source code: defmodule MyForm do
@behaviour Kino.Screen
import Kino.{Control, Input, Layout, Screen}
# Constants
@country_options [empty: "", usa: "USA", can: "Canada"]
@language_options %{
usa: [empty: "", en: "English"],
can: [empty: "", en: "English", fr: "French"]
}
@impl true
def render(state), do: build_form(state)
# Form Building
defp build_form(%{values: values} = state) do
fields = form_fields(values)
form(fields, report_changes: true, submit: "Submit")
|> control(&handle_event/2)
|> add_layout(state)
end
defp form_fields(form_values) when map_size(form_values) == 0 do
[country: country_select()]
end
defp form_fields(%{country: selected_country, language: selected_language}) do
[
country: country_select(selected_country),
language: language_select(selected_country, selected_language)
]
end
defp form_fields(%{country: selected_country}) do
[
country: country_select(selected_country),
language: language_select(selected_country)
]
end
# Form Controls
defp country_select(selected \\ :empty) do
select("Country", @country_options, default: selected)
end
defp language_select(country, selected \\ :empty) do
language_options = Map.get(@language_options, country)
select("Language", language_options, default: selected)
end
# Event Handling
defp handle_event(%{data: %{country: :empty}, type: :change}, state) do
Kino.Frame.append(state.debug_frame, "[event] country selected: empty")
Kino.Frame.clear(state.frame)
%{state | values: %{}}
end
defp handle_event(
%{data: %{country: _country, language: :empty}, type: :change} = event,
state
) do
Kino.Frame.append(state.debug_frame, "[event] language selected: empty")
Kino.Frame.clear(state.frame)
%{state | values: event.data}
end
defp handle_event(
%{data: %{country: country, language: language}, type: :change} = event,
state
) do
language_options = Map.get(@language_options, country)
language_text = Keyword.get(language_options, language)
Kino.Frame.append(state.debug_frame, "[event] language selected #{language_text}")
%{state | values: event.data}
end
defp handle_event(%{data: %{country: country}, type: :change} = event, state) do
country_text = Keyword.get(@country_options, country)
Kino.Frame.append(state.debug_frame, "[event] country selected: #{country_text}")
%{state | values: event.data}
end
defp handle_event(
%{data: %{country: country, language: language}, type: :submit} = event,
state
) do
render_submission(state.frame, country, language)
%{state | values: event.data}
end
# Helpers
defp add_layout(element, state) do
grid([element, state.frame, state.debug_frame])
end
defp render_submission(frame, country, language) do
country_text = Keyword.get(@country_options, country)
language_options = Map.get(@language_options, country)
language_text = Keyword.get(language_options, language)
Kino.Frame.render(
frame,
Kino.Markdown.new("""
Form submission:
- Country: #{country_text}
- Language: #{language_text}
""")
)
end
end
init_state = %{
frame: Kino.Frame.new(placeholder: false),
debug_frame: Kino.Frame.new(placeholder: false),
values: %{}
}
Kino.Screen.new(MyForm, init_state) This already better that doing all of the event subscribing by myself, like I did here. But, still feels kinda verbose to build a dynamic form with just two fields. I don't know if there would be a better way, but it would be nice to try. |
I have updated the docs to include your dynamic form example. |
lib/kino/screen.ex
Outdated
end | ||
|
||
def handle_event(%{data: data, type: :change}, state) do | ||
%{state | data: Map.merge(@defaults, data)} |
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.
@jonatanklosko we could remove this merge if we allow a form to have an input set to nil. In that case, the input is not rendered by the data comes back set as nil. Then I can change the default value to nil: ""
and streamline this code a bit more.
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.
I see how that would be convenient, but it does feel weird outside of this context. My main concern though, is that it would crash older Livebook versions. Perhaps we could do some special casing and transform the event before broadcasting in Kino, but I am not a fan of that either :<
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.
@jonatanklosko I think it could be used in other contexts, basically every time you need to build a form dynamically and some fields may be missing, we need to do pruning and then, when accessing the data
, use Map.get(data, field)
for those fields.
lib/kino/screen.ex
Outdated
end | ||
|
||
def handle_event(%{data: data, type: :change}, state) do | ||
%{state | data: Map.merge(@defaults, data)} |
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.
I see how that would be convenient, but it does feel weird outside of this context. My main concern though, is that it would crash older Livebook versions. Perhaps we could do some special casing and transform the event before broadcasting in Kino, but I am not a fan of that either :<
Co-authored-by: Jonatan Kłosko <[email protected]>
@josevalim lgtm! We are only missing tests. If you prefer me to add some, let me know :) |
@josevalim I'm satisfied with the API and abstraction level. I even asked Claude if we could build a So for me it's |
send(watcher, {:client_leave, "unknown_client1"}) | ||
send(watcher, {:client_leave, "client1"}) |
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.
@jonatanklosko those events are emitted by Kino.Bridge after monitoring objects. Is there a way to have the bridge send those or do I have to emulate them by sending it directly to the watcher?
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.
The message is sent from the evaluator, so for tests sending directly is the way to go. Similarly, we send control events directly, because they are sent from LV directly to the destination.
We could have some conveniences in Kino.Test
, but I usually wait until we need those in other packages, so that we don't end up with unnecessarily large API there :)
test/kino/screen_test.exs
Outdated
|
||
assert ExUnit.CaptureLog.capture_log(fn -> | ||
# Click the button and make it fail | ||
info = %{origin: "client1", type: :fail_event} |
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.
Ideally I would stick to events format used in practice. So instead of :type
here, we could render a grid with three buttons, wdyt? Shouldn't be much more code.
test/kino/screen_test.exs
Outdated
import Kino.Control | ||
|
||
defmodule MyScreen do | ||
def new(state) do |
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.
Let's name it render_fun
to make it more clear?
test/kino/screen_test.exs
Outdated
end | ||
end | ||
|
||
defp control(button, parent) do |
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.
It's preference, but I would move private functions to the end of the module :D
💚 💙 💜 💛 ❤️ |
Co-authored-by: Hugo Baraúna <[email protected]> Co-authored-by: Jonatan Kłosko <[email protected]>
I choose to keep
control/2
for now because usinghandle_event/3
requires us to wrap everything in{:noreply, ...}
tuples. But if you preferhandle_event
, let me know @hugobarauna / @jonatanklosko.I also have to figure out how to control that a certain page should only be seen by a certain user (the
client_id
).If someone wants to give it a try without depending on Kino main, here is a notebook: