Add data stream progress events and file rendering#30
Conversation
Add logic to handle rendering data streams with names and/or mime types associated as a special "file" state. I am assuming that these will be relatively large so files are buffered in tempfiles rather than in memory. And then also, add a "save" button which can be used to save files to arbitrary locations for inspection.
| /// Streams every chunk of a byte reader into an anonymous temp file, returning the open | ||
| /// handle. The file is unlinked at creation (`tempfile`), so the OS reclaims it when the | ||
| /// handle is dropped or the process exits — including on a panic/abort, where `Drop` would | ||
| /// not run (the release profile sets `panic = "abort"`). | ||
| async fn stream_to_temp_file( | ||
| reader: &mut ByteStreamReader, | ||
| entry: &Arc<Mutex<TopicEntry>>, | ||
| n: u64, | ||
| total: Option<u64>, | ||
| ) -> Result<std::fs::File, String> { | ||
| use tokio::io::AsyncWriteExt; | ||
| let std_file = tempfile::tempfile_in(std::env::temp_dir()).map_err(|e| e.to_string())?; | ||
| let mut file = tokio::fs::File::from_std(std_file); | ||
| let mut processed = 0u64; | ||
| while let Some(chunk) = reader.next().await { | ||
| let chunk = chunk.map_err(|e| e.to_string())?; | ||
| file.write_all(&chunk).await.map_err(|e| e.to_string())?; | ||
| processed += chunk.len() as u64; | ||
| update_progress(entry, n, processed, total); | ||
| } | ||
| file.flush().await.map_err(|e| e.to_string())?; | ||
| Ok(file.into_std().await) | ||
| } |
There was a problem hiding this comment.
Note to any reviewers - this maybe could make sense as an extension to ByteStreamReader?
There was a problem hiding this comment.
Yep, sounds generally useful! Also note in the Swift implementation, I had the equivalent method use the temporary directory by default and return the path to the written file.
|
Logs flood the console for me. 
|
I see it comes from here: https://github.com/livekit/rust-sdks/pull/1192/changes#diff-b27b5fab061a986aea2fd0654f77f4640c712b4eec98f54b14c84fe61a1aced0R2366 The event is marked as deprecated, is this still there on purpose? |
|
Otherwise it looks good to me, works as expected. |
| image = "0.25" | ||
| log = "0.4" | ||
| parking_lot = { version = "0.12", features = ["deadlock_detection"] } | ||
| rfd = "0.15" |
There was a problem hiding this comment.
praise: Nice, didn't know about this crate!
There was a problem hiding this comment.
question: Is there a reason you didn't use the latest version (0.17.x)?
| Some(p) => format!("{} ({}%)", format_size(r.size), (p * 100.0).round() as i64), | ||
| None => format_size(r.size), | ||
| }; | ||
| let meta = format!( |
There was a problem hiding this comment.
issue(non-blocking): This is a relatively expensive operation in the render path (format is allocating).
| if r.is_file { | ||
| show_file_box(ui, service, entry, r); | ||
| } else if let Some(StreamContent::Preview(preview)) = &r.content { | ||
| ui.add(egui::Label::new(RichText::new(preview).monospace())); |
There was a problem hiding this comment.
suggestion(non-blocking): Consider how this will render for a long string and possibly use Label::wrap().
|
One other issue I noticed, but it was pre-existing so this might not be the right place to fix it. In |
Yes, it exists in the implementation on |
|
|
||
| /// A single received stream, rendered as a row (metadata line + preview/file box). | ||
| /// A row is created as soon as the stream opens and updated live from the read task. | ||
| struct ReceivedStream { |
There was a problem hiding this comment.
curiously, do you think some customers might want to implement a "cancel" feature for the large file since they will show a progress bar now ?
and do we support such "cancel" feature ?
There was a problem hiding this comment.
Today, no, this is not supported. However, we've got a ticket to implement "cancellation tokens" (the rust idiomatic way to handle cancellation, very similar to like an AbortController in the web) sdk-wide as part of the rust sdk project(s) this quarter, and as part of this it will definitely be added to this api (alongside send_text and send_bytes too most likely).
cc @ladvoc
| struct ReceivedStream { | ||
| n: u64, | ||
| sender: String, | ||
| received_at: SystemTime, |
There was a problem hiding this comment.
what is this received_at ? is it the first chunk of data received timestamp ? or last chunk of data?
should we be able to store both values ?
| enum StreamStatus { | ||
| InProgress, | ||
| Complete, | ||
| Error(String), |
There was a problem hiding this comment.
curiously , I wonder if we will have cancelled, paused, for the long term ?
There was a problem hiding this comment.
Yep probably, but IMO out of scope for now. More info here: #30 (comment)
4 participants
Warning
This change relies on the rust data streams v2 implementation. This cannot be merged until that is merged first. This is also why CI is failing.
Add logic to handle rendering data streams with names and/or mime types associated as a special "file" state. I am assuming that these will be relatively large so files are buffered in tempfiles rather than in memory. And then also, add a "save" button which can be used to save files to arbitrary locations for inspection.
Untitled.mov
I will say, design wise, I think this side panel is starting to break down a little bit now that there is a card rendered within a card. I can try some more stuff here but also I'm not an
eguiexpert so I would be interested in some ideas before investigating anything.