From fe024502d45f68f051971e4efba46680e9ee8e2b Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Mon, 16 Jul 2018 16:01:32 +0300 Subject: [PATCH 1/6] EpisodeWidget: Pass the EpisodeModel by value. This is preparationg for being able to pass the model to the connect_craw callback directly. --- hammond-gtk/src/widgets/episode.rs | 6 +++--- hammond-gtk/src/widgets/home_view.rs | 4 ++-- hammond-gtk/src/widgets/show.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 720c244..0c3d2ab 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -206,10 +206,10 @@ impl Default for EpisodeWidget { } impl EpisodeWidget { - pub fn new(episode: &EpisodeWidgetModel, sender: &Sender) -> Rc { + pub fn new(episode: EpisodeWidgetModel, sender: &Sender) -> Rc { let widget = Rc::new(Self::default()); - widget.info.init(episode); - Self::determine_buttons_state(&widget, episode, sender) + widget.info.init(&episode); + Self::determine_buttons_state(&widget, &episode, sender) .map_err(|err| error!("Error: {}", err)) .ok(); widget diff --git a/hammond-gtk/src/widgets/home_view.rs b/hammond-gtk/src/widgets/home_view.rs index 584491a..fe892ed 100644 --- a/hammond-gtk/src/widgets/home_view.rs +++ b/hammond-gtk/src/widgets/home_view.rs @@ -95,7 +95,7 @@ impl HomeView { let view_ = view.clone(); let func = move |ep: EpisodeWidgetModel| { let epoch = ep.epoch(); - let widget = HomeEpisode::new(&ep, &sender); + let widget = HomeEpisode::new(ep, &sender); match split(&now_utc, i64::from(epoch)) { Today => add_to_box(&widget, &view_.today_list, &view_.today_box), @@ -197,7 +197,7 @@ impl Default for HomeEpisode { } impl HomeEpisode { - fn new(episode: &EpisodeWidgetModel, sender: &Sender) -> HomeEpisode { + fn new(episode: EpisodeWidgetModel, sender: &Sender) -> HomeEpisode { let builder = gtk::Builder::new_from_resource("/org/gnome/Hammond/gtk/episodes_view_widget.ui"); let container: gtk::Box = builder.get_object("container").unwrap(); diff --git a/hammond-gtk/src/widgets/show.rs b/hammond-gtk/src/widgets/show.rs index e62249e..5159d2f 100644 --- a/hammond-gtk/src/widgets/show.rs +++ b/hammond-gtk/src/widgets/show.rs @@ -206,7 +206,7 @@ fn populate_listbox( let list = show_.episodes.clone(); let constructor = clone!(sender => move |ep| { - EpisodeWidget::new(&ep, &sender).container.clone() + EpisodeWidget::new(ep, &sender).container.clone() }); let callback = clone!(pd, show_ => move || { From 1ab0291483e7a32073ce3a0934110009ca1595e7 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 17 Jul 2018 15:57:52 +0300 Subject: [PATCH 2/6] EpisodeWidget: Only initialize the episode once it's first drawn. --- hammond-gtk/src/widgets/episode.rs | 22 ++++++++++++++++++---- scripts/test.sh | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index 0c3d2ab..bfc8cc3 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -17,6 +17,7 @@ use hammond_data::EpisodeWidgetModel; use app::Action; use manager; +use std::cell::RefCell; use std::rc::Rc; use std::sync::{Arc, Mutex, TryLockError}; @@ -208,10 +209,23 @@ impl Default for EpisodeWidget { impl EpisodeWidget { pub fn new(episode: EpisodeWidgetModel, sender: &Sender) -> Rc { let widget = Rc::new(Self::default()); - widget.info.init(&episode); - Self::determine_buttons_state(&widget, &episode, sender) - .map_err(|err| error!("Error: {}", err)) - .ok(); + let episode = RefCell::new(Some(episode)); + // let weak = Rc::downgrade(widget); + let widget_ = widget.clone(); + let sender = sender.clone(); + widget.container.connect_draw(move |_, _| { + episode.borrow_mut().take().map(|ep| { + // widget.upgrade().map(|widget| { + widget_.info.init(&ep); + // FIXME: THERE IS A REFFERENCE CYCLE HERE + Self::determine_buttons_state(&widget_, &ep, &sender) + .map_err(|err| error!("Error: {}", err)) + .ok(); + // }) + }); + + gtk::Inhibit(false) + }); widget } diff --git a/scripts/test.sh b/scripts/test.sh index 25d1a40..83e3bb8 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -1,4 +1,4 @@ -#!/usr/bin/sh +#! /usr/bin/sh file="/.flatpak-info" if [ -f "$file" ] From 39e6c258d5842a7fb256c4ded856c9f6d9d05b7b Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 17 Jul 2018 16:04:04 +0300 Subject: [PATCH 3/6] ShowsChild: do not initialize the cover until it needs to be drawn. --- hammond-gtk/src/widgets/shows_view.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/hammond-gtk/src/widgets/shows_view.rs b/hammond-gtk/src/widgets/shows_view.rs index 5504a73..59cfef8 100644 --- a/hammond-gtk/src/widgets/shows_view.rs +++ b/hammond-gtk/src/widgets/shows_view.rs @@ -11,6 +11,7 @@ use hammond_data::Show; use app::Action; use utils::{self, get_ignored_shows, lazy_load, set_image_from_path}; +use std::cell::Cell; use std::rc::Rc; use std::sync::Arc; use std::sync::Mutex; @@ -158,11 +159,25 @@ impl ShowsChild { WidgetExt::set_name(&self.child, &pd.id().to_string()); self.set_cover(pd.id()) - .map_err(|err| error!("Failed to set a cover: {}", err)) - .ok(); } - fn set_cover(&self, show_id: i32) -> Result<(), Error> { - set_image_from_path(&self.cover, show_id, 256) + fn set_cover(&self, show_id: i32) { + // The closure above is a regular `Fn` closure. + // which means we can't mutate stuff inside it easily, + // so Cell is used. + // + // `Option` along with the `.take()` method ensure + // that the function will only be run once, during the first execution. + let show_id = Cell::new(Some(show_id)); + + self.cover.connect_draw(move |cover, _| { + show_id.take().map(|id| { + set_image_from_path(cover, id, 256) + .map_err(|err| error!("Failed to set a cover: {}", err)) + .ok(); + }); + + gtk::Inhibit(false) + }); } } From 6036562af2eedefc790266eb987342647dfd4042 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 17 Jul 2018 16:08:20 +0300 Subject: [PATCH 4/6] HomeEpisode: Do not initialize the Image until it needs to be drawn. --- hammond-gtk/src/widgets/home_view.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/hammond-gtk/src/widgets/home_view.rs b/hammond-gtk/src/widgets/home_view.rs index fe892ed..a9bd175 100644 --- a/hammond-gtk/src/widgets/home_view.rs +++ b/hammond-gtk/src/widgets/home_view.rs @@ -13,6 +13,7 @@ use app::Action; use utils::{self, lazy_load_full}; use widgets::EpisodeWidget; +use std::cell::Cell; use std::rc::Rc; use std::sync::Mutex; @@ -216,14 +217,27 @@ impl HomeEpisode { } fn init(&self, show_id: i32) { - self.set_cover(show_id) - .map_err(|err| error!("Failed to set a cover: {}", err)) - .ok(); - + self.set_cover(show_id); self.container.pack_start(&self.episode, true, true, 6); } - fn set_cover(&self, show_id: i32) -> Result<(), Error> { - utils::set_image_from_path(&self.image, show_id, 64) + fn set_cover(&self, show_id: i32) { + // The closure above is a regular `Fn` closure. + // which means we can't mutate stuff inside it easily, + // so Cell is used. + // + // `Option` along with the `.take()` method ensure + // that the function will only be run once, during the first execution. + let show_id = Cell::new(Some(show_id)); + + self.image.connect_draw(move |image, _| { + show_id.take().map(|id| { + utils::set_image_from_path(image, id, 64) + .map_err(|err| error!("Failed to set a cover: {}", err)) + .ok(); + }); + + gtk::Inhibit(false) + }); } } From 5913166a135f6c2ac0d3008b7a9b561225c2a274 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 17 Jul 2018 20:17:25 +0300 Subject: [PATCH 5/6] EpisodeWidget: Avoid Refference Cycles. When passing an Rc into a gtk callback it causes the Rust struct to be kept in memory even if the gtk+ wiget was dropped. Should have been using Weak refferences all along. --- hammond-gtk/src/widgets/episode.rs | 83 ++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/hammond-gtk/src/widgets/episode.rs b/hammond-gtk/src/widgets/episode.rs index bfc8cc3..754ad3e 100644 --- a/hammond-gtk/src/widgets/episode.rs +++ b/hammond-gtk/src/widgets/episode.rs @@ -18,7 +18,7 @@ use app::Action; use manager; use std::cell::RefCell; -use std::rc::Rc; +use std::rc::{Rc, Weak}; use std::sync::{Arc, Mutex, TryLockError}; lazy_static! { @@ -210,22 +210,34 @@ impl EpisodeWidget { pub fn new(episode: EpisodeWidgetModel, sender: &Sender) -> Rc { let widget = Rc::new(Self::default()); let episode = RefCell::new(Some(episode)); - // let weak = Rc::downgrade(widget); - let widget_ = widget.clone(); - let sender = sender.clone(); - widget.container.connect_draw(move |_, _| { + let weak = Rc::downgrade(&widget); + widget.container.connect_draw(clone!(sender => move |_, _| { episode.borrow_mut().take().map(|ep| { - // widget.upgrade().map(|widget| { - widget_.info.init(&ep); - // FIXME: THERE IS A REFFERENCE CYCLE HERE - Self::determine_buttons_state(&widget_, &ep, &sender) + weak.upgrade().map(|w| w.info.init(&ep)); + Self::determine_buttons_state(&weak, &ep, &sender) .map_err(|err| error!("Error: {}", err)) .ok(); - // }) }); gtk::Inhibit(false) + })); + + // When the widget is attached to a parent, + // since it's a rust struct and not a widget the + // compiler drops the refference to it at the end of + // scope. That's cause we only attach the `self.container` + // to the parent. + // + // So this callback keeps a refference to the Rust Struct + // so the compiler won't drop it. + // + // When the widget is detached from it's parent view this + // callback runs freeing the last refference we were holding. + let foo = RefCell::new(Some(widget.clone())); + widget.container.connect_remove(move |_, _| { + foo.borrow_mut().take(); }); + widget } @@ -310,10 +322,13 @@ impl EpisodeWidget { /// | ToDownload | /// ------------------- fn determine_buttons_state( - widget: &Rc, + weak: &Weak, episode: &EpisodeWidgetModel, sender: &Sender, ) -> Result<(), Error> { + let widget = weak + .upgrade() + .ok_or_else(|| format_err!("Widget is already dropped"))?; // Reset the buttons state no matter the glade file. // This is just to make it easier to port to relm in the future. widget.buttons.cancel.hide(); @@ -333,11 +348,11 @@ impl EpisodeWidget { // State: InProggress if let Some(prog) = active_dl()? { // set a callback that will update the state when the download finishes - let callback = clone!(widget, sender => move || { + let callback = clone!(weak, sender => move || { if let Ok(guard) = manager::ACTIVE_DOWNLOADS.read() { if !guard.contains_key(&id) { if let Ok(ep) = dbqueries::get_episode_widget_from_rowid(id) { - Self::determine_buttons_state(&widget, &ep, &sender) + Self::determine_buttons_state(&weak, &ep, &sender) .map_err(|err| error!("Error: {}", err)) .ok(); @@ -354,20 +369,20 @@ impl EpisodeWidget { widget .buttons .cancel - .connect_clicked(clone!(prog, widget, sender => move |_| { + .connect_clicked(clone!(prog, weak, sender => move |_| { // Cancel the download if let Ok(mut m) = prog.lock() { m.cancel(); } // Cancel is not instant so we have to wait a bit - timeout_add(50, clone!(widget, sender => move || { + timeout_add(50, clone!(weak, sender => move || { if let Ok(thing) = active_dl() { if thing.is_none() { // Recalculate the widget state dbqueries::get_episode_widget_from_rowid(id) .map_err(From::from) - .and_then(|ep| Self::determine_buttons_state(&widget, &ep, &sender)) + .and_then(|ep| Self::determine_buttons_state(&weak, &ep, &sender)) .map_err(|err| error!("Error: {}", err)) .ok(); @@ -382,10 +397,10 @@ impl EpisodeWidget { // 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(&widget, &prog); + update_total_size_callback(&weak, &prog); // Setup a callback that will update the progress bar. - update_progressbar_callback(&widget, &prog, id); + update_progressbar_callback(&weak, &prog, id); // Change the widget layout/state widget.state_prog(); @@ -402,9 +417,9 @@ impl EpisodeWidget { widget .buttons .play - .connect_clicked(clone!(widget, sender => move |_| { + .connect_clicked(clone!(weak, sender => move |_| { if let Ok(mut ep) = dbqueries::get_episode_widget_from_rowid(id) { - on_play_bttn_clicked(&widget, &mut ep, &sender) + on_play_bttn_clicked(&weak, &mut ep, &sender) .map_err(|err| error!("Error: {}", err)) .ok(); } @@ -418,14 +433,14 @@ impl EpisodeWidget { widget .buttons .download - .connect_clicked(clone!(widget, sender => move |dl| { + .connect_clicked(clone!(weak, sender => move |dl| { // Make the button insensitive so it won't be pressed twice dl.set_sensitive(false); if let Ok(ep) = dbqueries::get_episode_widget_from_rowid(id) { on_download_clicked(&ep, &sender) .and_then(|_| { info!("Donwload started succesfully."); - Self::determine_buttons_state(&widget, &ep, &sender) + Self::determine_buttons_state(&weak, &ep, &sender) }) .map_err(|err| error!("Error: {}", err)) .ok(); @@ -455,10 +470,14 @@ fn on_download_clicked(ep: &EpisodeWidgetModel, sender: &Sender) -> Resu } fn on_play_bttn_clicked( - widget: &Rc, + widget: &Weak, episode: &mut EpisodeWidgetModel, sender: &Sender, ) -> Result<(), Error> { + let widget = widget + .upgrade() + .ok_or_else(|| format_err!("Widget is already dropped"))?; + // Mark played episode.set_played_now()?; // Grey out the title @@ -475,7 +494,7 @@ fn on_play_bttn_clicked( #[inline] #[cfg_attr(feature = "cargo-clippy", allow(if_same_then_else))] fn update_progressbar_callback( - widget: &Rc, + widget: &Weak, prog: &Arc>, episode_rowid: i32, ) { @@ -488,10 +507,15 @@ fn update_progressbar_callback( #[allow(if_same_then_else)] fn progress_bar_helper( - widget: &Rc, + widget: &Weak, prog: &Arc>, episode_rowid: i32, ) -> Result { + let widget = match widget.upgrade() { + Some(w) => w, + None => return Ok(glib::Continue(false)), + }; + let (fraction, downloaded) = match prog.try_lock() { Ok(guard) => (guard.get_fraction(), guard.get_downloaded()), Err(TryLockError::WouldBlock) => return Ok(glib::Continue(true)), @@ -531,7 +555,7 @@ fn progress_bar_helper( // with the http ContentLength header number rather than // relying to the RSS feed. #[inline] -fn update_total_size_callback(widget: &Rc, prog: &Arc>) { +fn update_total_size_callback(widget: &Weak, prog: &Arc>) { let callback = clone!(prog, widget => move || { total_size_helper(&widget, &prog).unwrap_or(glib::Continue(true)) }); @@ -539,9 +563,14 @@ fn update_total_size_callback(widget: &Rc, prog: &Arc, + widget: &Weak, prog: &Arc>, ) -> Result { + let widget = match widget.upgrade() { + Some(w) => w, + None => return Ok(glib::Continue(false)), + }; + // Get the total_bytes. let total_bytes = match prog.try_lock() { Ok(guard) => guard.get_total_size(), From 7569465a612ee5ef84d0e58f4e1010c8d14080d4 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 17 Jul 2018 20:24:59 +0300 Subject: [PATCH 6/6] App: Remove the imposed delay before refresh_on_startup runs. The application is even lazier now and this is no longer an issue. --- hammond-gtk/src/app.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/hammond-gtk/src/app.rs b/hammond-gtk/src/app.rs index 80e1ae9..3c51f1b 100644 --- a/hammond-gtk/src/app.rs +++ b/hammond-gtk/src/app.rs @@ -156,14 +156,8 @@ impl App { let sender = self.sender.clone(); if self.settings.get_boolean("refresh-on-startup") { info!("Refresh on startup."); - // The ui loads async, after initialization - // so we need to delay this a bit so it won't block - // requests that will come from loading the gui on startup. - gtk::timeout_add(1500, move || { - let s: Option> = None; - utils::refresh(s, sender.clone()); - glib::Continue(false) - }); + let s: Option> = None; + utils::refresh(s, sender.clone()); } }