Repository: pop-os/popsicle (language: rust)

Issue:
Remove Sidebar
I think something got lost in translation from mockups to code. There should not be a sidebar UI going on, as it's extra UI that doesn't do anything.

The idea with the window contents layout is to mimic the Gtk+ dialog style, as seen here:

![image](https://user-images.githubusercontent.com/611168/37424210-92fc9120-2785-11e8-9f95-ca0d19605fc7.png)

```shell
zenity --info --text="<b>Here is some text</b>\n\nThis is fairly standard Gtk+ dialog style."
```


Discussion / hints:
I agree. Let's pull back a bit to the simpler look. It matches better with existing applications.

Fix patch:
diff --git a/cli/src/main.rs b/cli/src/main.rs
index 2a11be8..95ed542 100644
--- a/cli/src/main.rs
+++ b/cli/src/main.rs
@@ -98,9 +98,9 @@ fn muff() -> Result<(), String> {
         let mut pb = ProgressBar::new(image_size);
         pb.message("Reading image: ");
         pb.set_units(Units::Bytes);
-
-        let data = image
-            .read(|total| {
+        let mut data = Vec::new();
+        image
+            .read(&mut data, |total| {
                 pb.set(total);
             })
             .map_err(|err| format!("image error with image at '{}': {}", image_path, err))?;
diff --git a/gtk/src/image.rs b/gtk/src/image.rs
index 7ea563a..9c7aeef 100644
--- a/gtk/src/image.rs
+++ b/gtk/src/image.rs
@@ -1,29 +1,45 @@
-use super::ui::State;
-use gtk;
-use gtk::prelude::*;
+use super::ui::BufferingData;
 use muff::Image;
-use std::path::Path;
-use std::sync::Arc;
+use std::io;
+use std::path::{Path, PathBuf};
+use std::sync::atomic::Ordering;
+use std::sync::mpsc::Receiver;
 
-pub fn load_image<P: AsRef<Path>>(path: P, state: &State, label: &gtk::Label, next: &gtk::Button) {
+pub fn image_load_event_loop(path_receiver: Receiver<PathBuf>, buffer: &BufferingData) {
+    while let Ok(path) = path_receiver.recv() {
+        buffer.state.store(0b1, Ordering::SeqCst);
+        let (ref mut name, ref mut data) = *buffer.data.lock().unwrap();
+        match load_image(&path, data) {
+            Ok(_) => {
+                *name = path;
+                buffer.state.store(0b10, Ordering::SeqCst);
+            }
+            Err(why) => {
+                eprintln!("muff-gtk: image loading error: {}", why);
+                buffer.state.store(0b100, Ordering::SeqCst);
+            }
+        }
+    }
+}
+
+pub fn load_image<P: AsRef<Path>>(path: P, data: &mut Vec<u8>) -> io::Result<()> {
     let path = path.as_ref();
     let mut new_image = match Image::new(path) {
         Ok(image) => image,
         Err(why) => {
-            eprintln!("muff: unable to open image: {}", why);
-            return;
+            return Err(io::Error::new(
+                io::ErrorKind::Other,
+                format!("unable to open image: {}", why),
+            ));
         }
     };
 
-    let new_data = match new_image.read(|_| ()) {
-        Ok(data) => data,
-        Err(why) => {
-            eprintln!("muff: unable to read image: {}", why);
-            return;
-        }
-    };
+    if let Err(why) = new_image.read(data, |_| ()) {
+        return Err(io::Error::new(
+            io::ErrorKind::Other,
+            format!("unable to open image: {}", why),
+        ));
+    }
 
-    *state.image_data.borrow_mut() = Some(Arc::new(new_data));
-    label.set_label(&path.file_name().unwrap().to_string_lossy());
-    next.set_sensitive(true);
+    Ok(())
 }
diff --git a/gtk/src/main.rs b/gtk/src/main.rs
index cb308c8..123454c 100644
--- a/gtk/src/main.rs
+++ b/gtk/src/main.rs
@@ -10,15 +10,22 @@ mod image;
 mod ui;
 
 use std::env;
+use std::path::PathBuf;
+use std::sync::mpsc::channel;
+use std::thread;
 use ui::{App, Connect};
 
 fn main() {
-    let app = App::new();
+    let (sender, receiver) = channel::<PathBuf>();
+    let app = App::new(sender);
+
+    {
+        let buffer = app.state.buffer.clone();
+        thread::spawn(move || image::image_load_event_loop(receiver, &buffer));
+    }
 
     if let Some(iso_argument) = env::args().skip(1).next() {
-        let label = &app.content.image_view.image_path;
-        let next = &app.header.next;
-        image::load_image(&iso_argument, &app.state, label, next);
+        let _ = app.state.image_sender.send(PathBuf::from(iso_argument));
     }
 
     app.connect_events().then_execute()
diff --git a/gtk/src/ui/content/images.rs b/gtk/src/ui/content/images.rs
index 7b55e69..1537078 100644
--- a/gtk/src/ui/content/images.rs
+++ b/gtk/src/ui/content/images.rs
@@ -2,11 +2,12 @@ use gtk::*;
 use pango::EllipsizeMode;
 
 pub struct ImageView {
-    pub container:  Box,
-    pub chooser:    Button,
-    pub image_path: Label,
-    pub hash:       ComboBoxText,
-    pub hash_label: Entry,
+    pub container:         Box,
+    pub chooser_container: Stack,
+    pub chooser:           Button,
+    pub image_path:        Label,
+    pub hash:              ComboBoxText,
+    pub hash_label:        Entry,
 }
 
 impl ImageView {
@@ -34,6 +35,21 @@ impl ImageView {
         image_path.set_ellipsize(EllipsizeMode::End);
         image_path.get_style_context().map(|c| c.add_class("bold"));
 
+        let button_box = Box::new(Orientation::Vertical, 0);
+        button_box.pack_start(&chooser, false, false, 0);
+        button_box.pack_start(&image_path, false, false, 0);
+
+        let spinner = Spinner::new();
+        spinner.start();
+        let spinner_label = Label::new("Loading Image");
+        spinner_label
+            .get_style_context()
+            .map(|c| c.add_class("bold"));
+
+        let spinner_box = Box::new(Orientation::Vertical, 0);
+        spinner_box.pack_start(&spinner, false, false, 0);
+        spinner_box.pack_start(&spinner_label, false, false, 0);
+
         let hash = ComboBoxText::new();
         hash.append_text("Type");
         hash.append_text("SHA256");
@@ -50,9 +66,10 @@ impl ImageView {
         hash_container.pack_start(&hash, false, false, 0);
         hash_container.pack_start(&hash_label, true, true, 0);
 
-        let chooser_container = Box::new(Orientation::Vertical, 5);
-        chooser_container.pack_start(&chooser, false, false, 0);
-        chooser_container.pack_start(&image_path, false, false, 0);
+        let chooser_container = Stack::new();
+        chooser_container.add_named(&button_box, "chooser");
+        chooser_container.add_named(&spinner_box, "loader");
+        chooser_container.set_visible_child_name("chooser");
 
         let left_panel = Box::new(Orientation::Vertical, 0);
         left_panel
@@ -75,6 +92,7 @@ impl ImageView {
 
         ImageView {
             container,
+            chooser_container,
             chooser,
             image_path,
             hash,
diff --git a/gtk/src/ui/mod.rs b/gtk/src/ui/mod.rs
index 646faeb..1a818bd 100644
--- a/gtk/src/ui/mod.rs
+++ b/gtk/src/ui/mod.rs
@@ -7,15 +7,17 @@ mod header;
 use self::content::Content;
 pub use self::dialogs::OpenDialog;
 use self::header::Header;
-pub use self::state::Connect;
+pub use self::state::{BufferingData, Connect};
 
 // TODO: Use AtomicU64 / Bool when https://github.com/rust-lang/rust/issues/32976 is stable.
 
-use std::cell::RefCell;
+use std::cell::{Cell, RefCell};
 use std::mem;
+use std::path::PathBuf;
 use std::process;
 use std::sync::{Arc, Mutex};
 use std::sync::atomic::AtomicUsize;
+use std::sync::mpsc::Sender;
 use std::thread::JoinHandle;
 use std::time::Instant;
 
@@ -32,50 +34,38 @@ pub struct App {
     pub state:   Arc<State>,
 }
 
-// TODO:
-// pub struct BufferingData {
-//     data: Mutex<Vec<u8>>,
-//     state: AtomicUsize,
-// }
-
-// impl BufferingData {
-//     fn new() -> BufferingData {
-//         BufferingData { data: Vec::new().into(), state: 0.into() }
-//     }
-// }
-
 /// Contains all of the state that needs to be shared across the program's lifetime.
 pub struct State {
     /// Contains all of the progress bars in the flash view.
     pub bars: RefCell<Vec<(ProgressBar, Label)>>,
     /// Contains a list of devices detected, and their check buttons.
     pub devices: Mutex<Vec<(String, CheckButton)>>,
-    /// Contains a buffered vector of the ISO data, to be shared across threads.
-    pub image_data: RefCell<Option<Arc<Vec<u8>>>>,
     /// Holds the task threads that write the image to each device.
     /// The handles may contain errors when joined, for printing on the summary page.
     pub task_handles: Mutex<Vec<JoinHandle<Result<(), DiskError>>>>,
     /// Contains progress data regarding each active flash task -- namely the progress.
     pub tasks: Mutex<Vec<FlashTask>>,
     /// Stores an integer which defines the currently-active view.
-    pub view: RefCell<u8>,
+    pub view: Cell<u8>,
     /// Stores the time when the flashing process began.
     pub start: RefCell<Instant>,
-    /* TODO:
-     * buffer: BufferingData */
+    pub buffer: Arc<BufferingData>,
+    pub image_sender: Sender<PathBuf>,
+    pub image_length: Cell<usize>,
 }
 
 impl State {
-    fn new() -> State {
+    fn new(image_sender: Sender<PathBuf>) -> State {
         State {
-            bars:         RefCell::new(Vec::new()),
-            devices:      Mutex::new(Vec::new()),
-            image_data:   RefCell::new(None),
+            bars: RefCell::new(Vec::new()),
+            devices: Mutex::new(Vec::new()),
             task_handles: Mutex::new(Vec::new()),
-            tasks:        Mutex::new(Vec::new()),
-            view:         RefCell::new(0),
-            start:        RefCell::new(unsafe { mem::uninitialized() }),
-            // buffer:       BufferingData::new(),
+            tasks: Mutex::new(Vec::new()),
+            view: Cell::new(0),
+            start: RefCell::new(unsafe { mem::uninitialized() }),
+            buffer: Arc::new(BufferingData::new()),
+            image_sender,
+            image_length: Cell::new(0),
         }
     }
 }
@@ -87,7 +77,7 @@ pub struct FlashTask {
 }
 
 impl App {
-    pub fn new() -> App {
+    pub fn new(sender: Sender<PathBuf>) -> App {
         // Initialize GTK before proceeding.
         if gtk::init().is_err() {
             eprintln!("failed to initialize GTK Application");
@@ -130,7 +120,7 @@ impl App {
             window,
             header,
             content,
-            state: Arc::new(State::new()),
+            state: Arc::new(State::new(sender)),
         }
     }
 }
diff --git a/gtk/src/ui/state.rs b/gtk/src/ui/state.rs
index f8f33e6..e4551da 100644
--- a/gtk/src/ui/state.rs
+++ b/gtk/src/ui/state.rs
@@ -1,8 +1,8 @@
 use super::{hash, App, FlashTask, OpenDialog};
-use super::super::image::load_image;
 
-use std::ops::Deref;
-use std::path::Path;
+use std::mem;
+use std::ops::{Deref, DerefMut};
+use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::thread;
@@ -12,6 +12,20 @@ use gtk;
 use gtk::*;
 use muff::{self, DiskError};
 
+pub struct BufferingData {
+    pub data:  Mutex<(PathBuf, Vec<u8>)>,
+    pub state: AtomicUsize,
+}
+
+impl BufferingData {
+    pub fn new() -> BufferingData {
+        BufferingData {
+            data:  Mutex::new((PathBuf::new(), Vec::new())),
+            state: 0.into(),
+        }
+    }
+}
+
 pub struct Connected(App);
 
 impl Connected {
@@ -60,12 +74,10 @@ impl Connect for App {
     }
 
     fn connect_image_chooser(&self) {
-        let path_label = self.content.image_view.image_path.clone();
-        let next = self.header.next.clone();
         let state = self.state.clone();
         self.content.image_view.chooser.connect_clicked(move |_| {
             if let Some(path) = OpenDialog::new(None).run() {
-                load_image(&path, &state, &path_label, &next);
+                let _ = state.image_sender.send(path);
             }
         });
     }
@@ -74,7 +86,8 @@ impl Connect for App {
         let state = self.state.clone();
         let hash_label = self.content.image_view.hash_label.clone();
         self.content.image_view.hash.connect_changed(move |hash| {
-            if let Some(ref data) = *state.image_data.borrow() {
+            if state.buffer.state.load(Ordering::SeqCst) == 0b010 {
+                let (_, ref data) = *state.buffer.data.lock().unwrap();
                 hash_label.set_icon_from_icon_name(EntryIconPosition::Primary, "gnome-spinner");
                 hash_label.set_icon_sensitive(EntryIconPosition::Primary, true);
                 hash::set(&hash_label, hash.get_active_text().unwrap().as_str(), data);
@@ -89,7 +102,8 @@ impl Connect for App {
         let next = self.header.next.clone();
         let state = self.state.clone();
         back.connect_clicked(move |back| {
-            match *state.view.borrow() {
+            let view = state.view.get();
+            match view {
                 0 => gtk::main_quit(),
                 1 => {
                     stack.set_transition_type(StackTransitionType::SlideRight);
@@ -104,7 +118,8 @@ impl Connect for App {
                 }
                 _ => unreachable!(),
             }
-            *state.view.borrow_mut() -= 1;
+
+            state.view.set(view - 1);
         });
     }
 
@@ -118,14 +133,17 @@ impl Connect for App {
 
         next.connect_clicked(move |next| {
             let device_list = &state.devices;
-            let image_data = &state.image_data;
+            state.buffer.state.store(0b1000, Ordering::SeqCst);
+            let (_, ref mut image_data) = *state.buffer.data.lock().unwrap();
             let start = &state.start;
             let task_handles = &state.task_handles;
             let bars = &state.bars;
             let tasks = &state.tasks;
             let view = &state.view;
+            let view_value = view.get();
             stack.set_transition_type(StackTransitionType::SlideLeft);
-            match *view.borrow() {
+
+            match view_value {
                 // Move to device selection screen
                 0 => {
                     back.set_label("Back");
@@ -179,10 +197,19 @@ impl Connect for App {
                     *start.borrow_mut() = Instant::now();
                     let mut tasks = tasks.lock().unwrap();
                     let mut task_handles = task_handles.lock().unwrap();
+
+                    // Take ownership of the data, so that we may wrap it within an `Arc`
+                    // and redistribute it across threads.
+                    //
+                    // Note: Possible optimization could be done to avoid the wrap.
+                    //       Avoiding the wrap could eliminate two allocations.
+                    let mut data = Vec::new();
+                    mem::swap(&mut data, image_data);
+                    let image_data = Arc::new(data);
+
                     for (id, (disk_path, mut disk)) in disks.into_iter().enumerate() {
                         let id = id as i32;
-                        let image_data = image_data.borrow();
-                        let image_data = image_data.as_ref().unwrap().clone();
+                        let image_data = image_data.clone();
                         let progress = Arc::new(AtomicUsize::new(0));
                         let finished = Arc::new(AtomicUsize::new(0));
                         let bar = ProgressBar::new();
@@ -244,7 +271,7 @@ impl Connect for App {
                 _ => unreachable!(),
             }
 
-            *view.borrow_mut() += 1;
+            view.set(view_value + 1);
         });
     }
 
@@ -269,21 +296,43 @@ impl Connect for App {
         let description = self.content.summary_view.description.clone();
         let list = self.content.summary_view.list.clone();
         let state = self.state.clone();
+        let image_label = self.content.image_view.image_path.clone();
+        let chooser_container = self.content.image_view.chooser_container.clone();
 
-        use std::ops::DerefMut;
         gtk::timeout_add(500, move || {
             let tasks = &state.tasks;
             let bars = &state.bars;
-            let image_data = &state.image_data;
             let devices = &state.devices;
             let task_handles = &state.task_handles;
+            let image_length = &state.image_length;
 
-            let image_length = match *image_data.borrow() {
-                Some(ref data) => data.len() as f64,
-                None => {
+            // Ensure that the image has been loaded before continuing.
+            match state.buffer.state.load(Ordering::SeqCst) {
+                0b0000 => {
                     return Continue(true);
                 }
-            };
+                0b0001 => {
+                    chooser_container.set_visible_child_name("loader");
+                    next.set_sensitive(false);
+                    return Continue(true);
+                }
+                0b0010 => {
+                    chooser_container.set_visible_child_name("chooser");
+                    let (ref path, ref data) = *state.buffer.data.lock().unwrap();
+                    next.set_sensitive(true);
+                    image_label.set_text(&path.file_name().unwrap().to_string_lossy());
+                    image_length.set(data.len());
+                }
+                0b0100 => {
+                    chooser_container.set_visible_child_name("chooser");
+                    next.set_sensitive(false);
+                    return Continue(true);
+                }
+                0b1000 => (),
+                _ => unreachable!(),
+            }
+
+            let image_length = image_length.get();
 
             let tasks = tasks.lock().unwrap();
             let ntasks = tasks.len();
@@ -298,7 +347,7 @@ impl Connect for App {
                     1.0f64
                 } else {
                     finished = false;
-                    raw_value as f64 / image_length
+                    raw_value as f64 / image_length as f64
                 };
 
                 bar.set_fraction(value);
diff --git a/gtk/src/ui/ui.css b/gtk/src/ui/ui.css
index 6f51c7f..0da61b6 100644
--- a/gtk/src/ui/ui.css
+++ b/gtk/src/ui/ui.css
@@ -1,6 +1,4 @@
 .left-panel {
-    background-color: @theme_fg_color;
-    box-shadow: 0 0 .3em;
     margin-right: .5em;
 }
 
diff --git a/src/lib.rs b/src/lib.rs
index 1af28ea..d3ca4c1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -71,8 +71,19 @@ impl Image {
     pub fn get_size(&self) -> u64 { self.size }
 
     /// Reads the image into a vector, and reports progress to a callback.
-    pub fn read<P: FnMut(u64)>(&mut self, mut progress_callback: P) -> Result<Vec<u8>, ImageError> {
-        let mut data = vec![0; self.size as usize];
+    pub fn read<P: FnMut(u64)>(
+        &mut self,
+        data: &mut Vec<u8>,
+        mut progress_callback: P,
+    ) -> Result<(), ImageError> {
+        if data.capacity() < self.size as usize {
+            let capacity = self.size as usize - data.capacity();
+            data.reserve_exact(capacity);
+            data.append(&mut vec![0; capacity])
+        } else if data.capacity() > self.size as usize {
+            data.truncate(self.size as usize);
+            data.shrink_to_fit();
+        }
 
         let mut total = 0;
         while total < data.len() {
@@ -88,7 +99,7 @@ impl Image {
             progress_callback(total as u64);
         }
 
-        Ok(data)
+        Ok(())
     }
 }
 


Test patch: