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

Refactor view resolver such it it enables passing a ViewerContext to SpaceViewClass::default_query_range() #6918

Open
abey79 opened this issue Jul 16, 2024 · 1 comment
Labels
🚜 refactor Change the code, not the functionality

Comments

@abey79
Copy link
Member

abey79 commented Jul 16, 2024

#6917 could have been made much better if it was possible to pass ViewerContext and friends to SpaceViewClass::default_query_range(), thereby enabling the default visible time range to be derived from view properties.

Quoting that PR:

Note: this is a rather poor solution. Ideally, we would also have &ViewContext and friends, such that it would be possible to e.g. read view properties. This is however not possible because default_query_range() is called in a context where, all the way up in AppState, a ViewerContext is not available (precisely because it's being mutated). Improving on that would require a much more significant refactor. Passing the state at least allows ugly work around such as passing a value computed from e.g. SpaceViewClass:ui() (yes, ugly).

@abey79 abey79 added the 🚜 refactor Change the code, not the functionality label Jul 16, 2024
Wumpf pushed a commit that referenced this issue Jul 17, 2024
### What

This PR provides the view state to
`SpaceViewClass::default_query_range()`, such that the default query
range may be dynamic.

**Note**: this is a rather poor solution. Ideally, we would also have
`&ViewContext` and friends, such that it would be possible to e.g. read
view properties. This is however not possible because
`default_query_range()` is called in a context where, all the way up in
`AppState`, a `ViewerContext` is not available (precisely because it's
being mutated). Improving on that would require a much more significant
refactor. Passing the state at least allows ugly work around such as
passing a value computed from e.g. `SpaceViewClass:ui()` (yes, ugly).

- See #6918

### 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 the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6917?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6917?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/6917)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
@abey79
Copy link
Member Author

abey79 commented Aug 7, 2024

Albeit this still makes sense, the use-case I needed it for disappeared in #7071

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚜 refactor Change the code, not the functionality
Projects
None yet
Development

No branches or pull requests

1 participant