-
Notifications
You must be signed in to change notification settings - Fork 366
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
Fix encoded image being suggested for non-image blobs (like video) #7428
Conversation
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.
Great!
fn update_applicability(&mut self, event: &re_chunk_store::ChunkStoreEvent) -> bool { | ||
diff_component_filter(event, |media_type: &re_types::components::MediaType| { | ||
media_type.is_image() | ||
}) || diff_component_filter(event, |image: &re_types::components::Blob| { | ||
MediaType::guess_from_data(&image.0).map_or(false, |media| media.is_image()) | ||
}) | ||
} |
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.
Not new to this PR, but I think there is room for improvement of the naming of these things, maybe:
fn update_applicability(&mut self, event: &re_chunk_store::ChunkStoreEvent) -> bool { | |
diff_component_filter(event, |media_type: &re_types::components::MediaType| { | |
media_type.is_image() | |
}) || diff_component_filter(event, |image: &re_types::components::Blob| { | |
MediaType::guess_from_data(&image.0).map_or(false, |media| media.is_image()) | |
}) | |
} | |
fn is_applicable(&mut self, event: &re_chunk_store::ChunkStoreEvent) -> bool { | |
is_any_component(event, |media_type: &re_types::components::MediaType| { | |
media_type.is_image() | |
}) || is_any_component(event, |image: &re_types::components::Blob| { | |
MediaType::guess_from_data(&image.0).map_or(false, |media| media.is_image()) | |
}) | |
} |
Also, does this really need &mut self
?
anyway, no need to fix now. Just thinking out loud.
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'm not gonna change the trait and all associated implemenations now in this PR ;)
The semantics of update
are correct and very relevant as this is an incremental and stateful update of the applicability. Details are on the trait's documentation
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.
&mut self
is for convenience if a trait implementor wants to track additional data that influence applicability
What
Before:
After, the button is greyed out.
Made sure images still work fine by dragging in an image.
Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.