First PR review changes

src/app_state.rs

@@ -1,16 +1,18 @@
-use anyhow::{bail, Context, Result};
+use anyhow::{bail, Context, Error, Result};
 use crossterm::{
-    queue,
+    style::{ResetColor, SetForegroundColor},
-    style::{Print, ResetColor, SetForegroundColor},
+    terminal, QueueableCommand,
-    terminal,
 };
 use std::{
     env,
     fs::{File, OpenOptions},
-    io::{self, Read, Seek, StdoutLock, Write},
+    io::{Read, Seek, StdoutLock, Write},
     path::{Path, MAIN_SEPARATOR_STR},
     process::{Command, Stdio},
-    sync::{atomic::AtomicUsize, mpsc, Arc},
+    sync::{
+        atomic::{AtomicUsize, Ordering::Relaxed},
+        mpsc,
+    },
     thread,
 };
 
@@ -42,11 +44,17 @@ pub enum StateFileStatus {
     NotRead,
 }
 
-#[derive(Clone, Copy, PartialEq)]
+enum ExerciseCheckProgress {
-enum AllExercisesResult {
+    Checking,
+    Done,
+    Pending,
+    Error,
+}
+
+#[derive(Clone, Copy)]
+enum ExerciseCheckResult {
+    Done,
     Pending,
-    Success,
-    Failed,
     Error,
 }
 
@@ -280,7 +288,7 @@ impl AppState {
     }
 
     // Set the status of an exercise without saving. Returns `true` if the
-    // status actually changed (and thus needs saving later)
+    // status actually changed (and thus needs saving later).
     pub fn set_status(&mut self, exercise_ind: usize, done: bool) -> Result<bool> {
         let exercise = self
             .exercises
@@ -288,23 +296,25 @@ impl AppState {
             .context(BAD_INDEX_ERR)?;
 
         if exercise.done == done {
-            Ok(false)
+            return Ok(false);
-        } else {
+        }
+
         exercise.done = done;
         if done {
             self.n_done += 1;
         } else {
             self.n_done -= 1;
         }
+
         Ok(true)
     }
-    }
 
-    // Set the status of an exercise to "pending" and save
+    // Set the status of an exercise to "pending" and save.
     pub fn set_pending(&mut self, exercise_ind: usize) -> Result<()> {
         if self.set_status(exercise_ind, false)? {
             self.write()?;
         }
+
         Ok(())
     }
 
@@ -403,173 +413,154 @@ impl AppState {
     }
 
     // Return the exercise index of the first pending exercise found.
-    pub fn check_all_exercises(
+    pub fn check_all_exercises(&mut self, stdout: &mut StdoutLock) -> Result<Option<usize>> {
-        &mut self,
+        stdout.write_all("Checking all exercises…\n".as_bytes())?;
-        stdout: &mut StdoutLock,
+        let n_exercises = self.exercises.len() as u16;
-        final_check: bool,
+        let next_exercise_ind = AtomicUsize::new(0);
-    ) -> Result<Option<usize>> {
+        let term_width = terminal::size()
-        if !final_check {
+            .context("Failed to get the terminal size")?
-            stdout.write_all(INTERMEDIATE_CHECK_MSG)?;
+            .0;
-        } else {
-            stdout.write_all(FINAL_CHECK_MSG)?;
-        }
-        let n_exercises = self.exercises.len();
 
-        let (mut checked_count, mut results) = thread::scope(|s| {
+        let mut results = vec![ExerciseCheckResult::Error; self.exercises.len()];
-            let (tx, rx) = mpsc::channel();
+        let mut done = 0;
-            let exercise_ind = Arc::new(AtomicUsize::default());
-
-            let num_core = thread::available_parallelism()
-                .map_or(DEFAULT_CHECK_PARALLELISM, |count| count.get());
-            (0..num_core).for_each(|_| {
-                let tx = tx.clone();
-                let exercise_ind = exercise_ind.clone();
-                let this = &self;
-                let _ = thread::Builder::new().spawn_scoped(s, move || {
-                    loop {
-                        let exercise_ind =
-                            exercise_ind.fetch_add(1, std::sync::atomic::Ordering::AcqRel);
-                        let Some(exercise) = this.exercises.get(exercise_ind) else {
-                            // No more exercises
-                            break;
-                        };
-
-                        // Notify the progress bar that this exercise is pending
-                        if tx.send((exercise_ind, None)).is_err() {
-                            break;
-                        };
-
-                        let result = exercise.run_exercise(None, &this.cmd_runner);
-
-                        // Notify the progress bar that this exercise is done
-                        if tx.send((exercise_ind, Some(result))).is_err() {
-                            break;
-                        }
-                    }
-                });
-            });
-
-            // Drop this `tx`, since the `rx` loop will not stop while there is
-            // at least one tx alive (i.e. we want the loop to block only while
-            // there are `tx` clones, i.e. threads)
-            drop(tx);
-
-            // Print the legend
-            queue!(
-                stdout,
-                Print("Color legend:  "),
-                SetForegroundColor(term::PROGRESS_FAILED_COLOR),
-                Print("Failure"),
-                ResetColor,
-                Print("  -  "),
-                SetForegroundColor(term::PROGRESS_SUCCESS_COLOR),
-                Print("Success"),
-                ResetColor,
-                Print("  -  "),
-                SetForegroundColor(term::PROGRESS_PENDING_COLOR),
-                Print("Checking"),
-                ResetColor,
-                Print("\n"),
-            )
-            .unwrap();
-            // We expect at least a few "pending" notifications shortly, so don't
-            // bother printing the initial state of the progress bar and flushing
-            // stdout
-
-            let line_width = terminal::size().unwrap().0;
-            let mut results = vec![AllExercisesResult::Pending; n_exercises];
         let mut pending = 0;
-            let mut success = 0;
-            let mut failed = 0;
 
-            while let Ok((exercise_ind, result)) = rx.recv() {
+        thread::scope(|s| {
+            let mut checking = 0;
+            let (exercise_result_sender, exercise_result_receiver) = mpsc::channel();
+            let n_threads = thread::available_parallelism()
+                .map_or(DEFAULT_CHECK_PARALLELISM, |count| count.get());
+
+            for _ in 0..n_threads {
+                let exercise_result_sender = exercise_result_sender.clone();
+                let next_exercise_ind = &next_exercise_ind;
+                let slf = &self;
+                thread::Builder::new()
+                    .spawn_scoped(s, move || loop {
+                        let exercise_ind = next_exercise_ind.fetch_add(1, Relaxed);
+                        let Some(exercise) = slf.exercises.get(exercise_ind) else {
+                            // No more exercises.
+                            break;
+                        };
+
+                        // Notify the progress bar that this exercise is pending.
+                        if exercise_result_sender
+                            .send((exercise_ind, ExerciseCheckProgress::Checking))
+                            .is_err()
+                        {
+                            break;
+                        };
+
+                        let success = exercise.run_exercise(None, &slf.cmd_runner);
+                        let result = match success {
+                            Ok(true) => ExerciseCheckProgress::Done,
+                            Ok(false) => ExerciseCheckProgress::Pending,
+                            Err(_) => ExerciseCheckProgress::Error,
+                        };
+
+                        // Notify the progress bar that this exercise is done.
+                        if exercise_result_sender.send((exercise_ind, result)).is_err() {
+                            break;
+                        }
+                    })
+                    .context("Failed to spawn a thread to check all exercises")?;
+            }
+
+            // Drop this sender to detect when the last thread is done.
+            drop(exercise_result_sender);
+
+            // Print the legend.
+            stdout.write_all(b"Color legend: ")?;
+            stdout.queue(SetForegroundColor(term::PROGRESS_FAILED_COLOR))?;
+            stdout.write_all(b"Pending")?;
+            stdout.queue(ResetColor)?;
+            stdout.write_all(b" - ")?;
+            stdout.queue(SetForegroundColor(term::PROGRESS_SUCCESS_COLOR))?;
+            stdout.write_all(b"Done")?;
+            stdout.queue(ResetColor)?;
+            stdout.write_all(b" - ")?;
+            stdout.queue(SetForegroundColor(term::PROGRESS_PENDING_COLOR))?;
+            stdout.write_all(b"Checking")?;
+            stdout.queue(ResetColor)?;
+            stdout.write_all(b"\n")?;
+
+            while let Ok((exercise_ind, result)) = exercise_result_receiver.recv() {
                 match result {
-                    None => {
+                    ExerciseCheckProgress::Checking => checking += 1,
+                    ExerciseCheckProgress::Done => {
+                        results[exercise_ind] = ExerciseCheckResult::Done;
+                        checking -= 1;
+                        done += 1;
+                    }
+                    ExerciseCheckProgress::Pending => {
+                        results[exercise_ind] = ExerciseCheckResult::Pending;
+                        checking -= 1;
                         pending += 1;
                     }
-                    Some(Err(_)) => {
+                    ExerciseCheckProgress::Error => checking -= 1,
-                        results[exercise_ind] = AllExercisesResult::Error;
-                    }
-                    Some(Ok(true)) => {
-                        results[exercise_ind] = AllExercisesResult::Success;
-                        pending -= 1;
-                        success += 1;
-                    }
-                    Some(Ok(false)) => {
-                        results[exercise_ind] = AllExercisesResult::Failed;
-                        pending -= 1;
-                        failed += 1;
-                    }
                 }
 
-                write!(stdout, "\r").unwrap();
+                stdout.write_all(b"\r")?;
                 progress_bar_with_success(
                     stdout,
+                    checking,
                     pending,
-                    failed,
+                    done,
-                    success,
+                    n_exercises,
-                    n_exercises as u16,
+                    term_width,
-                    line_width,
+                )?;
-                )
-                .unwrap();
                 stdout.flush()?;
             }
 
-            Ok::<_, io::Error>((success, results))
+            Ok::<_, Error>(())
         })?;
 
+        let mut first_pending_exercise_ind = None;
+        for (exercise_ind, result) in results.into_iter().enumerate() {
+            match result {
+                ExerciseCheckResult::Done => {
+                    self.set_status(exercise_ind, true)?;
+                }
+                ExerciseCheckResult::Pending => {
+                    self.set_status(exercise_ind, false)?;
+                    if first_pending_exercise_ind.is_none() {
+                        first_pending_exercise_ind = Some(exercise_ind);
+                    }
+                }
+                ExerciseCheckResult::Error => {
                     // If we got an error while checking all exercises in parallel,
                     // it could be because we exceeded the limit of open file descriptors.
-        // Therefore, re-try those one at a time (i.e. sequentially).
+                    // Therefore, try running exercises with errors sequentially.
-        results
+                    let exercise = &self.exercises[exercise_ind];
-            .iter_mut()
+                    let success = exercise.run_exercise(None, &self.cmd_runner)?;
-            .enumerate()
+                    if success {
-            .filter(|(_, result)| {
+                        done += 1;
-                **result == AllExercisesResult::Pending || **result == AllExercisesResult::Error
+                    } else {
-            })
+                        pending += 1;
-            .try_for_each(|(exercise_ind, result)| {
+                        if first_pending_exercise_ind.is_none() {
-                let exercise = self.exercises.get(exercise_ind).context(BAD_INDEX_ERR)?;
+                            first_pending_exercise_ind = Some(exercise_ind);
-                *result = match exercise
+                        }
-                    .run_exercise(None, &self.cmd_runner)
+                    }
-                    .context("Sequential retry")
+                    self.set_status(exercise_ind, success)?;
-                {
+
-                    Ok(true) => AllExercisesResult::Success,
+                    stdout.write_all(b"\r")?;
-                    Ok(false) => AllExercisesResult::Failed,
+                    progress_bar_with_success(
-                    Err(err) => bail!(err),
+                        stdout,
-                };
+                        u16::from(pending + done < n_exercises),
-                checked_count += 1;
+                        pending,
-                write!(stdout, "\rProgress: {checked_count}/{n_exercises}")?;
+                        done,
+                        n_exercises,
+                        term_width,
+                    )?;
                     stdout.flush()?;
-                Ok(())
+                }
-            })?;
+            }
+        }
 
-        // Update the state of each exercise and return the first that failed
-        let first_fail = results
-            .iter()
-            .enumerate()
-            .filter_map(|(exercise_ind, result)| {
-                match result {
-                    AllExercisesResult::Success => self
-                        .set_status(exercise_ind, true)
-                        .map_or_else(|err| Some(Err(err)), |_| None),
-                    AllExercisesResult::Failed => self
-                        .set_status(exercise_ind, false)
-                        .map_or_else(|err| Some(Err(err)), |_| Some(Ok(exercise_ind))),
-                    // The sequential check done earlier will have converted all
-                    // exercises to Success/Failed, or bailed, so those are unreachable
-                    AllExercisesResult::Pending | AllExercisesResult::Error => unreachable!(),
-                }
-            })
-            .try_fold(None::<usize>, |current_min, index| {
-                match (current_min, index) {
-                    (_, Err(err)) => Err(err),
-                    (None, Ok(index)) => Ok(Some(index)),
-                    (Some(current_min), Ok(index)) => Ok(Some(current_min.min(index))),
-                }
-            })?;
         self.write()?;
+        stdout.write_all(b"\n\n")?;
 
-        Ok(first_fail)
+        Ok(first_pending_exercise_ind)
     }
 
     /// Mark the current exercise as done and move on to the next pending exercise if one exists.
@@ -596,18 +587,12 @@ impl AppState {
             stdout.write_all(b"\n")?;
         }
 
-        if let Some(pending_exercise_ind) = self.check_all_exercises(stdout, true)? {
+        if let Some(first_pending_exercise_ind) = self.check_all_exercises(stdout)? {
-            stdout.write_all(b"\n\n")?;
+            self.set_current_exercise_ind(first_pending_exercise_ind)?;
-
-            self.current_exercise_ind = pending_exercise_ind;
-            self.exercises[pending_exercise_ind].done = false;
 
             return Ok(ExercisesProgress::NewPending);
         }
 
-        // Write that the last exercise is done.
-        self.write()?;
-
         self.render_final_message(stdout)?;
 
         Ok(ExercisesProgress::AllDone)
@@ -629,11 +614,6 @@ impl AppState {
 
 const BAD_INDEX_ERR: &str = "The current exercise index is higher than the number of exercises";
 const STATE_FILE_HEADER: &[u8] = b"DON'T EDIT THIS FILE!\n\n";
-const INTERMEDIATE_CHECK_MSG: &[u8] = b"Checking all exercises
-";
-const FINAL_CHECK_MSG: &[u8] = b"All exercises seem to be done.
-Recompiling and running all exercises to make sure that all of them are actually done.
-";
 const FENISH_LINE: &str = "+----------------------------------------------------+
 |          You made it to the Fe-nish line!          |
 +--------------------------  ------------------------+

src/main.rs

@@ -146,7 +146,7 @@ fn main() -> Result<()> {
         }
         Some(Subcommands::RunAll) => {
             let mut stdout = io::stdout().lock();
-            if let Some(first_fail) = app_state.check_all_exercises(&mut stdout, false)? {
+            if let Some(first_fail) = app_state.check_all_exercises(&mut stdout)? {
                 let pending = app_state
                     .exercises()
                     .iter()
@@ -156,7 +156,6 @@ fn main() -> Result<()> {
                     app_state.set_current_exercise_ind(first_fail)?;
                 }
                 stdout
-                    .queue(Print("\n"))?
                     .queue(SetForegroundColor(Color::Red))?
                     .queue(Print(format!("{pending}")))?
                     .queue(ResetColor)?;

src/term.rs

@@ -89,34 +89,35 @@ impl<'a> CountedWrite<'a> for StdoutLock<'a> {
     }
 }
 
-/// Simple terminal progress bar
+/// Simple terminal progress bar.
 pub fn progress_bar<'a>(
     writer: &mut impl CountedWrite<'a>,
     progress: u16,
     total: u16,
-    line_width: u16,
+    term_width: u16,
 ) -> io::Result<()> {
-    progress_bar_with_success(writer, 0, 0, progress, total, line_width)
+    progress_bar_with_success(writer, 0, 0, progress, total, term_width)
 }
-/// Terminal progress bar with three states (pending + failed + success)
+
+/// Terminal progress bar with three states (pending + failed + success).
 pub fn progress_bar_with_success<'a>(
     writer: &mut impl CountedWrite<'a>,
     pending: u16,
     failed: u16,
     success: u16,
     total: u16,
-    line_width: u16,
+    term_width: u16,
 ) -> io::Result<()> {
     debug_assert!(total < 1000);
-    debug_assert!((pending + failed + success) <= total);
+    debug_assert!(pending + failed + success <= total);
 
     const PREFIX: &[u8] = b"Progress: [";
     const PREFIX_WIDTH: u16 = PREFIX.len() as u16;
     const POSTFIX_WIDTH: u16 = "] xxx/xxx".len() as u16;
     const WRAPPER_WIDTH: u16 = PREFIX_WIDTH + POSTFIX_WIDTH;
-    const MIN_LINE_WIDTH: u16 = WRAPPER_WIDTH + 4;
+    const MIN_TERM_WIDTH: u16 = WRAPPER_WIDTH + 4;
 
-    if line_width < MIN_LINE_WIDTH {
+    if term_width < MIN_TERM_WIDTH {
         writer.write_ascii(b"Progress: ")?;
         // Integers are in ASCII.
         return writer.write_ascii(format!("{}/{total}", failed + success).as_bytes());
@@ -125,7 +126,7 @@ pub fn progress_bar_with_success<'a>(
     let stdout = writer.stdout();
     stdout.write_all(PREFIX)?;
 
-    let width = line_width - WRAPPER_WIDTH;
+    let width = term_width - WRAPPER_WIDTH;
     let mut failed_end = (width * failed) / total;
     let mut success_end = (width * (failed + success)) / total;
     let mut pending_end = (width * (failed + success + pending)) / total;

src/watch.rs

@@ -108,7 +108,7 @@ fn run_watch(
             {
                 ExercisesProgress::AllDone => break,
                 ExercisesProgress::NewPending => watch_state.run_current_exercise(&mut stdout)?,
-                ExercisesProgress::CurrentPending => (),
+                ExercisesProgress::CurrentPending => watch_state.render(&mut stdout)?,
             },
             WatchEvent::Input(InputEvent::Reset) => watch_state.reset_exercise(&mut stdout)?,
             WatchEvent::Input(InputEvent::Quit) => {

src/watch/state.rs

@@ -157,8 +157,9 @@ impl<'a> WatchState<'a> {
 
     /// Move on to the next exercise if the current one is done.
     pub fn next_exercise(&mut self, stdout: &mut StdoutLock) -> Result<ExercisesProgress> {
-        if self.done_status == DoneStatus::Pending {
+        match self.done_status {
-            return Ok(ExercisesProgress::CurrentPending);
+            DoneStatus::DoneWithSolution(_) | DoneStatus::DoneWithoutSolution => (),
+            DoneStatus::Pending => return Ok(ExercisesProgress::CurrentPending),
         }
 
         self.app_state.done_current_exercise::<true>(stdout)
@@ -282,14 +283,15 @@ impl<'a> WatchState<'a> {
     pub fn check_all_exercises(&mut self, stdout: &mut StdoutLock) -> Result<ExercisesProgress> {
         stdout.write_all(b"\n")?;
 
-        if let Some(first_fail) = self.app_state.check_all_exercises(stdout, false)? {
+        if let Some(first_pending_exercise_ind) = self.app_state.check_all_exercises(stdout)? {
-            // Only change exercise if the current one is done...
+            // Only change exercise if the current one is done.
             if self.app_state.current_exercise().done {
-                self.app_state.set_current_exercise_ind(first_fail)?;
+                self.app_state
-            }
+                    .set_current_exercise_ind(first_pending_exercise_ind)?;
-            // ...but always pretend it's a "new" anyway because that refreshes
-            // the display
                 Ok(ExercisesProgress::NewPending)
+            } else {
+                Ok(ExercisesProgress::CurrentPending)
+            }
         } else {
             self.app_state.render_final_message(stdout)?;
             Ok(ExercisesProgress::AllDone)