Merge branch 'fix-ref-cycles' into 'master'

Fix more refference cycles

See merge request World/podcasts!59

podcasts-gtk/src/utils.rs

@@ -1,8 +1,8 @@
 #![cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
 
 use gdk::FrameClockExt;
-use gdk_pixbuf::Pixbuf;
+use gdk_pixbuf::{Object, Pixbuf};
-use glib;
+use glib::{self, object::WeakRef};
 use gtk;
 use gtk::prelude::*;
 use gtk::{IsA, Widget};
@@ -64,16 +64,26 @@ use i18n::i18n;
 /// let list = gtk::ListBox::new();
 /// lazy_load(widgets, list, |w| w, || {});
 /// ```
-pub(crate) fn lazy_load<T, C, F, W, U>(data: T, container: C, mut contructor: F, callback: U)
+pub(crate) fn lazy_load<T, C, F, W, U>(
-where
+    data: T,
+    container: WeakRef<C>,
+    mut contructor: F,
+    callback: U,
+) where
     T: IntoIterator + 'static,
     T::Item: 'static,
-    C: ContainerExt + 'static,
+    // FIXME: leaking a strong refference here
+    C: IsA<Object> + ContainerExt + 'static,
     F: FnMut(T::Item) -> W + 'static,
     W: IsA<Widget> + WidgetExt,
     U: Fn() + 'static,
 {
     let func = move |x| {
+        let container = match container.upgrade() {
+            Some(c) => c,
+            None => return,
+        };
+
         let widget = contructor(x);
         container.add(&widget);
         widget.show();

podcasts-gtk/src/widgets/player.rs

@@ -6,7 +6,7 @@ use gtk;
 use gtk::prelude::*;
 
 use gio::{File, FileExt};
-use glib::SignalHandlerId;
+use glib::{SignalHandlerId, WeakRef};
 
 use chrono::NaiveTime;
 use crossbeam_channel::Sender;
@@ -204,7 +204,8 @@ impl Default for PlayerWidget {
         let separator = builder.get_object("separator").unwrap();
         let slider: gtk::Scale = builder.get_object("seek").unwrap();
         slider.set_range(0.0, 1.0);
-        let slider_update = Rc::new(Self::connect_update_slider(&slider, &player));
+        let player_weak = player.downgrade();
+        let slider_update = Rc::new(Self::connect_update_slider(&slider, player_weak));
         let timer = PlayerTimes {
             container: timer_container,
             progressed,
@@ -385,11 +386,19 @@ impl PlayerWidget {
         Ok(())
     }
 
-    fn connect_update_slider(slider: &gtk::Scale, player: &gst_player::Player) -> SignalHandlerId {
+    fn connect_update_slider(
-        slider.connect_value_changed(clone!(player => move |slider| {
+        slider: &gtk::Scale,
+        player: WeakRef<gst_player::Player>,
+    ) -> SignalHandlerId {
+        slider.connect_value_changed(move |slider| {
+            let player = match player.upgrade() {
+                Some(p) => p,
+                None => return,
+            };
+
             let value = slider.get_value() as u64;
             player.seek(ClockTime::from_seconds(value));
-        }))
+        })
     }
 }
 

podcasts-gtk/src/widgets/show.rs

@@ -1,7 +1,7 @@
 use glib;
 use gtk::{self, prelude::*, Adjustment};
 
-use crossbeam_channel::Sender;
+use crossbeam_channel::{bounded, Sender};
 use failure::Error;
 use fragile::Fragile;
 use html2text;
@@ -103,14 +103,11 @@ impl ShowWidget {
 
 /// Populate the listbox with the shows episodes.
 fn populate_listbox(
-    // FIXME: we are leaking strong refs here
     show: &Rc<ShowWidget>,
     pd: Arc<Show>,
     sender: Sender<Action>,
     vadj: Option<Adjustment>,
 ) -> Result<(), Error> {
-    use crossbeam_channel::bounded;
-
     let count = dbqueries::get_pd_episodes_count(&pd)?;
 
     let (sender_, receiver) = bounded(1);
@@ -128,7 +125,8 @@ fn populate_listbox(
         return Ok(());
     }
 
-    let show_ = show.clone();
+    let show_weak = Rc::downgrade(&show);
+    let list_weak = show.episodes.downgrade();
     gtk::idle_add(move || {
         let episodes = match receiver.try_recv() {
             Some(e) => e,
@@ -136,18 +134,18 @@ fn populate_listbox(
         };
         debug_assert!(episodes.len() as i64 == count);
 
-        let list = show_.episodes.clone();
         let constructor = clone!(sender => move |ep| {
             EpisodeWidget::new(ep, &sender).container.clone()
         });
 
-        let callback = clone!(show_, vadj => move || {
+        let callback = clone!(show_weak, vadj => move || {
-            if let Some(ref v) = vadj {
+            match (show_weak.upgrade(), &vadj) {
-                show_.view.set_adjutments(None, Some(v))
+                (Some(ref shows), Some(ref v)) => shows.view.set_adjutments(None, Some(v)),
+                _ => (),
             };
         });
 
-        lazy_load(episodes, list.clone(), constructor, callback);
+        lazy_load(episodes, list_weak.clone(), constructor, callback);
 
         glib::Continue(false)
     });

podcasts-gtk/src/widgets/show_menu.rs

@@ -68,8 +68,13 @@ impl ShowMenu {
     }
 
     fn connect_played(&self, pd: &Arc<Show>, episodes: &gtk::ListBox, sender: &Sender<Action>) {
-        self.played
+        let episodes_weak = episodes.downgrade();
-            .connect_clicked(clone!(pd, episodes, sender => move |_| {
+        self.played.connect_clicked(clone!(pd, sender => move |_| {
+            let episodes = match episodes_weak.upgrade() {
+                Some(e) => e,
+                None => return,
+            };
+
             let res = dim_titles(&episodes);
             debug_assert!(res.is_some());
 
@@ -121,6 +126,7 @@ fn dim_titles(episodes: &gtk::ListBox) -> Option<()> {
 }
 
 fn mark_all_watched(pd: &Show, sender: &Sender<Action>) -> Result<(), Error> {
+    // TODO: If this fails for whatever reason, it should be impossible, show an error
     dbqueries::update_none_to_played_now(pd)?;
     // Not all widgets might have been loaded when the mark_all is hit
     // So we will need to refresh again after it's done.

podcasts-gtk/src/widgets/shows_view.rs

@@ -64,17 +64,18 @@ impl ShowsView {
 fn populate_flowbox(shows: &Rc<ShowsView>, vadj: Option<Adjustment>) -> Result<(), Error> {
     let ignore = get_ignored_shows()?;
     let podcasts = dbqueries::get_podcasts_filter(&ignore)?;
+    let show_weak = Rc::downgrade(&shows);
+    let flowbox_weak = shows.flowbox.downgrade();
 
     let constructor = move |parent| ShowsChild::new(&parent).child;
-    // FIXME: We are, possibly,leaking the strong ref here
+    let callback = move || {
-    let callback = clone!(shows => move || {
+        match (show_weak.upgrade(), &vadj) {
-        if let Some(ref v) = vadj {
+            (Some(ref shows), Some(ref v)) => shows.view.set_adjutments(None, Some(v)),
-            shows.view.set_adjutments(None, Some(v))
+            _ => (),
         };
-     });
+    };
 
-    let flowbox = shows.flowbox.clone();
+    lazy_load(podcasts, flowbox_weak, constructor, callback);
-    lazy_load(podcasts, flowbox, constructor, callback);
     Ok(())
 }