-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
avoid reloading invalid views #4889
Conversation
245b9f3
to
b912d9c
Compare
b912d9c
to
eef3cca
Compare
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.
Would #4888 make this obsolete?
Yes and no, it would not be needed but having this effectively prevents a regression if changes are made to the close mechanism or view map assumptions. Happy to close it if it's not needed :) |
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 think we should avoid being defensive here: by being defensive we might allow broken assumptions to cause hard-to-debug quirks
.filter_map(|id| match cx.editor.tree.contains(id) { | ||
true => Some(id), | ||
false => None, | ||
}) |
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 think clippy is saying here that this could be filter(|id| cx.editor.tree.contains(*id))
rather than a filter_map
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 think we should avoid being defensive here: by being defensive we might allow broken assumptions to cause hard-to-debug quirks
No worries, I wasnt sure how large a fix the close stuff would be so decided to check this route as well. I'll close the PR
Closes #4878