Add display_name() to SpaceViewClass so space views may provide a user-facing name
#4393
Conversation
abey79
added
🚜 refactor
| /// Must be unique within a viewer session. | ||
| /// | ||
| /// TODO(#2336): Display name and identifier should be separate. | ||
| /// Used for identification. Must be unique within a viewer session. | ||
| fn name(&self) -> SpaceViewClassName; |
There was a problem hiding this comment.
the logical next/additional step would be to rename this to identifier(&self) -> SpaceViewClassIdentifier I think in order to avoid confusion
There was a problem hiding this comment.
I agree with that, but I'll punt on it as consistency would require a large sweep across the code base.
There was a problem hiding this comment.
I think it is worth doing though; do it in a follow-up PR if you like. rust-analyzers rename feature should make it pretty painless.
There was a problem hiding this comment.
Let's see about painless (function argument names...), but will do 👍🏻
| /// User-facing name of this space view class. | ||
| /// | ||
| /// Used for UI display. | ||
| fn ui_name(&self) -> &'static str; |
There was a problem hiding this comment.
I think the more common name for this in the codebase is display_name.
... come to think it's more like the default display name? Because the user can edit the actual display name. From that pov ui_name is probably good as is since it avoid confusing it with the configurable display_name of a space view instance 🤔 . Your call :)
There was a problem hiding this comment.
Agreed again, and this one is worth fixing 👍🏻
There was a problem hiding this comment.
...but disagree on default_display_name. This is the class's display name, aka the space view type, which is displayed as such. It turns out the space view class display name is also used as part of the default space view instance naming scheme, but that's accidental (and that bit has a shaky future imho).
…fiedViewSystem` (#4406) ### What As per #4393 (comment) Review per commit: we could merge either or both. ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [app.rerun.io](https://app.rerun.io/pr/4406) (if applicable) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4406) - [Docs preview](https://rerun.io/preview/5063dd0203b5563eaccde1c46c76e176f576bee7/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/5063dd0203b5563eaccde1c46c76e176f576bee7/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
abey79
changed the title
ui_name() to SpaceViewClass so space views may provide a user-facing namedisplay_name() to SpaceViewClass so space views may provide a user-facing name
What
With this change, the user-facing string for a space view class can be changed without breaking blueprint files. This PR also reverts the change from "TextLog" to "Text Log" to avoid breaking last release's blueprint (*)
(*) Unknown space view class name do not trigger a blueprint rebuild, but instead yield an "unknown space view" in the UI, which is Bad UX™️
Checklist