From ed5ff16598b6c47ecbc94b190eedb07b07b28a2b Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 02:00:32 +0200 Subject: [PATCH 01/12] Downloader: clean a bit downloader::get_episode function. --- hammond-downloader/src/downloader.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/hammond-downloader/src/downloader.rs b/hammond-downloader/src/downloader.rs index 53851aa..cb2a94a 100644 --- a/hammond-downloader/src/downloader.rs +++ b/hammond-downloader/src/downloader.rs @@ -172,29 +172,24 @@ pub fn get_episode( ep.save()?; }; - let res = download_into( + let path = download_into( download_folder, &ep.rowid().to_string(), ep.uri().unwrap(), progress, - ); + )?; - if let Ok(path) = res { - // If download succedes set episode local_uri to dlpath. - ep.set_local_uri(Some(&path)); + // If download succedes set episode local_uri to dlpath. + ep.set_local_uri(Some(&path)); - // Over-write episode lenght - let size = fs::metadata(path); - if let Ok(s) = size { - ep.set_length(Some(s.len() as i32)) - }; + // Over-write episode lenght + let size = fs::metadata(path); + if let Ok(s) = size { + ep.set_length(Some(s.len() as i32)) + }; - ep.save()?; - Ok(()) - } else { - error!("Something whent wrong while downloading."); - Err(res.unwrap_err()) - } + ep.save()?; + Ok(()) } pub fn cache_image(pd: &PodcastCoverQuery) -> Option { From 0dc16dab9a6008754503e5b7d26f98614b341531 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 02:31:53 +0200 Subject: [PATCH 02/12] EpisodeWidget: Refactor to return Result wherever possible. --- hammond-data/src/models/source.rs | 2 +- hammond-gtk/src/main.rs | 4 +- hammond-gtk/src/views/episodes.rs | 5 +- hammond-gtk/src/widgets/episode.rs | 235 ++++++++++++++++------------- 4 files changed, 136 insertions(+), 110 deletions(-) diff --git a/hammond-data/src/models/source.rs b/hammond-data/src/models/source.rs index 4a52dc0..92ec4cb 100644 --- a/hammond-data/src/models/source.rs +++ b/hammond-data/src/models/source.rs @@ -212,7 +212,7 @@ impl Source { /// Updates the validator Http Headers. /// /// Consumes `self` and Returns the corresponding `Feed` Object. - // TODO: Refactor into TryInto once it lands on stable. + // Refactor into TryInto once it lands on stable. pub fn into_feed( self, client: &Client>, diff --git a/hammond-gtk/src/main.rs b/hammond-gtk/src/main.rs index e241db8..67d69c3 100644 --- a/hammond-gtk/src/main.rs +++ b/hammond-gtk/src/main.rs @@ -9,8 +9,8 @@ extern crate gtk; #[macro_use] extern crate failure; -#[macro_use] -extern crate failure_derive; +// #[macro_use] +// extern crate failure_derive; #[macro_use] extern crate lazy_static; #[macro_use] diff --git a/hammond-gtk/src/views/episodes.rs b/hammond-gtk/src/views/episodes.rs index e32b4ae..94d63bd 100644 --- a/hammond-gtk/src/views/episodes.rs +++ b/hammond-gtk/src/views/episodes.rs @@ -9,7 +9,6 @@ use app::Action; use utils::get_pixbuf_from_path; use widgets::episode::EpisodeWidget; -use std::sync::Arc; use std::sync::mpsc::Sender; #[derive(Debug, Clone)] @@ -75,7 +74,7 @@ impl Default for EpisodesView { // TODO: REFACTOR ME impl EpisodesView { - pub fn new(sender: Sender) -> Arc { + pub fn new(sender: Sender) -> EpisodesView { let view = EpisodesView::default(); let episodes = dbqueries::get_episodes_widgets_with_limit(50).unwrap(); let now_utc = Utc::now(); @@ -124,7 +123,7 @@ impl EpisodesView { } view.container.show_all(); - Arc::new(view) + view } pub fn is_empty(&self) -> bool { diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 2886176..ba8ab7c 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -123,13 +123,20 @@ impl EpisodeWidget { self.show_buttons(episode.local_uri()); // Determine what the state of the progress bar should be. - self.determine_progess_bar(); + if let Err(err) = self.determine_progess_bar() { + error!("Something went wrong determining the ProgressBar State."); + error!("Error: {}", err); + } let title = &self.title; self.play .connect_clicked(clone!(episode, title, sender => move |_| { let mut episode = episode.clone(); - on_play_bttn_clicked(episode.rowid()); + + if let Err(err) = on_play_bttn_clicked(episode.rowid()) { + error!("Error: {}", err); + }; + if episode.set_played_now().is_ok() { title .get_style_context() @@ -141,7 +148,12 @@ impl EpisodeWidget { self.download .connect_clicked(clone!(episode, sender => move |dl| { dl.set_sensitive(false); - on_download_clicked(&episode, sender.clone()); + if let Err(err) = on_download_clicked(&episode, sender.clone()) { + error!("Download failed to start."); + error!("Error: {}", err); + } else { + info!("Donwload started succesfully."); + } })); } @@ -204,26 +216,22 @@ impl EpisodeWidget { } // FIXME: REFACTOR ME - fn determine_progess_bar(&self) { + // Something Something State-Machine? + fn determine_progess_bar(&self) -> Result<(), Error> { let id = WidgetExt::get_name(&self.container) - .unwrap() - .parse::() - .unwrap(); + .ok_or_else(|| format_err!("Failed to get widget Name"))? + .parse::()?; - let prog_struct = || -> Option<_> { - if let Ok(m) = manager::ACTIVE_DOWNLOADS.read() { - if !m.contains_key(&id) { - return None; - }; - return m.get(&id).cloned(); - } - None - }(); + let active_dl = || -> Result, Error> { + let m = manager::ACTIVE_DOWNLOADS + .read() + .map_err(|_| format_err!("Failed to get a lock on the mutex."))?; - let progress_bar = self.progress.clone(); - let total_size = self.total_size.clone(); - let local_size = self.local_size.clone(); - if let Some(prog) = prog_struct { + Ok(m.get(&id).cloned()) + }()?; + + if let Some(prog) = active_dl { + // FIXME: Document me? self.download.hide(); self.progress.show(); self.local_size.show(); @@ -232,13 +240,17 @@ impl EpisodeWidget { self.prog_separator.show(); self.cancel.show(); + let progress_bar = self.progress.clone(); + let total_size = self.total_size.clone(); + let local_size = self.local_size.clone(); + // Setup a callback that will update the progress bar. - update_progressbar_callback(prog.clone(), id, progress_bar, local_size); + update_progressbar_callback(prog.clone(), id, &progress_bar, &local_size); // Setup a callback that will update the total_size label // with the http ContentLength header number rather than // relying to the RSS feed. - update_total_size_callback(prog.clone(), total_size); + update_total_size_callback(prog.clone(), &total_size); self.cancel.connect_clicked(clone!(prog => move |cancel| { if let Ok(mut m) = prog.lock() { @@ -247,44 +259,37 @@ impl EpisodeWidget { } })); } + + Ok(()) } } -fn on_download_clicked(ep: &EpisodeWidgetQuery, sender: Sender) { - let download_fold = dbqueries::get_podcast_from_id(ep.podcast_id()) - .ok() - .map(|pd| get_download_folder(&pd.title().to_owned()).ok()) - .and_then(|x| x); +fn on_download_clicked(ep: &EpisodeWidgetQuery, sender: Sender) -> Result<(), Error> { + let pd = dbqueries::get_podcast_from_id(ep.podcast_id())?; + let download_fold = get_download_folder(&pd.title().to_owned())?; // Start a new download. - if let Some(fold) = download_fold { - manager::add(ep.rowid(), &fold, sender.clone()); - } + manager::add(ep.rowid(), &download_fold, sender.clone()); // Update Views - sender.send(Action::RefreshEpisodesView).unwrap(); - sender.send(Action::RefreshWidgetIfVis).unwrap(); + sender.send(Action::RefreshEpisodesView)?; + sender.send(Action::RefreshWidgetIfVis)?; + + Ok(()) } -fn on_play_bttn_clicked(episode_id: i32) { - let local_uri = dbqueries::get_episode_local_uri_from_id(episode_id) - .ok() - .and_then(|x| x); +fn on_play_bttn_clicked(episode_id: i32) -> Result<(), Error> { + let uri = dbqueries::get_episode_local_uri_from_id(episode_id)? + .ok_or_else(|| format_err!("Expected Some found None."))?; - if let Some(uri) = local_uri { - if Path::new(&uri).exists() { - info!("Opening {}", uri); - open::that(&uri).err().map(|err| { - error!("Error while trying to open file: {}", uri); - error!("Error: {}", err); - }); - } + if Path::new(&uri).exists() { + info!("Opening {}", uri); + open::that(&uri)?; } else { - error!( - "Something went wrong evaluating the following path: {:?}", - local_uri - ); + bail!("File \"{}\" does not exist.", uri); } + + Ok(()) } // Setup a callback that will update the progress bar. @@ -292,80 +297,102 @@ fn on_play_bttn_clicked(episode_id: i32) { fn update_progressbar_callback( prog: Arc>, episode_rowid: i32, - progress_bar: gtk::ProgressBar, - local_size: gtk::Label, + progress_bar: >k::ProgressBar, + local_size: >k::Label, ) { timeout_add( 400, - clone!(prog, progress_bar => move || { - let (fraction, downloaded) = { - let m = prog.lock().unwrap(); - (m.get_fraction(), m.get_downloaded()) - }; - - // Update local_size label - downloaded.file_size(SIZE_OPTS.clone()).ok().map(|x| local_size.set_text(&x)); - - // I hate floating points. - // Update the progress_bar. - if (fraction >= 0.0) && (fraction <= 1.0) && (!fraction.is_nan()) { - progress_bar.set_fraction(fraction); - } - - // info!("Fraction: {}", progress_bar.get_fraction()); - // info!("Fraction: {}", fraction); - - // Check if the download is still active - let active = { - let m = manager::ACTIVE_DOWNLOADS.read().unwrap(); - m.contains_key(&episode_rowid) - }; - - if (fraction >= 1.0) && (!fraction.is_nan()){ - glib::Continue(false) - } else if !active { - glib::Continue(false) - } else { - glib::Continue(true) - } + clone!(prog, progress_bar, progress_bar, local_size=> move || { + progress_bar_helper(prog.clone(), episode_rowid, &progress_bar, &local_size) + .unwrap_or(glib::Continue(false)) }), ); } +fn progress_bar_helper( + prog: Arc>, + episode_rowid: i32, + progress_bar: >k::ProgressBar, + local_size: >k::Label, +) -> Result { + let (fraction, downloaded) = { + let m = prog.lock() + .map_err(|_| format_err!("Failed to get a lock on the mutex."))?; + (m.get_fraction(), m.get_downloaded()) + }; + + // Update local_size label + downloaded + .file_size(SIZE_OPTS.clone()) + .map_err(|err| format_err!("{}", err)) + .map(|x| local_size.set_text(&x))?; + + // I hate floating points. + // Update the progress_bar. + if (fraction >= 0.0) && (fraction <= 1.0) && (!fraction.is_nan()) { + progress_bar.set_fraction(fraction); + } + + // info!("Fraction: {}", progress_bar.get_fraction()); + // info!("Fraction: {}", fraction); + + // Check if the download is still active + let active = { + let m = manager::ACTIVE_DOWNLOADS + .read() + .map_err(|_| format_err!("Failed to get a lock on the mutex."))?; + m.contains_key(&episode_rowid) + }; + + if (fraction >= 1.0) && (!fraction.is_nan()) { + Ok(glib::Continue(false)) + } else if !active { + Ok(glib::Continue(false)) + } else { + Ok(glib::Continue(true)) + } +} + // Setup a callback that will update the total_size label // with the http ContentLength header number rather than // relying to the RSS feed. -fn update_total_size_callback(prog: Arc>, total_size: gtk::Label) { +fn update_total_size_callback(prog: Arc>, total_size: >k::Label) { timeout_add( 500, clone!(prog, total_size => move || { - let total_bytes = { - let m = prog.lock().unwrap(); - m.get_total_size() - }; - - debug!("Total Size: {}", total_bytes); - if total_bytes != 0 { - // Update the total_size label - total_bytes.file_size(SIZE_OPTS.clone()).ok().map(|x| total_size.set_text(&x)); - glib::Continue(false) - } else { - glib::Continue(true) - } + total_size_helper(prog.clone(), &total_size).unwrap_or(glib::Continue(true)) }), ); } -// fn on_delete_bttn_clicked(episode_id: i32) { -// let mut ep = dbqueries::get_episode_from_rowid(episode_id) -// .unwrap() -// .into(); +fn total_size_helper( + prog: Arc>, + total_size: >k::Label, +) -> Result { + // Get the total_bytes. + let total_bytes = { + let m = prog.lock() + .map_err(|_| format_err!("Failed to get a lock on the mutex."))?; + m.get_total_size() + }; -// let e = delete_local_content(&mut ep); -// if let Err(err) = e { -// error!("Error while trying to delete file: {:?}", ep.local_uri()); -// error!("Error: {}", err); -// }; + debug!("Total Size: {}", total_bytes); + if total_bytes != 0 { + // Update the total_size label + total_bytes + .file_size(SIZE_OPTS.clone()) + .map_err(|err| format_err!("{}", err)) + .map(|x| total_size.set_text(&x))?; + // Do not call again the callback + Ok(glib::Continue(false)) + } else { + Ok(glib::Continue(true)) + } +} + +// fn on_delete_bttn_clicked(episode_id: i32) -> Result<(), Error> { +// let mut ep = dbqueries::get_episode_from_rowid(episode_id)?.into(); +// delete_local_content(&mut ep).map_err(From::from).map(|_| ()) // } pub fn episodes_listbox(pd: &Podcast, sender: Sender) -> Result { From d3f279374ab2225fd93a10c0814bd52fa3e7d1e7 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 02:47:39 +0200 Subject: [PATCH 03/12] ShowWidget: Refactor to return Result wherever possible. --- hammond-gtk/src/widgets/show.rs | 37 +++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/hammond-gtk/src/widgets/show.rs b/hammond-gtk/src/widgets/show.rs index 0573903..f8f8add 100644 --- a/hammond-gtk/src/widgets/show.rs +++ b/hammond-gtk/src/widgets/show.rs @@ -1,4 +1,5 @@ use dissolve; +use failure::Error; use gtk; use gtk::prelude::*; use open; @@ -65,7 +66,9 @@ impl ShowWidget { self.unsub .connect_clicked(clone!(pd, sender => move |bttn| { - on_unsub_button_clicked(&pd, bttn, sender.clone()); + if let Err(err) = on_unsub_button_clicked(&pd, bttn, sender.clone()) { + error!("Error: {}", err); + } })); self.setup_listbox(pd, sender.clone()); @@ -76,9 +79,10 @@ impl ShowWidget { self.link.set_tooltip_text(Some(link.as_str())); self.link.connect_clicked(move |_| { info!("Opening link: {}", &link); - open::that(&link) - .err() - .map(|err| error!("Something went wrong: {}", err)); + if let Err(err) = open::that(&link) { + error!("Failed to open link: {}", &link); + error!("Error: {}", err); + } }); } @@ -107,7 +111,11 @@ impl ShowWidget { } } -fn on_unsub_button_clicked(pd: &Podcast, unsub_button: >k::Button, sender: Sender) { +fn on_unsub_button_clicked( + pd: &Podcast, + unsub_button: >k::Button, + sender: Sender, +) -> Result<(), Error> { // hack to get away without properly checking for none. // if pressed twice would panic. unsub_button.hide(); @@ -118,16 +126,19 @@ fn on_unsub_button_clicked(pd: &Podcast, unsub_button: >k::Button, sender: Sen error!("Error: {}", err); } })); - sender.send(Action::HeaderBarNormal).unwrap(); - sender.send(Action::ShowShowsAnimated).unwrap(); + + sender.send(Action::HeaderBarNormal)?; + sender.send(Action::ShowShowsAnimated)?; // Queue a refresh after the switch to avoid blocking the db. - sender.send(Action::RefreshShowsView).unwrap(); - sender.send(Action::RefreshEpisodesView).unwrap(); + sender.send(Action::RefreshShowsView)?; + sender.send(Action::RefreshEpisodesView)?; + + Ok(()) } #[allow(dead_code)] -fn on_played_button_clicked(pd: &Podcast, sender: Sender) { - let _ = dbqueries::update_none_to_played_now(pd); - - sender.send(Action::RefreshWidget).unwrap(); +fn on_played_button_clicked(pd: &Podcast, sender: Sender) -> Result<(), Error> { + dbqueries::update_none_to_played_now(pd)?; + sender.send(Action::RefreshWidget)?; + Ok(()) } From c6e426cbacd261b31762b022060c3475506d493e Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 20:14:03 +0200 Subject: [PATCH 04/12] Downloader: Change cache_image function to return Result instead of Option. --- hammond-downloader/src/downloader.rs | 40 +++++++++++++++------------- hammond-downloader/src/errors.rs | 10 +++++-- hammond-gtk/src/app.rs | 4 +-- hammond-gtk/src/utils.rs | 3 ++- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/hammond-downloader/src/downloader.rs b/hammond-downloader/src/downloader.rs index cb2a94a..4fa9bee 100644 --- a/hammond-downloader/src/downloader.rs +++ b/hammond-downloader/src/downloader.rs @@ -192,40 +192,39 @@ pub fn get_episode( Ok(()) } -pub fn cache_image(pd: &PodcastCoverQuery) -> Option { - let url = pd.image_uri()?.to_owned(); +pub fn cache_image(pd: &PodcastCoverQuery) -> Result { + let url = pd.image_uri() + .ok_or_else(|| DownloadError::NoImageLocation)? + .to_owned(); + if url == "" { - return None; + return Err(DownloadError::NoImageLocation); } - let cache_download_fold = format!("{}{}", HAMMOND_CACHE.to_str()?, pd.title().to_owned()); + let cache_path = HAMMOND_CACHE + .to_str() + .ok_or_else(|| DownloadError::InvalidCacheLocation)?; + let cache_download_fold = format!("{}{}", cache_path, pd.title().to_owned()); // Weird glob magic. if let Ok(mut foo) = glob(&format!("{}/cover.*", cache_download_fold)) { // For some reason there is no .first() method so nth(0) is used let path = foo.nth(0).and_then(|x| x.ok()); if let Some(p) = path { - return Some(p.to_str()?.into()); + return Ok(p.to_str() + .ok_or_else(|| DownloadError::InvalidCachedImageLocation)? + .into()); } }; // Create the folders if they don't exist. DirBuilder::new() .recursive(true) - .create(&cache_download_fold) - .ok()?; + .create(&cache_download_fold)?; - match download_into(&cache_download_fold, "cover", &url, None) { - Ok(path) => { - info!("Cached img into: {}", &path); - Some(path) - } - Err(err) => { - error!("Failed to get feed image."); - error!("Error: {}", err); - None - } - } + let path = download_into(&cache_download_fold, "cover", &url, None)?; + info!("Cached img into: {}", &path); + Ok(path) } #[cfg(test)] @@ -235,6 +234,8 @@ mod tests { use hammond_data::dbqueries; use hammond_data::pipeline; + use std::fs; + #[test] // This test inserts an rss feed to your `XDG_DATA/hammond/hammond.db` so we make it explicit // to run it. @@ -257,6 +258,7 @@ mod tests { HAMMOND_CACHE.to_str().unwrap(), pd.title() ); - assert_eq!(img_path, Some(foo_)); + assert_eq!(img_path.unwrap(), foo_); + fs::remove_file(foo_).unwrap(); } } diff --git a/hammond-downloader/src/errors.rs b/hammond-downloader/src/errors.rs index 7df3bd1..43d9400 100644 --- a/hammond-downloader/src/errors.rs +++ b/hammond-downloader/src/errors.rs @@ -10,10 +10,16 @@ pub enum DownloadError { DataError(#[cause] DataError), #[fail(display = "Io error: {}", _0)] IoError(#[cause] io::Error), - #[fail(display = "The Download was cancelled")] - DownloadCancelled, #[fail(display = "Unexpected server response: {}", _0)] UnexpectedResponse(reqwest::StatusCode), + #[fail(display = "The Download was cancelled.")] + DownloadCancelled, + #[fail(display = "Remote Image location not found.")] + NoImageLocation, + #[fail(display = "Failed to parse CacheLocation.")] + InvalidCacheLocation, + #[fail(display = "Failed to parse Cached Image Location.")] + InvalidCachedImageLocation, } impl From for DownloadError { diff --git a/hammond-gtk/src/app.rs b/hammond-gtk/src/app.rs index 8bb95f5..fc48072 100644 --- a/hammond-gtk/src/app.rs +++ b/hammond-gtk/src/app.rs @@ -122,9 +122,9 @@ impl App { match receiver.recv_timeout(Duration::from_millis(10)) { Ok(Action::UpdateSources(source)) => { if let Some(s) = source { - utils::refresh_feed(Some(vec![s]), sender.clone()) + utils::refresh_feed(Some(vec![s]), sender.clone()); } else { - utils::refresh_feed(None, sender.clone()) + utils::refresh_feed(None, sender.clone()); } } Ok(Action::RefreshAllViews) => content.update(), diff --git a/hammond-gtk/src/utils.rs b/hammond-gtk/src/utils.rs index f300f18..aa896c7 100644 --- a/hammond-gtk/src/utils.rs +++ b/hammond-gtk/src/utils.rs @@ -1,5 +1,6 @@ #![cfg_attr(feature = "cargo-clippy", allow(type_complexity))] +use failure::Error; use gdk_pixbuf::Pixbuf; use send_cell::SendCell; @@ -82,7 +83,7 @@ pub fn get_pixbuf_from_path(pd: &PodcastCoverQuery, size: u32) -> Option } } - let img_path = downloader::cache_image(pd)?; + let img_path = downloader::cache_image(pd).ok()?; let px = Pixbuf::new_from_file_at_scale(&img_path, size as i32, size as i32, true).ok(); if let Some(px) = px { let mut hashmap = CACHED_PIXBUFS.write().unwrap(); From 7ed1cd8b263e6998e9a294b77b18b5d256e19136 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 20:56:55 +0200 Subject: [PATCH 05/12] hammond-gtk: Change utils::get_pixbuf_from_path function to return a Result. --- hammond-gtk/src/utils.rs | 31 ++++++++++++++++--------------- hammond-gtk/src/views/episodes.rs | 27 ++++++++++++++++++++------- hammond-gtk/src/views/shows.rs | 14 ++++++++++---- hammond-gtk/src/widgets/show.rs | 13 ++++++++----- 4 files changed, 54 insertions(+), 31 deletions(-) diff --git a/hammond-gtk/src/utils.rs b/hammond-gtk/src/utils.rs index aa896c7..ffac353 100644 --- a/hammond-gtk/src/utils.rs +++ b/hammond-gtk/src/utils.rs @@ -73,24 +73,25 @@ lazy_static! { // GObjects do not implement Send trait, so SendCell is a way around that. // Also lazy_static requires Sync trait, so that's what the mutexes are. // TODO: maybe use something that would just scale to requested size? -pub fn get_pixbuf_from_path(pd: &PodcastCoverQuery, size: u32) -> Option { +pub fn get_pixbuf_from_path(pd: &PodcastCoverQuery, size: u32) -> Result { { - let hashmap = CACHED_PIXBUFS.read().unwrap(); - let res = hashmap.get(&(pd.id(), size)); - if let Some(px) = res { - let m = px.lock().unwrap(); - return Some(m.clone().into_inner()); + let hashmap = CACHED_PIXBUFS + .read() + .map_err(|_| format_err!("Failed to get a lock on the pixbuf cache mutex."))?; + if let Some(px) = hashmap.get(&(pd.id(), size)) { + let m = px.lock() + .map_err(|_| format_err!("Failed to lock pixbuf mutex."))?; + return Ok(m.clone().into_inner()); } } - let img_path = downloader::cache_image(pd).ok()?; - let px = Pixbuf::new_from_file_at_scale(&img_path, size as i32, size as i32, true).ok(); - if let Some(px) = px { - let mut hashmap = CACHED_PIXBUFS.write().unwrap(); - hashmap.insert((pd.id(), size), Mutex::new(SendCell::new(px.clone()))); - return Some(px); - } - None + let img_path = downloader::cache_image(pd)?; + let px = Pixbuf::new_from_file_at_scale(&img_path, size as i32, size as i32, true)?; + let mut hashmap = CACHED_PIXBUFS + .write() + .map_err(|_| format_err!("Failed to lock pixbuf mutex."))?; + hashmap.insert((pd.id(), size), Mutex::new(SendCell::new(px.clone()))); + Ok(px) } #[cfg(test)] @@ -114,6 +115,6 @@ mod tests { // Get the Podcast let pd = dbqueries::get_podcast_from_source_id(sid).unwrap(); let pxbuf = get_pixbuf_from_path(&pd.into(), 256); - assert!(pxbuf.is_some()); + assert!(pxbuf.is_ok()); } } diff --git a/hammond-gtk/src/views/episodes.rs b/hammond-gtk/src/views/episodes.rs index 94d63bd..e4893d6 100644 --- a/hammond-gtk/src/views/episodes.rs +++ b/hammond-gtk/src/views/episodes.rs @@ -1,4 +1,5 @@ use chrono::prelude::*; +use failure::Error; use gtk; use gtk::prelude::*; @@ -202,18 +203,30 @@ impl EpisodesViewWidget { gtk::Builder::new_from_resource("/org/gnome/hammond/gtk/episodes_view_widget.ui"); let container: gtk::Box = builder.get_object("container").unwrap(); let image: gtk::Image = builder.get_object("cover").unwrap(); - - if let Ok(pd) = dbqueries::get_podcast_cover_from_id(episode.podcast_id()) { - get_pixbuf_from_path(&pd, 64).map(|img| image.set_from_pixbuf(&img)); - } - let ep = EpisodeWidget::new(episode, sender.clone()); - container.pack_start(&ep.container, true, true, 6); - EpisodesViewWidget { + let view = EpisodesViewWidget { container, image, episode: ep.container, + }; + + view.init(episode); + view + } + + fn init(&self, episode: &mut EpisodeWidgetQuery) { + if let Err(err) = self.set_cover(episode) { + error!("Failed to set a cover: {}", err) } + + self.container.pack_start(&self.episode, true, true, 6); + } + + fn set_cover(&self, episode: &mut EpisodeWidgetQuery) -> Result<(), Error> { + let pd = dbqueries::get_podcast_cover_from_id(episode.podcast_id())?; + let img = get_pixbuf_from_path(&pd, 64)?; + self.image.set_from_pixbuf(&img); + Ok(()) } } diff --git a/hammond-gtk/src/views/shows.rs b/hammond-gtk/src/views/shows.rs index a0a0851..3f72981 100644 --- a/hammond-gtk/src/views/shows.rs +++ b/hammond-gtk/src/views/shows.rs @@ -1,3 +1,4 @@ +use failure::Error; use gtk; use gtk::prelude::*; @@ -114,11 +115,16 @@ impl ShowsChild { fn init(&self, pd: &Podcast) { self.container.set_tooltip_text(pd.title()); - let cover = get_pixbuf_from_path(&pd.clone().into(), 256); - if let Some(img) = cover { - self.cover.set_from_pixbuf(&img); - }; + if let Err(err) = self.set_cover(pd) { + error!("Failed to set a cover: {}", err) + } WidgetExt::set_name(&self.child, &pd.id().to_string()); } + + fn set_cover(&self, pd: &Podcast) -> Result<(), Error> { + let image = get_pixbuf_from_path(&pd.clone().into(), 256)?; + self.cover.set_from_pixbuf(&image); + Ok(()) + } } diff --git a/hammond-gtk/src/widgets/show.rs b/hammond-gtk/src/widgets/show.rs index f8f8add..132be8f 100644 --- a/hammond-gtk/src/widgets/show.rs +++ b/hammond-gtk/src/widgets/show.rs @@ -72,9 +72,12 @@ impl ShowWidget { })); self.setup_listbox(pd, sender.clone()); - self.set_cover(pd); self.set_description(pd.description()); + if let Err(err) = self.set_cover(pd) { + error!("Failed to set a cover: {}", err) + } + let link = pd.link().to_owned(); self.link.set_tooltip_text(Some(link.as_str())); self.link.connect_clicked(move |_| { @@ -91,11 +94,11 @@ impl ShowWidget { let listbox = episodes_listbox(pd, sender.clone()); listbox.ok().map(|l| self.episodes.add(&l)); } - /// Set the show cover. - fn set_cover(&self, pd: &Podcast) { - let img = get_pixbuf_from_path(&pd.clone().into(), 128); - img.map(|i| self.cover.set_from_pixbuf(&i)); + fn set_cover(&self, pd: &Podcast) -> Result<(), Error> { + let image = get_pixbuf_from_path(&pd.clone().into(), 128)?; + self.cover.set_from_pixbuf(&image); + Ok(()) } /// Set the descripton text. From ab519a54d3bcd8d82bc3e0cd49b3a16738a499a3 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 21:32:21 +0200 Subject: [PATCH 06/12] Headerbar: Use Result wherever possible. --- hammond-gtk/src/headerbar.rs | 43 +++++++++++++++++++++++------------- hammond-gtk/src/manager.rs | 3 +++ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/hammond-gtk/src/headerbar.rs b/hammond-gtk/src/headerbar.rs index 6648e46..e8ba764 100644 --- a/hammond-gtk/src/headerbar.rs +++ b/hammond-gtk/src/headerbar.rs @@ -1,9 +1,11 @@ +use failure::Error; +use failure::ResultExt; use gtk; use gtk::prelude::*; +use url::Url; use hammond_data::Source; use hammond_data::dbqueries; -use url::Url; use std::sync::Arc; use std::sync::mpsc::Sender; @@ -55,6 +57,7 @@ impl Default for Header { } } +// TODO: Refactor components into smaller state machines impl Header { pub fn new(content: Arc, window: >k::Window, sender: Sender) -> Header { let h = Header::default(); @@ -72,11 +75,15 @@ impl Header { self.switch.set_stack(&content.get_stack()); new_url.connect_changed(clone!(add_button => move |url| { - on_url_change(url, &result_label, &add_button); + if let Err(err) = on_url_change(url, &result_label, &add_button) { + error!("Error: {}", err); + } })); add_button.connect_clicked(clone!(add_popover, new_url, sender => move |_| { - on_add_bttn_clicked(&new_url, sender.clone()); + if let Err(err) = on_add_bttn_clicked(&new_url, sender.clone()) { + error!("Error: {}", err); + } add_popover.hide(); })); @@ -142,28 +149,32 @@ impl Header { } } -fn on_add_bttn_clicked(entry: >k::Entry, sender: Sender) { +fn on_add_bttn_clicked(entry: >k::Entry, sender: Sender) -> Result<(), Error> { let url = entry.get_text().unwrap_or_default(); - let source = Source::from_url(&url); + let source = Source::from_url(&url).context("Failed to convert url to a Source entry.")?; + entry.set_text(""); - if source.is_ok() { - entry.set_text(""); - sender.send(Action::UpdateSources(source.ok())).unwrap(); - } else { - error!("Something went wrong."); - error!("Error: {:?}", source.unwrap_err()); - } + sender + .send(Action::UpdateSources(Some(source))) + .context("App channel blew up.")?; + Ok(()) } -fn on_url_change(entry: >k::Entry, result: >k::Label, add_button: >k::Button) { - let uri = entry.get_text().unwrap(); +fn on_url_change( + entry: >k::Entry, + result: >k::Label, + add_button: >k::Button, +) -> Result<(), Error> { + let uri = entry + .get_text() + .ok_or_else(|| format_err!("GtkEntry blew up somehow."))?; debug!("Url: {}", uri); let url = Url::parse(&uri); // TODO: refactor to avoid duplication match url { Ok(u) => { - if !dbqueries::source_exists(u.as_str()).unwrap() { + if !dbqueries::source_exists(u.as_str())? { add_button.set_sensitive(true); result.hide(); result.set_label(""); @@ -172,6 +183,7 @@ fn on_url_change(entry: >k::Entry, result: >k::Label, add_button: >k::Butt result.set_label("Show already exists."); result.show(); } + Ok(()) } Err(err) => { add_button.set_sensitive(false); @@ -182,6 +194,7 @@ fn on_url_change(entry: >k::Entry, result: >k::Label, add_button: >k::Butt } else { result.hide(); } + Ok(()) } } } diff --git a/hammond-gtk/src/manager.rs b/hammond-gtk/src/manager.rs index c910889..5805a0d 100644 --- a/hammond-gtk/src/manager.rs +++ b/hammond-gtk/src/manager.rs @@ -11,6 +11,9 @@ use std::sync::mpsc::Sender; // use std::path::PathBuf; use std::thread; +// This is messy, undocumented and hacky af. +// I am terrible at writting downloaders and download managers. + #[derive(Debug)] pub struct Progress { total_bytes: u64, From 1c84f0d7212db59647a8db162a3082f8dcef6468 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 6 Feb 2018 22:03:17 +0200 Subject: [PATCH 07/12] hammond-gtk::manager: Switch the add function to return a Result<(), Error>. --- hammond-gtk/src/manager.rs | 26 +++++++++++++++----------- hammond-gtk/src/widgets/episode.rs | 2 +- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/hammond-gtk/src/manager.rs b/hammond-gtk/src/manager.rs index 5805a0d..da958d2 100644 --- a/hammond-gtk/src/manager.rs +++ b/hammond-gtk/src/manager.rs @@ -1,3 +1,5 @@ +use failure::Error; + // use hammond_data::Episode; use hammond_data::dbqueries; use hammond_downloader::downloader::{get_episode, DownloadProgress}; @@ -76,14 +78,15 @@ lazy_static! { }; } -pub fn add(id: i32, directory: &str, sender: Sender) { +pub fn add(id: i32, directory: &str, sender: Sender) -> Result<(), Error> { // Create a new `Progress` struct to keep track of dl progress. let prog = Arc::new(Mutex::new(Progress::default())); { - if let Ok(mut m) = ACTIVE_DOWNLOADS.write() { - m.insert(id, prog.clone()); - } + let mut m = ACTIVE_DOWNLOADS + .write() + .map_err(|_| format_err!("Failed to get a lock on the mutex."))?; + m.insert(id, prog.clone()); } let dir = directory.to_owned(); @@ -91,12 +94,11 @@ pub fn add(id: i32, directory: &str, sender: Sender) { if let Ok(episode) = dbqueries::get_episode_from_rowid(id) { let pid = episode.podcast_id(); let id = episode.rowid(); - get_episode(&mut episode.into(), dir.as_str(), Some(prog)) - .err() - .map(|err| { - error!("Error while trying to download an episode"); - error!("Error: {}", err); - }); + + if let Err(err) = get_episode(&mut episode.into(), dir.as_str(), Some(prog)) { + error!("Error while trying to download an episode"); + error!("Error: {}", err); + } { if let Ok(mut m) = ACTIVE_DOWNLOADS.write() { @@ -114,6 +116,8 @@ pub fn add(id: i32, directory: &str, sender: Sender) { sender.send(Action::RefreshWidgetIfSame(pid)).unwrap(); } }); + + Ok(()) } #[cfg(test)] @@ -155,7 +159,7 @@ mod tests { let (sender, _rx) = channel(); let download_fold = get_download_folder(&pd.title()).unwrap(); - add(episode.rowid(), download_fold.as_str(), sender); + add(episode.rowid(), download_fold.as_str(), sender).unwrap(); assert_eq!(ACTIVE_DOWNLOADS.read().unwrap().len(), 1); // Give it soem time to download the file diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index ba8ab7c..44bc1ff 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -269,7 +269,7 @@ fn on_download_clicked(ep: &EpisodeWidgetQuery, sender: Sender) -> Resul let download_fold = get_download_folder(&pd.title().to_owned())?; // Start a new download. - manager::add(ep.rowid(), &download_fold, sender.clone()); + manager::add(ep.rowid(), &download_fold, sender.clone())?; // Update Views sender.send(Action::RefreshEpisodesView)?; From 7eb038b899536396179afd2298b976da3314cdc7 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Wed, 7 Feb 2018 00:31:04 +0200 Subject: [PATCH 08/12] EpisodesStack: Refactor update method to return a Result. --- hammond-gtk/src/content.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/hammond-gtk/src/content.rs b/hammond-gtk/src/content.rs index 400659b..48b95ad 100644 --- a/hammond-gtk/src/content.rs +++ b/hammond-gtk/src/content.rs @@ -2,6 +2,8 @@ use gtk; use gtk::Cast; use gtk::prelude::*; +use failure::Error; + use hammond_data::Podcast; use hammond_data::dbqueries; @@ -46,13 +48,17 @@ impl Content { self.update_widget() } + // TODO: Maybe propagate the error? pub fn update_episode_view(&self) { - self.episodes.update(); + if let Err(err) = self.episodes.update() { + error!("Something went wrong while trying to update the episode view."); + error!("Error: {}", err); + } } pub fn update_episode_view_if_baground(&self) { if self.stack.get_visible_child_name() != Some("episodes".into()) { - self.episodes.update(); + self.update_episode_view(); } } @@ -270,24 +276,21 @@ impl EpisodeStack { EpisodeStack { stack, sender } } - pub fn update(&self) { + // Look into refactoring to a state-machine. + pub fn update(&self) -> Result<(), Error> { let old = self.stack .get_child_by_name("episodes") - // This is guaranted to exists, based on `EpisodeStack::new()`. - .unwrap() + .ok_or_else(|| format_err!("Faild to get \"episodes\" child from the stack."))? .downcast::() - // This is guaranted to be a Box based on the `EpisodesView` impl. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a Box."))?; debug!("Name: {:?}", WidgetExt::get_name(&old)); let scrolled_window = old.get_children() .first() - // This is guaranted to exist based on the episodes_view.ui file. - .unwrap() + .ok_or_else(|| format_err!("Box container has no childs."))? .clone() .downcast::() - // This is guaranted based on the episodes_view.ui file. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a ScrolledWindow."))?; debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); let eps = EpisodesView::new(self.sender.clone()); @@ -306,5 +309,7 @@ impl EpisodeStack { } old.destroy(); + + Ok(()) } } From e196a6c905f3b947e08bbdcab830d97231af9d34 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Wed, 7 Feb 2018 00:55:43 +0200 Subject: [PATCH 09/12] ShowStack: update_widget methods now return Result. --- hammond-gtk/src/content.rs | 47 ++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/hammond-gtk/src/content.rs b/hammond-gtk/src/content.rs index 48b95ad..9b923d5 100644 --- a/hammond-gtk/src/content.rs +++ b/hammond-gtk/src/content.rs @@ -67,18 +67,24 @@ impl Content { } pub fn update_widget(&self) { - self.shows.update_widget(); + if let Err(err) = self.shows.update_widget() { + error!("Something went wrong while trying to update the Show Widget."); + error!("Error: {}", err); + } } pub fn update_widget_if_same(&self, pid: i32) { - self.shows.update_widget_if_same(pid); + if let Err(err) = self.shows.update_widget_if_same(pid) { + error!("Something went wrong while trying to update the Show Widget."); + error!("Error: {}", err); + } } pub fn update_widget_if_visible(&self) { if self.stack.get_visible_child_name() == Some("shows".to_string()) && self.shows.get_stack().get_visible_child_name() == Some("widget".to_string()) { - self.shows.update_widget(); + self.update_widget(); } } @@ -209,32 +215,39 @@ impl ShowStack { self.stack.add_named(&new.container, "widget"); } - pub fn update_widget(&self) { - let vis = self.stack.get_visible_child_name().unwrap(); - let old = self.stack.get_child_by_name("widget").unwrap(); + pub fn update_widget(&self) -> Result<(), Error> { + let vis = self.stack + .get_visible_child_name() + .ok_or_else(|| format_err!("Failed to get visible child name."))?; + let old = self.stack + .get_child_by_name("widget") + .ok_or_else(|| format_err!("Faild to get \"widget\" child from the stack."))?; let id = WidgetExt::get_name(&old); if id == Some("GtkBox".to_string()) || id.is_none() { - return; + return Ok(()); } - let pd = dbqueries::get_podcast_from_id(id.unwrap().parse::().unwrap()); - if let Ok(pd) = pd { - self.replace_widget(&pd); - self.stack.set_visible_child_name(&vis); - old.destroy(); - } + let id = id.ok_or_else(|| format_err!("Failed to get widget's name."))?; + let pd = dbqueries::get_podcast_from_id(id.parse::()?)?; + self.replace_widget(&pd); + self.stack.set_visible_child_name(&vis); + old.destroy(); + Ok(()) } // Only update widget if it's podcast_id is equal to pid. - pub fn update_widget_if_same(&self, pid: i32) { - let old = self.stack.get_child_by_name("widget").unwrap(); + pub fn update_widget_if_same(&self, pid: i32) -> Result<(), Error> { + let old = self.stack + .get_child_by_name("widget") + .ok_or_else(|| format_err!("Faild to get \"widget\" child from the stack."))?; let id = WidgetExt::get_name(&old); if id != Some(pid.to_string()) || id.is_none() { - return; + debug!("Different widget. Early return"); + return Ok(()); } - self.update_widget(); + self.update_widget() } pub fn switch_podcasts_animated(&self) { From 89564996df84bb3edcb7b1670a7e34484652d86d Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Wed, 7 Feb 2018 01:19:07 +0200 Subject: [PATCH 10/12] ShowStack: Convert rest methods to return Result. --- hammond-gtk/src/app.rs | 7 ++++++- hammond-gtk/src/content.rs | 37 +++++++++++++++++-------------------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/hammond-gtk/src/app.rs b/hammond-gtk/src/app.rs index fc48072..5a13913 100644 --- a/hammond-gtk/src/app.rs +++ b/hammond-gtk/src/app.rs @@ -134,7 +134,12 @@ impl App { Ok(Action::RefreshWidgetIfSame(id)) => content.update_widget_if_same(id), Ok(Action::RefreshEpisodesView) => content.update_episode_view(), Ok(Action::RefreshEpisodesViewBGR) => content.update_episode_view_if_baground(), - Ok(Action::ReplaceWidget(ref pd)) => content.get_shows().replace_widget(pd), + Ok(Action::ReplaceWidget(ref pd)) => { + if let Err(err) = content.get_shows().replace_widget(pd) { + error!("Something went wrong while trying to update the ShowWidget."); + error!("Error: {}", err); + } + } Ok(Action::ShowWidgetAnimated) => content.get_shows().switch_widget_animated(), Ok(Action::ShowShowsAnimated) => content.get_shows().switch_podcasts_animated(), Ok(Action::HeaderBarShowTile(title)) => headerbar.switch_to_back(&title), diff --git a/hammond-gtk/src/content.rs b/hammond-gtk/src/content.rs index 9b923d5..17811cd 100644 --- a/hammond-gtk/src/content.rs +++ b/hammond-gtk/src/content.rs @@ -63,7 +63,10 @@ impl Content { } pub fn update_shows_view(&self) { - self.shows.update_podcasts(); + if let Err(err) = self.shows.update_podcasts() { + error!("Something went wrong while trying to update the ShowsView."); + error!("Error: {}", err); + } } pub fn update_widget(&self) { @@ -134,26 +137,22 @@ impl ShowStack { // self.update_podcasts(); // } - pub fn update_podcasts(&self) { + pub fn update_podcasts(&self) -> Result<(), Error> { let vis = self.stack.get_visible_child_name().unwrap(); let old = self.stack .get_child_by_name("podcasts") - // This is guaranted to exists, based on `ShowStack::new()`. - .unwrap() + .ok_or_else(|| format_err!("Faild to get \"podcasts\" child from the stack."))? .downcast::() - // This is guaranted to be a Box based on the `ShowsPopulated` impl. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a Box."))?; debug!("Name: {:?}", WidgetExt::get_name(&old)); let scrolled_window = old.get_children() .first() - // This is guaranted to exist based on the show_widget.ui file. - .unwrap() + .ok_or_else(|| format_err!("Box container has no childs."))? .clone() .downcast::() - // This is guaranted based on the show_widget.ui file. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a ScrolledWindow."))?; debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); let pop = ShowsPopulated::new(self.sender.clone()); @@ -174,16 +173,15 @@ impl ShowStack { } old.destroy(); + Ok(()) } - pub fn replace_widget(&self, pd: &Podcast) { + pub fn replace_widget(&self, pd: &Podcast) -> Result<(), Error> { let old = self.stack .get_child_by_name("widget") - // This is guaranted to exists, based on `ShowStack::new()`. - .unwrap() + .ok_or_else(|| format_err!("Faild to get \"widget\" child from the stack."))? .downcast::() - // This is guaranted to be a Box based on the `ShowWidget` impl. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a Box."))?; debug!("Name: {:?}", WidgetExt::get_name(&old)); let new = ShowWidget::new(pd, self.sender.clone()); @@ -197,12 +195,10 @@ impl ShowStack { if newid == oldid { let scrolled_window = old.get_children() .first() - // This is guaranted to exist based on the show_widget.ui file. - .unwrap() + .ok_or_else(|| format_err!("Box container has no childs."))? .clone() .downcast::() - // This is guaranted based on the show_widget.ui file. - .unwrap(); + .map_err(|_| format_err!("Failed to downcast stack child to a ScrolledWindow."))?; debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); // Copy the vertical scrollbar adjustment from the old view into the new one. @@ -213,6 +209,7 @@ impl ShowStack { self.stack.remove(&old); self.stack.add_named(&new.container, "widget"); + Ok(()) } pub fn update_widget(&self) -> Result<(), Error> { @@ -230,7 +227,7 @@ impl ShowStack { let id = id.ok_or_else(|| format_err!("Failed to get widget's name."))?; let pd = dbqueries::get_podcast_from_id(id.parse::()?)?; - self.replace_widget(&pd); + self.replace_widget(&pd)?; self.stack.set_visible_child_name(&vis); old.destroy(); Ok(()) From 2d3360625173b8a191e6a2e72374c5500aa9ee37 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Wed, 7 Feb 2018 03:17:37 +0200 Subject: [PATCH 11/12] Hammond-gtk: Stack, Content constructors return Results now. Constructors now proxy underlying errors that migth occur during initialazation. I think that's about the last unwraps in the main thread. --- hammond-gtk/src/app.rs | 11 +++--- hammond-gtk/src/content.rs | 30 ++++++++-------- hammond-gtk/src/headerbar.rs | 4 ++- hammond-gtk/src/manager.rs | 8 +++-- hammond-gtk/src/utils.rs | 34 ++++++++++++------ hammond-gtk/src/views/episodes.rs | 6 ++-- hammond-gtk/src/views/shows.rs | 60 +++++++++++++++++-------------- 7 files changed, 92 insertions(+), 61 deletions(-) diff --git a/hammond-gtk/src/app.rs b/hammond-gtk/src/app.rs index 5a13913..02edbfa 100644 --- a/hammond-gtk/src/app.rs +++ b/hammond-gtk/src/app.rs @@ -65,7 +65,8 @@ impl App { let (sender, receiver) = channel(); // Create a content instance - let content = Arc::new(Content::new(sender.clone())); + let content = + Arc::new(Content::new(sender.clone()).expect("Content Initialization failed.")); // Create the headerbar let header = Arc::new(Header::new(content.clone(), &window, sender.clone())); @@ -87,7 +88,7 @@ impl App { let sender = self.sender.clone(); // Update the feeds right after the Application is initialized. gtk::timeout_add_seconds(2, move || { - utils::refresh_feed(None, sender.clone()); + utils::refresh_feed_wrapper(None, sender.clone()); glib::Continue(false) }); @@ -95,7 +96,7 @@ impl App { // Auto-updater, runs every hour. // TODO: expose the interval in which it run to a user setting. gtk::timeout_add_seconds(3600, move || { - utils::refresh_feed(None, sender.clone()); + utils::refresh_feed_wrapper(None, sender.clone()); glib::Continue(true) }); @@ -122,9 +123,9 @@ impl App { match receiver.recv_timeout(Duration::from_millis(10)) { Ok(Action::UpdateSources(source)) => { if let Some(s) = source { - utils::refresh_feed(Some(vec![s]), sender.clone()); + utils::refresh_feed_wrapper(Some(vec![s]), sender.clone()); } else { - utils::refresh_feed(None, sender.clone()); + utils::refresh_feed_wrapper(None, sender.clone()); } } Ok(Action::RefreshAllViews) => content.update(), diff --git a/hammond-gtk/src/content.rs b/hammond-gtk/src/content.rs index 17811cd..f73803c 100644 --- a/hammond-gtk/src/content.rs +++ b/hammond-gtk/src/content.rs @@ -26,20 +26,20 @@ pub struct Content { } impl Content { - pub fn new(sender: Sender) -> Content { + pub fn new(sender: Sender) -> Result { let stack = gtk::Stack::new(); - let episodes = Arc::new(EpisodeStack::new(sender.clone())); - let shows = Arc::new(ShowStack::new(sender.clone())); + let episodes = Arc::new(EpisodeStack::new(sender.clone())?); + let shows = Arc::new(ShowStack::new(sender.clone())?); stack.add_titled(&episodes.stack, "episodes", "Episodes"); stack.add_titled(&shows.stack, "shows", "Shows"); - Content { + Ok(Content { stack, shows, episodes, sender, - } + }) } pub fn update(&self) { @@ -107,7 +107,7 @@ pub struct ShowStack { } impl ShowStack { - fn new(sender: Sender) -> ShowStack { + fn new(sender: Sender) -> Result { let stack = gtk::Stack::new(); let show = ShowStack { @@ -115,7 +115,7 @@ impl ShowStack { sender: sender.clone(), }; - let pop = ShowsPopulated::new(sender.clone()); + let pop = ShowsPopulated::new(sender.clone())?; let widget = ShowWidget::default(); let empty = EmptyView::new(); @@ -129,7 +129,7 @@ impl ShowStack { show.stack.set_visible_child_name("podcasts") } - show + Ok(show) } // pub fn update(&self) { @@ -138,7 +138,9 @@ impl ShowStack { // } pub fn update_podcasts(&self) -> Result<(), Error> { - let vis = self.stack.get_visible_child_name().unwrap(); + let vis = self.stack + .get_visible_child_name() + .ok_or_else(|| format_err!("Failed to get visible child name."))?; let old = self.stack .get_child_by_name("podcasts") @@ -155,7 +157,7 @@ impl ShowStack { .map_err(|_| format_err!("Failed to downcast stack child to a ScrolledWindow."))?; debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); - let pop = ShowsPopulated::new(self.sender.clone()); + let pop = ShowsPopulated::new(self.sender.clone())?; // Copy the vertical scrollbar adjustment from the old view into the new one. scrolled_window .get_vadjustment() @@ -269,8 +271,8 @@ pub struct EpisodeStack { } impl EpisodeStack { - fn new(sender: Sender) -> EpisodeStack { - let episodes = EpisodesView::new(sender.clone()); + fn new(sender: Sender) -> Result { + let episodes = EpisodesView::new(sender.clone())?; let empty = EmptyView::new(); let stack = gtk::Stack::new(); @@ -283,7 +285,7 @@ impl EpisodeStack { stack.set_visible_child_name("episodes"); } - EpisodeStack { stack, sender } + Ok(EpisodeStack { stack, sender }) } // Look into refactoring to a state-machine. @@ -303,7 +305,7 @@ impl EpisodeStack { .map_err(|_| format_err!("Failed to downcast stack child to a ScrolledWindow."))?; debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); - let eps = EpisodesView::new(self.sender.clone()); + let eps = EpisodesView::new(self.sender.clone())?; // Copy the vertical scrollbar adjustment from the old view into the new one. scrolled_window .get_vadjustment() diff --git a/hammond-gtk/src/headerbar.rs b/hammond-gtk/src/headerbar.rs index e8ba764..399d3c6 100644 --- a/hammond-gtk/src/headerbar.rs +++ b/hammond-gtk/src/headerbar.rs @@ -90,7 +90,9 @@ impl Header { self.add_toggle.set_popover(&add_popover); self.update_button.connect_clicked(move |_| { - sender.send(Action::UpdateSources(None)).unwrap(); + sender + .send(Action::UpdateSources(None)) + .expect("Action channel blew up."); }); self.about_button diff --git a/hammond-gtk/src/manager.rs b/hammond-gtk/src/manager.rs index da958d2..a9d70fe 100644 --- a/hammond-gtk/src/manager.rs +++ b/hammond-gtk/src/manager.rs @@ -112,8 +112,12 @@ pub fn add(id: i32, directory: &str, sender: Sender) -> Result<(), Error // } // } - sender.send(Action::RefreshEpisodesView).unwrap(); - sender.send(Action::RefreshWidgetIfSame(pid)).unwrap(); + sender + .send(Action::RefreshEpisodesView) + .expect("Action channel blew up."); + sender + .send(Action::RefreshWidgetIfSame(pid)) + .expect("Action channel blew up."); } }); diff --git a/hammond-gtk/src/utils.rs b/hammond-gtk/src/utils.rs index ffac353..1d91f74 100644 --- a/hammond-gtk/src/utils.rs +++ b/hammond-gtk/src/utils.rs @@ -17,14 +17,23 @@ use std::thread; use app::Action; +pub fn refresh_feed_wrapper(source: Option>, sender: Sender) { + if let Err(err) = refresh_feed(source, sender) { + error!("An error occured while trying to update the feeds."); + error!("Error: {}", err); + } +} + /// Update the rss feed(s) originating from `source`. /// If `source` is None, Fetches all the `Source` entries in the database and updates them. /// When It's done,it queues up a `RefreshViews` action. -pub fn refresh_feed(source: Option>, sender: Sender) { - sender.send(Action::HeaderBarShowUpdateIndicator).unwrap(); +fn refresh_feed(source: Option>, sender: Sender) -> Result<(), Error> { + sender.send(Action::HeaderBarShowUpdateIndicator)?; thread::spawn(move || { - let mut sources = source.unwrap_or_else(|| dbqueries::get_sources().unwrap()); + let mut sources = source.unwrap_or_else(|| { + dbqueries::get_sources().expect("Failed to retrieve Sources from the database.") + }); // Work around to improve the feed addition experience. // Many times links to rss feeds are just redirects(usually to an https version). @@ -40,11 +49,11 @@ pub fn refresh_feed(source: Option>, sender: Sender) { if let Err(err) = pipeline::index_single_source(source, false) { error!("Error While trying to update the database."); error!("Error msg: {}", err); - let source = dbqueries::get_source_from_id(id).unwrap(); - - if let Err(err) = pipeline::index_single_source(source, false) { - error!("Error While trying to update the database."); - error!("Error msg: {}", err); + if let Ok(source) = dbqueries::get_source_from_id(id) { + if let Err(err) = pipeline::index_single_source(source, false) { + error!("Error While trying to update the database."); + error!("Error msg: {}", err); + } } } } else { @@ -55,9 +64,14 @@ pub fn refresh_feed(source: Option>, sender: Sender) { } } - sender.send(Action::HeaderBarHideUpdateIndicator).unwrap(); - sender.send(Action::RefreshAllViews).unwrap(); + sender + .send(Action::HeaderBarHideUpdateIndicator) + .expect("Action channel blew up."); + sender + .send(Action::RefreshAllViews) + .expect("Action channel blew up."); }); + Ok(()) } lazy_static! { diff --git a/hammond-gtk/src/views/episodes.rs b/hammond-gtk/src/views/episodes.rs index e4893d6..2321665 100644 --- a/hammond-gtk/src/views/episodes.rs +++ b/hammond-gtk/src/views/episodes.rs @@ -75,9 +75,9 @@ impl Default for EpisodesView { // TODO: REFACTOR ME impl EpisodesView { - pub fn new(sender: Sender) -> EpisodesView { + pub fn new(sender: Sender) -> Result { let view = EpisodesView::default(); - let episodes = dbqueries::get_episodes_widgets_with_limit(50).unwrap(); + let episodes = dbqueries::get_episodes_widgets_with_limit(50)?; let now_utc = Utc::now(); episodes.into_iter().for_each(|mut ep| { @@ -124,7 +124,7 @@ impl EpisodesView { } view.container.show_all(); - view + Ok(view) } pub fn is_empty(&self) -> bool { diff --git a/hammond-gtk/src/views/shows.rs b/hammond-gtk/src/views/shows.rs index 3f72981..52be6a1 100644 --- a/hammond-gtk/src/views/shows.rs +++ b/hammond-gtk/src/views/shows.rs @@ -33,41 +33,34 @@ impl Default for ShowsPopulated { } impl ShowsPopulated { - pub fn new(sender: Sender) -> ShowsPopulated { + pub fn new(sender: Sender) -> Result { let pop = ShowsPopulated::default(); - pop.init(sender); - pop + pop.init(sender)?; + Ok(pop) } - pub fn init(&self, sender: Sender) { - use gtk::WidgetExt; - - // TODO: handle unwraps. + pub fn init(&self, sender: Sender) -> Result<(), Error> { self.flowbox.connect_child_activated(move |_, child| { - // This is such an ugly hack... - let id = WidgetExt::get_name(child).unwrap().parse::().unwrap(); - let pd = dbqueries::get_podcast_from_id(id).unwrap(); - - sender - .send(Action::HeaderBarShowTile(pd.title().into())) - .unwrap(); - sender.send(Action::ReplaceWidget(pd)).unwrap(); - sender.send(Action::ShowWidgetAnimated).unwrap(); + if let Err(err) = on_child_activate(child, sender.clone()) { + error!( + "Something went wrong during flowbox child activation: {}.", + err + ) + }; }); // Populate the flowbox with the Podcasts. - self.populate_flowbox(); + self.populate_flowbox() } - fn populate_flowbox(&self) { - let podcasts = dbqueries::get_podcasts(); + fn populate_flowbox(&self) -> Result<(), Error> { + let podcasts = dbqueries::get_podcasts()?; - if let Ok(pds) = podcasts { - pds.iter().for_each(|parent| { - let flowbox_child = ShowsChild::new(parent); - self.flowbox.add(&flowbox_child.child); - }); - self.flowbox.show_all(); - } + podcasts.iter().for_each(|parent| { + let flowbox_child = ShowsChild::new(parent); + self.flowbox.add(&flowbox_child.child); + }); + self.flowbox.show_all(); + Ok(()) } pub fn is_empty(&self) -> bool { @@ -80,6 +73,21 @@ impl ShowsPopulated { } } +fn on_child_activate(child: >k::FlowBoxChild, sender: Sender) -> Result<(), Error> { + use gtk::WidgetExt; + + // This is such an ugly hack... + let id = WidgetExt::get_name(child) + .ok_or_else(|| format_err!("Faild to get \"episodes\" child from the stack."))? + .parse::()?; + let pd = dbqueries::get_podcast_from_id(id)?; + + sender.send(Action::HeaderBarShowTile(pd.title().into()))?; + sender.send(Action::ReplaceWidget(pd))?; + sender.send(Action::ShowWidgetAnimated)?; + Ok(()) +} + #[derive(Debug)] struct ShowsChild { container: gtk::Box, From d3696fc5ec7ce8c270675d81359a853af243a8fa Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Wed, 7 Feb 2018 03:52:21 +0200 Subject: [PATCH 12/12] Minor Error messages wording changes. --- hammond-gtk/src/app.rs | 2 +- hammond-gtk/src/main.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hammond-gtk/src/app.rs b/hammond-gtk/src/app.rs index 02edbfa..169ac24 100644 --- a/hammond-gtk/src/app.rs +++ b/hammond-gtk/src/app.rs @@ -46,7 +46,7 @@ pub struct App { impl App { pub fn new() -> App { let application = gtk::Application::new("org.gnome.Hammond", ApplicationFlags::empty()) - .expect("Initialization failed..."); + .expect("Application Initialization failed..."); // Weird magic I copy-pasted that sets the Application Name in the Shell. glib::set_application_name("Hammond"); diff --git a/hammond-gtk/src/main.rs b/hammond-gtk/src/main.rs index 67d69c3..eb66a6a 100644 --- a/hammond-gtk/src/main.rs +++ b/hammond-gtk/src/main.rs @@ -65,15 +65,15 @@ use app::App; fn main() { // TODO: make the the logger a cli -vv option - loggerv::init_with_level(Level::Info).unwrap(); - gtk::init().expect("Error initializing gtk"); + loggerv::init_with_level(Level::Info).expect("Error initializing loggerv."); + gtk::init().expect("Error initializing gtk."); static_resource::init().expect("Something went wrong with the resource file initialization."); // Add custom style let provider = gtk::CssProvider::new(); gtk::CssProvider::load_from_resource(&provider, "/org/gnome/hammond/gtk/style.css"); gtk::StyleContext::add_provider_for_screen( - &gdk::Screen::get_default().unwrap(), + &gdk::Screen::get_default().expect("Error initializing gtk css provider."), &provider, 600, );