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

Reduce need for mutable App access, towards decoupling with the ViewerContext #2333

Closed
wants to merge 1 commit into from

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jun 8, 2023

What

In hope to help out #2330, but doesn't nearly go far enough yet.

Interior mutability for App's pending commands, app options and selected recording & blueprint

Checklist

PR Build Summary: https://build.rerun.io/pr/2333

Docs preview: https://rerun.io/preview/e95997a/docs
Examples preview: https://rerun.io/preview/e95997a/examples

@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🚜 refactor Change the code, not the functionality labels Jun 8, 2023
@@ -38,6 +38,7 @@ serde_json = "1"
strum = { version = "0.24", features = ["derive"] }
strum_macros = "0.24"
sublime_fuzzy = "0.7"
crossbeam.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

sort!

#[serde(skip)]
pub(crate) selected_blueprint_by_app: HashMap<ApplicationId, StoreId>,
selected_blueprint_by_app: RwLock<HashMap<ApplicationId, StoreId>>,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good solution. It's nice that &AppState means read-only, and adding interior mutability to everything is quite unidiomatic, and an ugly way to circumvent the borrowchecker when we should not need to circumvent it at all

@Wumpf Wumpf marked this pull request as draft June 8, 2023 14:05
@Wumpf Wumpf closed this Jun 8, 2023
@Wumpf Wumpf deleted the andreas/reduce-mutable-app branch June 8, 2023 15:23
Wumpf added a commit that referenced this pull request Jun 8, 2023
…ithout mutable access (#2339)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

Allows for decoupling a bunch of things!
The good parts from #2333 ;)

### 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)

<!-- This line will get updated when the PR build summary job finishes.
-->
PR Build Summary: https://build.rerun.io/pr/2339

<!-- pr-link-docs:start -->
Docs preview: https://rerun.io/preview/c58f5af/docs
Examples preview: https://rerun.io/preview/c58f5af/examples
<!-- pr-link-docs:end -->

---------

Co-authored-by: Jeremy Leibs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📺 re_viewer affects re_viewer itself 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants