From bb5c25d87fbb7a378c092d7514142dcda345961d Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sat, 6 Jan 2018 03:09:24 +0200 Subject: [PATCH 1/8] Write unit tests for the itunes_duration_extension parser. --- hammond-data/src/parser.rs | 62 +++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/hammond-data/src/parser.rs b/hammond-data/src/parser.rs index 0bfa5e9..9f9299b 100644 --- a/hammond-data/src/parser.rs +++ b/hammond-data/src/parser.rs @@ -123,10 +123,70 @@ fn parse_itunes_duration(item: &Item) -> Option { mod tests { use std::fs::File; use std::io::BufReader; - use rss::Channel; + use rss; use super::*; + #[test] + fn test_itunes_duration() { + use rss::extension::itunes::ITunesItemExtensionBuilder; + + // Input is a String + let extension = ITunesItemExtensionBuilder::default() + .duration(Some("3370".into())) + .build() + .unwrap(); + let item = rss::ItemBuilder::default() + .itunes_ext(Some(extension)) + .build() + .unwrap(); + assert_eq!(parse_itunes_duration(&item), Some(3370)); + + // Input is a String + let extension = ITunesItemExtensionBuilder::default() + .duration(Some("6:10".into())) + .build() + .unwrap(); + let item = rss::ItemBuilder::default() + .itunes_ext(Some(extension)) + .build() + .unwrap(); + assert_eq!(parse_itunes_duration(&item), Some(370)); + + // Input is a String + let extension = ITunesItemExtensionBuilder::default() + .duration(Some("56:10".into())) + .build() + .unwrap(); + let item = rss::ItemBuilder::default() + .itunes_ext(Some(extension)) + .build() + .unwrap(); + assert_eq!(parse_itunes_duration(&item), Some(3370)); + + // Input is a String + let extension = ITunesItemExtensionBuilder::default() + .duration(Some("1:56:10".into())) + .build() + .unwrap(); + let item = rss::ItemBuilder::default() + .itunes_ext(Some(extension)) + .build() + .unwrap(); + assert_eq!(parse_itunes_duration(&item), Some(6970)); + + // Input is a String + let extension = ITunesItemExtensionBuilder::default() + .duration(Some("01:56:10".into())) + .build() + .unwrap(); + let item = rss::ItemBuilder::default() + .itunes_ext(Some(extension)) + .build() + .unwrap(); + assert_eq!(parse_itunes_duration(&item), Some(6970)); + } + #[test] fn test_new_podcast_intercepted() { let file = File::open("tests/feeds/Intercepted.xml").unwrap(); From 5fed283ff407a474ff9825fbbff7de7072a9f47b Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sat, 6 Jan 2018 03:18:28 +0200 Subject: [PATCH 2/8] EpisodeWidget: Hide duration label if its equal to 0. --- hammond-gtk/src/widgets/episode.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 623ab2f..51f7677 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -160,6 +160,10 @@ impl EpisodeWidget { /// Set the duration label. fn set_duration(&self, seconds: Option) { + if (seconds == Some(0)) || seconds.is_none() { + return; + }; + if let Some(secs) = seconds { self.duration.set_text(&format!("{} min", secs / 60)); self.duration.show(); From e961d5f8b04e6f8f203490625fe2b17f9f3cfdef Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sat, 6 Jan 2018 03:49:26 +0200 Subject: [PATCH 3/8] Use lazystatic to cache the current chrono date. This will backfire on every new year's eve. --- hammond-gtk/src/widgets/episode.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 51f7677..4651f1d 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -9,7 +9,6 @@ use humansize::{file_size_opts as size_opts, FileSize}; use hammond_data::dbqueries; use hammond_data::{EpisodeWidgetQuery, Podcast}; -// use hammond_data::utils::*; use hammond_data::errors::*; use hammond_downloader::downloader; @@ -72,6 +71,10 @@ impl Default for EpisodeWidget { } } +lazy_static! { + static ref NOW: DateTime = Utc::now(); +} + impl EpisodeWidget { pub fn new(episode: &mut EpisodeWidgetQuery, sender: Sender) -> EpisodeWidget { let widget = EpisodeWidget::default(); @@ -148,9 +151,8 @@ impl EpisodeWidget { /// Set the date label depending on the current time. fn set_date(&self, epoch: i32) { - let now = Utc::now(); let date = Utc.timestamp(i64::from(epoch), 0); - if now.year() == date.year() { + if NOW.year() == date.year() { self.date.set_text(&date.format("%e %b").to_string().trim()); } else { self.date @@ -173,6 +175,10 @@ impl EpisodeWidget { /// Set the Episode label dependings on its size fn set_size(&self, bytes: Option) { + if (bytes == Some(0)) || bytes.is_none() { + return; + }; + // Declare a custom humansize option struct // See: https://docs.rs/humansize/1.0.2/humansize/file_size_opts/struct.FileSizeOpts.html let custom_options = size_opts::FileSizeOpts { @@ -188,13 +194,11 @@ impl EpisodeWidget { }; if let Some(size) = bytes { - if size != 0 { - let s = size.file_size(custom_options); - if let Ok(s) = s { - self.size.set_text(&s); - self.size.show(); - self.separator2.show(); - } + let s = size.file_size(custom_options); + if let Ok(s) = s { + self.size.set_text(&s); + self.size.show(); + self.separator2.show(); } }; } From 4a6a9517f1eb127eb6282245ab90abc2bdc8b67c Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sat, 6 Jan 2018 05:38:31 +0200 Subject: [PATCH 4/8] ShowStack: Copy the scrollbar position only if both widget represent the same podcast. --- hammond-gtk/src/content.rs | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/hammond-gtk/src/content.rs b/hammond-gtk/src/content.rs index 0480512..9a3861d 100644 --- a/hammond-gtk/src/content.rs +++ b/hammond-gtk/src/content.rs @@ -161,21 +161,30 @@ impl ShowStack { .unwrap(); 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() - .clone() - .downcast::() - // This is guaranted based on the show_widget.ui file. - .unwrap(); - debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); - let new = ShowWidget::new(Arc::new(self.clone()), pd, self.sender.clone()); - // Copy the vertical scrollbar adjustment from the old view into the new one. - scrolled_window - .get_vadjustment() - .map(|x| new.set_vadjustment(&x)); + // Each composite ShowWidget is a gtkBox with the Podcast.id encoded in the gtk::Widget + // name. It's a hack since we can't yet subclass GObject easily. + let oldid = WidgetExt::get_name(&old); + let newid = WidgetExt::get_name(&new.container); + debug!("Old widget Name: {:?}\nNew widget Name: {:?}", oldid, newid); + + // Only copy the old scrollbar if both widget's represent the same podcast. + if newid == oldid { + let scrolled_window = old.get_children() + .first() + // This is guaranted to exist based on the show_widget.ui file. + .unwrap() + .clone() + .downcast::() + // This is guaranted based on the show_widget.ui file. + .unwrap(); + debug!("Name: {:?}", WidgetExt::get_name(&scrolled_window)); + + // Copy the vertical scrollbar adjustment from the old view into the new one. + scrolled_window + .get_vadjustment() + .map(|x| new.set_vadjustment(&x)); + } self.stack.remove(&old); self.stack.add_named(&new.container, "widget"); From e290ae223e3d48bf24e3b4f95fa1ef05d2ebd9a4 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sun, 7 Jan 2018 07:49:16 +0200 Subject: [PATCH 5/8] Pixbuf cache: use rwlock where possible. --- hammond-gtk/src/utils.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hammond-gtk/src/utils.rs b/hammond-gtk/src/utils.rs index 4928abc..34370a2 100644 --- a/hammond-gtk/src/utils.rs +++ b/hammond-gtk/src/utils.rs @@ -7,7 +7,7 @@ use hammond_downloader::downloader; use std::thread; use std::sync::mpsc::Sender; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, RwLock}; use std::collections::HashMap; use headerbar::Header; @@ -35,8 +35,8 @@ pub fn refresh_feed(headerbar: Arc
, source: Option>, sender: } lazy_static! { - static ref CACHED_PIXBUFS: Mutex>>> = { - Mutex::new(HashMap::new()) + static ref CACHED_PIXBUFS: RwLock>>> = { + RwLock::new(HashMap::new()) }; } @@ -48,8 +48,8 @@ lazy_static! { // 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 { - let mut hashmap = CACHED_PIXBUFS.lock().unwrap(); { + let hashmap = CACHED_PIXBUFS.read().unwrap(); let res = hashmap.get(&(pd.id(), size)); if let Some(px) = res { let m = px.lock().unwrap(); @@ -60,6 +60,7 @@ pub fn get_pixbuf_from_path(pd: &PodcastCoverQuery, size: u32) -> Option let img_path = downloader::cache_image(pd)?; 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); } From 074284d2864561faf0f3cfce7c7aa3fd25768a1c Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sun, 7 Jan 2018 08:26:38 +0200 Subject: [PATCH 6/8] Move unsub show logic and get_download_folder func to hammond-data::utils. --- hammond-data/src/utils.rs | 46 ++++++++++++++++++++++++++-- hammond-downloader/src/downloader.rs | 20 +----------- hammond-gtk/src/widgets/episode.rs | 3 +- hammond-gtk/src/widgets/show.rs | 18 ++--------- 4 files changed, 49 insertions(+), 38 deletions(-) diff --git a/hammond-data/src/utils.rs b/hammond-data/src/utils.rs index fd0224b..c9a55a6 100644 --- a/hammond-data/src/utils.rs +++ b/hammond-data/src/utils.rs @@ -8,11 +8,13 @@ use itertools::Itertools; use errors::*; use dbqueries; -use models::queryables::EpisodeCleanerQuery; +use models::queryables::{EpisodeCleanerQuery, Podcast}; +use xdg_dirs::DL_DIR; use std::path::Path; use std::fs; +/// Scan downloaded `episode` entries that might have broken `local_uri`s and set them to `None`. fn download_checker() -> Result<()> { let episodes = dbqueries::get_downloaded_episodes()?; @@ -30,6 +32,7 @@ fn download_checker() -> Result<()> { Ok(()) } +/// Delete watched `episodes` that have exceded their liftime after played. fn played_cleaner() -> Result<()> { let mut episodes = dbqueries::get_played_cleaner_episodes()?; @@ -54,7 +57,7 @@ fn played_cleaner() -> Result<()> { } /// Check `ep.local_uri` field and delete the file it points to. -pub fn delete_local_content(ep: &mut EpisodeCleanerQuery) -> Result<()> { +fn delete_local_content(ep: &mut EpisodeCleanerQuery) -> Result<()> { if ep.local_uri().is_some() { let uri = ep.local_uri().unwrap().to_owned(); if Path::new(&uri).exists() { @@ -119,6 +122,38 @@ pub fn replace_extra_spaces(s: &str) -> String { .collect::() } +/// Returns the URI of a Podcast Downloads given it's title. +pub fn get_download_folder(pd_title: &str) -> Result { + // It might be better to make it a hash of the title or the podcast rowid + let download_fold = format!("{}/{}", DL_DIR.to_str().unwrap(), pd_title); + + // Create the folder + fs::DirBuilder::new() + .recursive(true) + .create(&download_fold)?; + Ok(download_fold) +} + +/// Removes all the entries associated with the given show from the database, +/// and deletes all of the downloaded content. +/// TODO: Write Tests +/// TODO: Return Result instead +pub fn delete_show(pd: &Podcast) { + let res = dbqueries::remove_feed(&pd); + if res.is_ok() { + info!("{} was removed succesfully.", pd.title()); + + let dl_fold = get_download_folder(pd.title()); + if let Ok(fold) = dl_fold { + let res3 = fs::remove_dir_all(&fold); + // TODO: Show errors? + if res3.is_ok() { + info!("All the content at, {} was removed succesfully", &fold); + } + }; + } +} + #[cfg(test)] mod tests { extern crate tempdir; @@ -277,4 +312,11 @@ mod tests { assert_eq!(replace_extra_spaces(&bad_txt), valid_txt); } + + #[test] + fn test_get_dl_folder() { + let foo_ = format!("{}/{}", DL_DIR.to_str().unwrap(), "foo"); + assert_eq!(get_download_folder("foo").unwrap(), foo_); + let _ = fs::remove_dir_all(foo_); + } } diff --git a/hammond-downloader/src/downloader.rs b/hammond-downloader/src/downloader.rs index 7df62b6..83521e7 100644 --- a/hammond-downloader/src/downloader.rs +++ b/hammond-downloader/src/downloader.rs @@ -10,7 +10,7 @@ use std::fs; use errors::*; use hammond_data::{EpisodeWidgetQuery, PodcastCoverQuery}; -use hammond_data::xdg_dirs::{DL_DIR, HAMMOND_CACHE}; +use hammond_data::xdg_dirs::HAMMOND_CACHE; // TODO: Replace path that are of type &str with std::path. // TODO: Have a convention/document absolute/relative paths, if they should end with / or not. @@ -97,15 +97,6 @@ fn save_io(file: &str, resp: &mut reqwest::Response, content_lenght: Option Ok(()) } -pub fn get_download_folder(pd_title: &str) -> Result { - // It might be better to make it a hash of the title - let download_fold = format!("{}/{}", DL_DIR.to_str().unwrap(), pd_title); - - // Create the folder - DirBuilder::new().recursive(true).create(&download_fold)?; - Ok(download_fold) -} - // TODO: Refactor pub fn get_episode(ep: &mut EpisodeWidgetQuery, download_folder: &str) -> Result<()> { // Check if its alrdy downloaded @@ -192,15 +183,6 @@ mod tests { use hammond_data::dbqueries; use diesel::associations::Identifiable; - use std::fs; - - #[test] - fn test_get_dl_folder() { - let foo_ = format!("{}/{}", DL_DIR.to_str().unwrap(), "foo"); - assert_eq!(get_download_folder("foo").unwrap(), foo_); - let _ = fs::remove_dir_all(foo_); - } - #[test] // This test inserts an rss feed to your `XDG_DATA/hammond/hammond.db` so we make it explicit // to run it. diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 4651f1d..bcb4061 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -9,6 +9,7 @@ use humansize::{file_size_opts as size_opts, FileSize}; use hammond_data::dbqueries; use hammond_data::{EpisodeWidgetQuery, Podcast}; +use hammond_data::utils::get_download_folder; use hammond_data::errors::*; use hammond_downloader::downloader; @@ -227,7 +228,7 @@ fn on_download_clicked( download_bttn.hide(); sender.send(Action::RefreshEpisodesViewBGR).unwrap(); thread::spawn(move || { - let download_fold = downloader::get_download_folder(&pd_title).unwrap(); + let download_fold = get_download_folder(&pd_title).unwrap(); let e = downloader::get_episode(&mut ep, download_fold.as_str()); if let Err(err) = e { error!("Error while trying to download: {:?}", ep.uri()); diff --git a/hammond-gtk/src/widgets/show.rs b/hammond-gtk/src/widgets/show.rs index 37a571e..5a0c5f8 100644 --- a/hammond-gtk/src/widgets/show.rs +++ b/hammond-gtk/src/widgets/show.rs @@ -6,8 +6,7 @@ use dissolve; use hammond_data::dbqueries; use hammond_data::Podcast; -use hammond_data::utils::replace_extra_spaces; -use hammond_downloader::downloader; +use hammond_data::utils::{delete_show, replace_extra_spaces}; use widgets::episode::episodes_listbox; use utils::get_pixbuf_from_path; @@ -17,7 +16,6 @@ use app::Action; use std::sync::mpsc::Sender; use std::sync::Arc; use std::thread; -use std::fs; #[derive(Debug, Clone)] pub struct ShowWidget { @@ -126,19 +124,7 @@ fn on_unsub_button_clicked( unsub_button.hide(); // Spawn a thread so it won't block the ui. thread::spawn(clone!(pd => move || { - let res = dbqueries::remove_feed(&pd); - if res.is_ok() { - info!("{} was removed succesfully.", pd.title()); - - let dl_fold = downloader::get_download_folder(pd.title()); - if let Ok(fold) = dl_fold { - let res3 = fs::remove_dir_all(&fold); - // TODO: Show errors? - if res3.is_ok() { - info!("All the content at, {} was removed succesfully", &fold); - } - }; - } + delete_show(&pd) })); shows.switch_podcasts_animated(); // Queue a refresh after the switch to avoid blocking the db. From 81b1ec810cdec2b5311ee10c53e6c7a53193635a Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sun, 7 Jan 2018 08:36:02 +0200 Subject: [PATCH 7/8] Apply clippy suggestions. --- hammond-data/src/utils.rs | 2 +- hammond-gtk/src/utils.rs | 8 +++----- hammond-gtk/src/widgets/episode.rs | 4 ++-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/hammond-data/src/utils.rs b/hammond-data/src/utils.rs index c9a55a6..1e7f4c2 100644 --- a/hammond-data/src/utils.rs +++ b/hammond-data/src/utils.rs @@ -139,7 +139,7 @@ pub fn get_download_folder(pd_title: &str) -> Result { /// TODO: Write Tests /// TODO: Return Result instead pub fn delete_show(pd: &Podcast) { - let res = dbqueries::remove_feed(&pd); + let res = dbqueries::remove_feed(pd); if res.is_ok() { info!("{} was removed succesfully.", pd.title()); diff --git a/hammond-gtk/src/utils.rs b/hammond-gtk/src/utils.rs index 34370a2..849eba6 100644 --- a/hammond-gtk/src/utils.rs +++ b/hammond-gtk/src/utils.rs @@ -22,11 +22,9 @@ pub fn refresh_feed(headerbar: Arc
, source: Option>, sender: thread::spawn(move || { if let Some(s) = source { feed::index_loop(s); - } else { - if let Err(err) = feed::index_all() { - error!("Error While trying to update the database."); - error!("Error msg: {}", err); - } + } else if let Err(err) = feed::index_all() { + error!("Error While trying to update the database."); + error!("Error msg: {}", err); }; sender.send(Action::HeaderBarHideUpdateIndicator).unwrap(); diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index bcb4061..c8f061a 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -154,10 +154,10 @@ impl EpisodeWidget { fn set_date(&self, epoch: i32) { let date = Utc.timestamp(i64::from(epoch), 0); if NOW.year() == date.year() { - self.date.set_text(&date.format("%e %b").to_string().trim()); + self.date.set_text(date.format("%e %b").to_string().trim()); } else { self.date - .set_text(&date.format("%e %b %Y").to_string().trim()); + .set_text(date.format("%e %b %Y").to_string().trim()); }; } From 345d4b3865f5ab0cc58e8382b18bf9fa1cbfa630 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Mon, 8 Jan 2018 02:41:37 +0200 Subject: [PATCH 8/8] Check more http status codes when parsing feeds. --- hammond-data/src/models/queryables.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/hammond-data/src/models/queryables.rs b/hammond-data/src/models/queryables.rs index d1a9cc0..35b342e 100644 --- a/hammond-data/src/models/queryables.rs +++ b/hammond-data/src/models/queryables.rs @@ -630,6 +630,7 @@ impl<'a> Source { // TODO: Refactor into TryInto once it lands on stable. pub fn into_feed(mut self, ignore_etags: bool) -> Result { use reqwest::header::{EntityTag, Headers, HttpDate, IfModifiedSince, IfNoneMatch}; + use reqwest::StatusCode; let mut headers = Headers::new(); @@ -655,12 +656,26 @@ impl<'a> Source { self.update_etag(&req)?; // TODO match on more stuff - // 301: Permanent redirect of the url - // 302: Temporary redirect of the url + // 301: Moved Permanently // 304: Up to date Feed, checked with the Etag + // 307: Temporary redirect of the url + // 308: Permanent redirect of the url + // 401: Unathorized + // 403: Forbidden + // 408: Timeout // 410: Feed deleted match req.status() { - reqwest::StatusCode::NotModified => bail!("304, skipping.."), + StatusCode::NotModified => bail!("304: skipping.."), + StatusCode::TemporaryRedirect => debug!("307: Temporary Redirect."), + // TODO: Change the source uri to the new one + StatusCode::MovedPermanently | StatusCode::PermanentRedirect => { + warn!("Feed was moved permanently.") + } + StatusCode::Unauthorized => bail!("401: Unauthorized."), + StatusCode::Forbidden => bail!("403: Forbidden."), + StatusCode::NotFound => bail!("404: Not found."), + StatusCode::RequestTimeout => bail!("408: Request Timeout."), + StatusCode::Gone => bail!("410: Feed was deleted."), _ => (), };