fix: a bare channel row no longer outlives everything else
Removing every subscription left one channel behind — the parent of a video saved on its own from the menu bar. That row is deliberately not a subscription, so a replace spared it; but with its video no longer on disk it was pure residue, and confusing residue: a channel absent from the sidebar whose video still sat in the feed after everything else had gone. A replace now sweeps bare rows that have nothing downloaded and nothing downloading. One whose video is still on disk survives, because that is a deliberate save and not this function's to delete. The preview counts the sweep too. It said "0 channels" while it would have taken one, and a confirmation that understates itself is worse than no confirmation. Both the count and the sweep read from one definition of what an orphan is, so they cannot drift apart. Also restores the step that brings the browser to the front before scraping, which was lost in an edit that failed halfway. Without it a background tab has its DOM discarded and reports its address while holding nothing — indistinguishable from an empty subscription list.
This commit is contained in:
+180
-10
@@ -55,6 +55,22 @@ CREATE TABLE IF NOT EXISTS downloads (
|
|||||||
);
|
);
|
||||||
"#;
|
"#;
|
||||||
|
|
||||||
|
/// A channel row that is not a subscription and has no video on disk or on its
|
||||||
|
/// way there. Written once and used by both the count and the sweep, so the
|
||||||
|
/// confirmation can never disagree with what happens.
|
||||||
|
const ORPHAN_WHERE: &str = "c.subscribed = 0 AND NOT EXISTS (
|
||||||
|
SELECT 1 FROM videos v JOIN downloads d ON d.video_id = v.id
|
||||||
|
WHERE v.channel_id = c.id AND d.state IN ('done','queued','running'))";
|
||||||
|
|
||||||
|
const ORPHAN_COUNT_SQL: &str = "SELECT COUNT(*) FROM channels c WHERE c.subscribed = 0
|
||||||
|
AND NOT EXISTS (SELECT 1 FROM videos v JOIN downloads d ON d.video_id = v.id
|
||||||
|
WHERE v.channel_id = c.id AND d.state IN ('done','queued','running'))";
|
||||||
|
|
||||||
|
const ORPHAN_VIDEO_COUNT_SQL: &str = "SELECT COUNT(*) FROM videos v WHERE v.channel_id IN (
|
||||||
|
SELECT c.id FROM channels c WHERE c.subscribed = 0
|
||||||
|
AND NOT EXISTS (SELECT 1 FROM videos v2 JOIN downloads d ON d.video_id = v2.id
|
||||||
|
WHERE v2.channel_id = c.id AND d.state IN ('done','queued','running')))";
|
||||||
|
|
||||||
fn now() -> i64 {
|
fn now() -> i64 {
|
||||||
std::time::SystemTime::now()
|
std::time::SystemTime::now()
|
||||||
.duration_since(std::time::UNIX_EPOCH)
|
.duration_since(std::time::UNIX_EPOCH)
|
||||||
@@ -214,10 +230,13 @@ impl Db {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Bare rows that the replace would sweep count as removals too: saying
|
||||||
|
// "0 channels" while quietly taking one is worse than taking it.
|
||||||
|
let (orphan_channels, orphan_videos) = self.orphan_counts()?;
|
||||||
Ok(ImportPreview {
|
Ok(ImportPreview {
|
||||||
incoming: incoming.len() as i64,
|
incoming: incoming.len() as i64,
|
||||||
removed_channels,
|
removed_channels: removed_channels + orphan_channels,
|
||||||
removed_videos,
|
removed_videos: removed_videos + orphan_videos,
|
||||||
removed_downloads,
|
removed_downloads,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
@@ -306,6 +325,9 @@ impl Db {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
tx.commit().map_err(|e| e.to_string())?;
|
tx.commit().map_err(|e| e.to_string())?;
|
||||||
|
// A replace is the moment to notice that a bare row has outlived its
|
||||||
|
// video.
|
||||||
|
self.prune_orphan_channels()?;
|
||||||
Ok(channels.len())
|
Ok(channels.len())
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -356,6 +378,18 @@ impl Db {
|
|||||||
tx.commit().map_err(|e| e.to_string())
|
tx.commit().map_err(|e| e.to_string())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// True when a channel row exists at all, subscription or not.
|
||||||
|
pub fn has_any_channel(&self, id: &str) -> Result<bool, String> {
|
||||||
|
self.conn
|
||||||
|
.query_row("SELECT 1 FROM channels WHERE id = ?1", params![id], |_| Ok(()))
|
||||||
|
.map(|_| true)
|
||||||
|
.or_else(|e| match e {
|
||||||
|
rusqlite::Error::QueryReturnedNoRows => Ok(false),
|
||||||
|
other => Err(other),
|
||||||
|
})
|
||||||
|
.map_err(|e| e.to_string())
|
||||||
|
}
|
||||||
|
|
||||||
/// True when a channel is already a subscription. A bare row saved for a
|
/// True when a channel is already a subscription. A bare row saved for a
|
||||||
/// one-off video does not count: subscribing to it is a real change.
|
/// one-off video does not count: subscribing to it is a real change.
|
||||||
pub fn is_subscribed(&self, id: &str) -> Result<bool, String> {
|
pub fn is_subscribed(&self, id: &str) -> Result<bool, String> {
|
||||||
@@ -447,6 +481,49 @@ impl Db {
|
|||||||
/// Upserts video metadata only. Deliberately never touches the `downloads`
|
/// Upserts video metadata only. Deliberately never touches the `downloads`
|
||||||
/// table, so a refresh cannot lose the record of what has been downloaded.
|
/// table, so a refresh cannot lose the record of what has been downloaded.
|
||||||
/// `thumb_path` is preserved via COALESCE for the same reason.
|
/// `thumb_path` is preserved via COALESCE for the same reason.
|
||||||
|
/// How many bare rows, and their videos, a replace would sweep.
|
||||||
|
fn orphan_counts(&self) -> Result<(i64, i64), String> {
|
||||||
|
let channels: i64 = self
|
||||||
|
.conn
|
||||||
|
.query_row(ORPHAN_COUNT_SQL, [], |r| r.get(0))
|
||||||
|
.map_err(|e| e.to_string())?;
|
||||||
|
let videos: i64 = self
|
||||||
|
.conn
|
||||||
|
.query_row(ORPHAN_VIDEO_COUNT_SQL, [], |r| r.get(0))
|
||||||
|
.map_err(|e| e.to_string())?;
|
||||||
|
Ok((channels, videos))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Drops channel rows that are not subscriptions and have nothing left to
|
||||||
|
/// hold on to.
|
||||||
|
///
|
||||||
|
/// A bare row exists only to parent a video saved on its own from the menu
|
||||||
|
/// bar. Once that video is no longer on disk — or on its way there — the
|
||||||
|
/// row is residue, and it is confusing residue: the channel is not in the
|
||||||
|
/// sidebar, yet its video still sits in the feed after everything else has
|
||||||
|
/// been removed.
|
||||||
|
pub fn prune_orphan_channels(&mut self) -> Result<usize, String> {
|
||||||
|
let tx = self.conn.transaction().map_err(|e| e.to_string())?;
|
||||||
|
tx.execute_batch(
|
||||||
|
&format!(
|
||||||
|
"CREATE TEMP VIEW IF NOT EXISTS orphans AS
|
||||||
|
SELECT c.id FROM channels c WHERE {ORPHAN_WHERE};
|
||||||
|
DELETE FROM downloads WHERE video_id IN (
|
||||||
|
SELECT id FROM videos WHERE channel_id IN (SELECT id FROM orphans));
|
||||||
|
DELETE FROM playback WHERE video_id IN (
|
||||||
|
SELECT id FROM videos WHERE channel_id IN (SELECT id FROM orphans));
|
||||||
|
DELETE FROM videos WHERE channel_id IN (SELECT id FROM orphans);"
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.map_err(|e| e.to_string())?;
|
||||||
|
let n = tx
|
||||||
|
.execute("DELETE FROM channels WHERE id IN (SELECT id FROM orphans)", [])
|
||||||
|
.map_err(|e| e.to_string())?;
|
||||||
|
tx.execute_batch("DROP VIEW orphans;").map_err(|e| e.to_string())?;
|
||||||
|
tx.commit().map_err(|e| e.to_string())?;
|
||||||
|
Ok(n)
|
||||||
|
}
|
||||||
|
|
||||||
/// Records a channel without subscribing to it, for a video saved on its
|
/// Records a channel without subscribing to it, for a video saved on its
|
||||||
/// own. An existing subscription is left exactly as it is.
|
/// own. An existing subscription is left exactly as it is.
|
||||||
pub fn ensure_channel(&self, c: &Channel) -> Result<(), String> {
|
pub fn ensure_channel(&self, c: &Channel) -> Result<(), String> {
|
||||||
@@ -1190,20 +1267,27 @@ mod tests {
|
|||||||
assert!(!db.list_channels().unwrap().iter().any(|c| c.id == "UCX"));
|
assert!(!db.list_channels().unwrap().iter().any(|c| c.id == "UCX"));
|
||||||
assert!(!db.channel_ids().unwrap().contains(&"UCX".to_string()));
|
assert!(!db.channel_ids().unwrap().contains(&"UCX".to_string()));
|
||||||
|
|
||||||
// And importing a CSV, which replaces subscriptions, leaves it alone.
|
// Importing a CSV, which replaces subscriptions, leaves it alone while
|
||||||
|
// it still has a downloaded video to hold on to.
|
||||||
|
db.upsert_videos(&[Video {
|
||||||
|
id: "x1".into(),
|
||||||
|
channel_id: "UCX".into(),
|
||||||
|
title: "A one-off".into(),
|
||||||
|
description: String::new(),
|
||||||
|
published: 10,
|
||||||
|
thumb_url: String::new(),
|
||||||
|
views: 0,
|
||||||
|
is_short: false,
|
||||||
|
}])
|
||||||
|
.unwrap();
|
||||||
|
db.set_download_state("x1", DownloadState::Done, None).unwrap();
|
||||||
let incoming = vec![Channel {
|
let incoming = vec![Channel {
|
||||||
id: "UC1".into(),
|
id: "UC1".into(),
|
||||||
title: "Alpha".into(),
|
title: "Alpha".into(),
|
||||||
url: "https://youtube.com/channel/UC1".into(),
|
url: "https://youtube.com/channel/UC1".into(),
|
||||||
}];
|
}];
|
||||||
db.replace_channels(&incoming).unwrap();
|
db.replace_channels(&incoming).unwrap();
|
||||||
assert_eq!(
|
assert!(db.has_any_channel("UCX").unwrap());
|
||||||
db.conn
|
|
||||||
.query_row("SELECT COUNT(*) FROM channels WHERE id='UCX'", [], |r| r
|
|
||||||
.get::<_, i64>(0))
|
|
||||||
.unwrap(),
|
|
||||||
1
|
|
||||||
);
|
|
||||||
|
|
||||||
// Subscribing to it later promotes the row rather than being refused
|
// Subscribing to it later promotes the row rather than being refused
|
||||||
// as a duplicate — saving a video is not subscribing.
|
// as a duplicate — saving a video is not subscribing.
|
||||||
@@ -1212,6 +1296,92 @@ mod tests {
|
|||||||
assert!(db.list_channels().unwrap().iter().any(|c| c.id == "UCX"));
|
assert!(db.list_channels().unwrap().iter().any(|c| c.id == "UCX"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_bare_row_with_nothing_downloaded_is_swept_by_a_replace() {
|
||||||
|
let mut db = seeded();
|
||||||
|
let one_off = Channel {
|
||||||
|
id: "UCX".into(),
|
||||||
|
title: "Saved Once".into(),
|
||||||
|
url: "https://youtube.com/channel/UCX".into(),
|
||||||
|
};
|
||||||
|
db.ensure_channel(&one_off).unwrap();
|
||||||
|
db.upsert_videos(&[Video {
|
||||||
|
id: "x1".into(),
|
||||||
|
channel_id: "UCX".into(),
|
||||||
|
title: "A one-off".into(),
|
||||||
|
description: String::new(),
|
||||||
|
published: 10,
|
||||||
|
thumb_url: String::new(),
|
||||||
|
views: 0,
|
||||||
|
is_short: false,
|
||||||
|
}])
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
// Nothing downloaded: emptying the list takes it too, or its video
|
||||||
|
// would sit in the feed with no channel behind it.
|
||||||
|
db.replace_channels(&[]).unwrap();
|
||||||
|
assert!(!db.has_any_channel("UCX").unwrap());
|
||||||
|
assert!(db.list_feed(&FeedFilter::default()).unwrap().is_empty());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn the_preview_counts_what_the_sweep_will_take() {
|
||||||
|
let mut db = seeded();
|
||||||
|
db.ensure_channel(&Channel {
|
||||||
|
id: "UCX".into(),
|
||||||
|
title: "Saved Once".into(),
|
||||||
|
url: "https://youtube.com/channel/UCX".into(),
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
db.upsert_videos(&[Video {
|
||||||
|
id: "x1".into(),
|
||||||
|
channel_id: "UCX".into(),
|
||||||
|
title: "A one-off".into(),
|
||||||
|
description: String::new(),
|
||||||
|
published: 10,
|
||||||
|
thumb_url: String::new(),
|
||||||
|
views: 0,
|
||||||
|
is_short: false,
|
||||||
|
}])
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
// Emptying the list takes both subscriptions and this bare row, and
|
||||||
|
// the confirmation has to say so before it happens.
|
||||||
|
let p = db.preview_replace(&[]).unwrap();
|
||||||
|
assert_eq!(p.removed_channels, 3, "two subscriptions and the bare row");
|
||||||
|
assert_eq!(p.removed_videos, 4, "their videos and the one-off");
|
||||||
|
|
||||||
|
db.replace_channels(&[]).unwrap();
|
||||||
|
assert!(!db.has_any_channel("UCX").unwrap());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_bare_row_whose_video_is_downloaded_survives() {
|
||||||
|
let mut db = seeded();
|
||||||
|
let one_off = Channel {
|
||||||
|
id: "UCX".into(),
|
||||||
|
title: "Saved Once".into(),
|
||||||
|
url: "https://youtube.com/channel/UCX".into(),
|
||||||
|
};
|
||||||
|
db.ensure_channel(&one_off).unwrap();
|
||||||
|
db.upsert_videos(&[Video {
|
||||||
|
id: "x1".into(),
|
||||||
|
channel_id: "UCX".into(),
|
||||||
|
title: "A one-off".into(),
|
||||||
|
description: String::new(),
|
||||||
|
published: 10,
|
||||||
|
thumb_url: String::new(),
|
||||||
|
views: 0,
|
||||||
|
is_short: false,
|
||||||
|
}])
|
||||||
|
.unwrap();
|
||||||
|
db.set_download_state("x1", DownloadState::Done, None).unwrap();
|
||||||
|
|
||||||
|
// Deliberately saved and still on disk: not this function's to delete.
|
||||||
|
db.replace_channels(&[]).unwrap();
|
||||||
|
assert!(db.has_any_channel("UCX").unwrap());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn deleting_a_subscription_takes_its_videos_with_it() {
|
fn deleting_a_subscription_takes_its_videos_with_it() {
|
||||||
let mut db = seeded();
|
let mut db = seeded();
|
||||||
|
|||||||
+14
-2
@@ -1,8 +1,9 @@
|
|||||||
//! The menu bar item.
|
//! The menu bar item.
|
||||||
//!
|
//!
|
||||||
//! Its point is to act on what a browser is showing without leaving it: while a
|
//! Its point is to act on what a browser is showing without leaving it: while a
|
||||||
//! video plays in Safari or Chrome, one click saves that video or subscribes to
|
//! video plays in Safari or Chrome, one click saves it — and one reads the
|
||||||
//! its channel, and playback carries on undisturbed.
|
//! whole subscription list off youtube.com/feed/channels — with playback
|
||||||
|
//! carrying on undisturbed.
|
||||||
//!
|
//!
|
||||||
//! Reading the browser's address needs Apple events, which macOS gates behind a
|
//! Reading the browser's address needs Apple events, which macOS gates behind a
|
||||||
//! permission the user grants once. Without it the menu still opens and says
|
//! permission the user grants once. Without it the menu still opens and says
|
||||||
@@ -199,6 +200,17 @@ async fn browser_on_subscriptions(app: &AppHandle) -> Result<String, String> {
|
|||||||
pub async fn scrape_subscriptions(app: &AppHandle) -> Result<subscriptions::Scraped, String> {
|
pub async fn scrape_subscriptions(app: &AppHandle) -> Result<subscriptions::Scraped, String> {
|
||||||
let browser = browser_on_subscriptions(app).await?;
|
let browser = browser_on_subscriptions(app).await?;
|
||||||
|
|
||||||
|
// Bring it to the front first. Chromium browsers discard a background
|
||||||
|
// tab's DOM to save memory, so the page keeps reporting its address while
|
||||||
|
// having nothing on it — which reads exactly like an empty subscription
|
||||||
|
// list rather than like a sleeping tab.
|
||||||
|
let _ = tokio::process::Command::new("/usr/bin/osascript")
|
||||||
|
.arg("-e")
|
||||||
|
.arg(format!("tell application {} to activate", quote(&browser)))
|
||||||
|
.output()
|
||||||
|
.await;
|
||||||
|
tokio::time::sleep(std::time::Duration::from_millis(1200)).await;
|
||||||
|
|
||||||
let mut last = 0usize;
|
let mut last = 0usize;
|
||||||
let mut settled = 0;
|
let mut settled = 0;
|
||||||
for round in 0..40 {
|
for round in 0..40 {
|
||||||
|
|||||||
Reference in New Issue
Block a user