Skip to content

Introduce variant UIs in re_component_ui#10034

Merged
abey79 merged 11 commits into
mainfrom
antoine/component-ui-variant
May 22, 2025
Merged

Introduce variant UIs in re_component_ui#10034
abey79 merged 11 commits into
mainfrom
antoine/component-ui-variant

Conversation

@abey79
Copy link
Copy Markdown
Member

@abey79 abey79 commented May 21, 2025

Related

What

This PR add the capability to register and use arbitrary pieces of Arrow-based UI that are not necessarily linked to a specific component. They are called "variant UI", as opposed to "component UI".

To summarise, component UIs:

  • are registered against a component name
  • are (typically) implemented with closures that accept (and return, if editable) fully deserialised component
  • (because of what precedes) are inflexible w.r.t the arrow datatype then can handle
  • support view and edit, single line and multiline

In contrast, variant UIs (for now):

  • are registered against a VariantName (conceptually a &'static str constant)
  • are implemented with closures that accept raw arrow data
  • are flexible w.r.t the arrow datatype they can handle
  • (because of what precedes) should explicitly document what arrow datatype they expect/support
  • only support edit, single line (for now)

Implementation notes:

  • For now, the variant UI are registered with add_variant_ui and used with variant_ui_raw, which is mostly an independent, parallel path from the pre-existing stuff.
  • Component descriptor and row id are now passed to UntypedComponentEditOrViewCallback, because these are will be convenient to have in variant ui impls.
  • This PR includes a redap_uri_button variant ui, which handles data that happens to be rerun-conforming URIs.

@abey79 abey79 added 📺 re_viewer affects re_viewer itself include in changelog labels May 21, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
5a5a549 https://rerun.io/viewer/pr/10034 +nightly +main

Note: This comment is updated whenever you push a commit.

@Wumpf Wumpf self-requested a review May 21, 2025 15:01
Copy link
Copy Markdown
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

neat, looks straight forward


let uri = RedapUri::from_str(url_str)?;

//TODO(ab): we should provide feedback if the URI is already loaded, e.g. have "go to" instead of "open"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is that hard? we know all the open stores and we know their source, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

shouldn't be hard, basically about the URI <-> StoreId matching, which might be slightly dirty at this point. I've already an issue open about this so I'll reference it from there.

.clicked()
{
let url = if ui.input(|i| i.modifiers.command) {
egui::OpenUrl::new_tab(uri)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

woah, this works on the web now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well probably not in the way you think.

Regular click -> open the recording in the current viewer and display it.
Cmd-click -> open the recording in the current viewer, but do not switch to it (aka leave the DisplayMode alone)

IMO this is defo the intended behaviour on native. What should happen on web is debatable. I can see myself wanting either same viewer or separate browser tab, depending on whether I'm focused on latency or ram :) Anyways, orthogonal to this PR. This is dealt with by the bit that hooks into egui and custom handles rerun url schemes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, after testing this in the next PR, it appears that this is broken. But again, this has to do with the way we handle it, so orthogonal to this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah I see, thanks for clarifying

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

problem with separate browser tab is that we don't know what the user may have built around rerun

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we should probably behave differently if we are in a SPA vs. integrated context (which we currently don't know about), but I digress 😅

fallback_ui(ui, ui_layout, component_raw);
}

/// Show a UI corresponding to the provided variant name.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should document that this falls back to component name based if there's no variant found!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't though? Or am I missing something? The fallback is basically the arrow array thrown at arrow_ui

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh I misread because of gh collapsing. Should have expanded more in the diff view

Given that we have a component descriptor.. maybe we should though?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've got mixed feelings about this. Asking for a non-existent variant is likely a bug. I'll add a todo and a debug log for now, but definitely something we could revisit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@abey79 abey79 merged commit ecaabd7 into main May 22, 2025
41 checks passed
@abey79 abey79 deleted the antoine/component-ui-variant branch May 22, 2025 12:32
abey79 added a commit that referenced this pull request May 22, 2025
… button in the partition table recording links (#10035)

### Related

- fixes #9795 
- follow-up to #10034
- part of #9741 
- future work #10036

### What

This PR introduces the ability to specify on a per-column basis a
specific variant ui (see #10034) to use. To that end, a new
`ColumnBlueprint` structure is introduced and a bunch of things around
that is refactored.

This PR also uses the recently introduced `redap_url` variant UI to
display the recording link as an "open" button instead of a fully URL.



![image](https://github.com/user-attachments/assets/d512d62c-ac3c-4c46-9caa-4c9bc5305d4f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants