Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 46 additions & 33 deletions src/launcher/batcher.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use color_eyre::eyre::{Result, ensure};
use color_eyre::eyre::{Result, ensure, eyre};

use tracing::{debug, info};

Expand Down Expand Up @@ -115,6 +115,18 @@ impl<Cushion> Default for BatcherState<Cushion> {
}
}

pub struct Prepared<T>(Buffer<(T, usize)>);

impl<T> Prepared<T> {
pub(crate) fn into_inner(self) -> Buffer<(T, usize)> {
self.0
}

pub(crate) fn new(value: Buffer<(T, usize)>) -> Self {
Self(value)
}
}

impl<Cushion, UIContext> Batcher<Cushion, UIContext>
where
Cushion: Send,
Expand Down Expand Up @@ -157,16 +169,26 @@ where

/// Prepares the next batch of indices for rendering.
///
/// This asynchronous function generates and returns a `Buffer<usize>` containing indices
/// This asynchronous function generates and returns a `Prepared<UIContext>` containing indices
/// that correspond to UI elements needing rendering. The resulting buffer is intended to be used
/// in conjunction with a rendering buffer of type `Buffer<(UIContext, usize)>` during the merge process.
///
/// For optimal performance, it is recommended that this function runs concurrently with the rendering process.
#[must_use]
#[inline]
pub async fn prepare(&mut self) -> Buffer<usize> {
pub async fn prepare(&mut self) -> Prepared<UIContext> {
info!("Preparing");
debug!("state on prepare {:?}", self.state);

if self.cushion_to_ui.is_none() {
panic!(
"{}",
eyre!(
"Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)"
)
);
Comment on lines +183 to +189
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The panic! macro immediately terminates the current thread. While this can be useful in development to catch unexpected states, it's generally better to handle errors more gracefully in production code. Consider returning a Result with an appropriate error message to allow the caller to handle the error case.

Also, the error message is very specific and includes implementation details. It would be better to provide a more general error message that doesn't expose internal workings.

        if self.cushion_to_ui.is_none() {
            return Err(eyre("UIContext is not set.")); // More general error message
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

@gemini-code-assist This branch is probably unreachable because the UI type parameter will not be decided without being set via the function(it also sets ctuf) or adding a type annotation. The user must know what they are doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that under normal, correct usage, this branch might be unreachable. However, in Rust, panic! is generally reserved for unrecoverable errors or programmer bugs that indicate a fundamental flaw in the program's logic. For situations where an API contract might be violated by the caller (e.g., cushion_to_ui not being set), returning a Result is the idiomatic way to signal an error that the caller can then handle gracefully.

Even if the user 'must know what they are doing,' a panic! will crash the thread, which is often undesirable in a larger application. Returning an Err allows the application to recover, log the issue, or present a user-friendly error message without terminating.

Additionally, the original error message still contains internal details. A more general message would be beneficial regardless of whether it's a panic! or an Err.

Suggested change
if self.cushion_to_ui.is_none() {
panic!(
"{}",
eyre!(
"Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)"
)
);
if self.cushion_to_ui.is_none() {
return Err(eyre("UIContext is not set.")); // More general error message
}

}

let mut batch_count = if self.batch_size == 0 {
usize::MAX
} else {
Expand Down Expand Up @@ -262,6 +284,8 @@ where
}
}

let ctuf = self.cushion_to_ui.as_ref().unwrap();

let mut v: Vec<_> = v
.into_iter()
.filter(|ci| {
Expand All @@ -275,11 +299,14 @@ where
.any(|filter| filter.predicate(&self.state.items[*ci], &self.state.input))
}
})
.map(|ci| (ctuf(&self.state.items[ci]), ci))
.collect();

v.sort_by(self.create_sorter());
let sorterf = self.create_sorter();

v.sort_by(|(_, lhs), (_, rhs)| sorterf(lhs, rhs));

v.into()
Prepared::new(v.into())
}

/// Merges UI context data into the rendering buffer.
Expand All @@ -299,18 +326,14 @@ where
pub fn merge(
&mut self,
buf: &mut Buffer<(UIContext, usize)>,
mut from: Buffer<usize>,
from: Prepared<UIContext>,
) -> Result<bool> {
ensure!(
self.cushion_to_ui.is_some(),
"Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)"
);
debug!("state on merge: {:?}", self.state);

// sorterは順番に適用していくのと、逆にしてstd::Ordering::Equalが出たら次のやつを参照するっていうのが同義っぽいきがする
// どっちにするかだけど、std::Ordering::Equalが出たら戻るほうが(ここでは逆にしたりしない)計算量が少なそう

let v = from.as_mut();
let v = from.into_inner().into_inner();

let sorterf = self.create_sorter();

Expand All @@ -321,23 +344,20 @@ where
let mut merged = Vec::with_capacity(dst_owned.len() + v.len());

let mut iter_dst = dst_owned.into_iter();
let mut iter_src = v.iter();
let mut iter_src = v.into_iter();

let mut next_dst = iter_dst.next();
let mut next_src = iter_src.next();

while let (Some(a), Some(b)) = (next_dst.as_ref(), next_src.as_ref()) {
if sorterf(&a.1, b) != std::cmp::Ordering::Greater {
merged.push(next_dst.take().unwrap());
while next_src.is_some() && next_dst.is_some() {
let a = next_dst.take().unwrap();
let b = next_src.take().unwrap();

if sorterf(&a.1, &b.1) != std::cmp::Ordering::Greater {
merged.push(a);
next_dst = iter_dst.next();
} else {
merged.push({
let ui_ctx = (self.cushion_to_ui.as_ref().unwrap())(
&self.state.items[*next_src.unwrap()],
);

(ui_ctx, *next_src.unwrap())
});
merged.push(b);
next_src = iter_src.next();
}
}
Expand All @@ -347,16 +367,8 @@ where
merged.extend(iter_dst);
}
if let Some(val) = next_src {
merged.push({
let ui_ctx = (self.cushion_to_ui.as_ref().unwrap())(&self.state.items[*val]);

(ui_ctx, *val)
});
merged.extend(iter_src.map(|ci| {
let ui_ctx = (self.cushion_to_ui.as_ref().unwrap())(&self.state.items[*ci]);

(ui_ctx, *ci)
}));
merged.push(val);
merged.extend(iter_src);
}

*dst = merged;
Expand Down Expand Up @@ -460,11 +472,12 @@ mod tests {
#[tokio::test]
async fn test_prepare() -> Result<(), Box<dyn std::error::Error>> {
let mut batcher: Batcher<i32, ()> = Batcher::default();
batcher.cushion_to_ui = Some(Box::new(|_: &i32| ()));

batcher.add_raw_source(Box::pin(tokio_stream::iter(vec![1, 2])));

let buf = batcher.prepare().await;
assert_eq!(buf.len(), 2);
assert_eq!(buf.0.len(), 2);
Ok(())
}
}
5 changes: 5 additions & 0 deletions src/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ impl<T> Buffer<T> {
*self = Self::default();
}

#[inline]
pub(crate) fn into_inner(self) -> Vec<T> {
self.vec
}

#[inline]
pub(crate) fn as_mut(&mut self) -> &mut Vec<T> {
&mut self.vec
Expand Down