mirror of
https://github.com/alexta69/metube.git
synced 2026-09-21 13:35:01 +00:00
fix: stop offering a dead Start button on queued downloads (closes #1081)
A download waiting for a MAX_CONCURRENT_DOWNLOADS slot sat at status
'pending' — the same status as an item added with auto-start off, which
is waiting for the user to press Start. The Downloading table draws its
Start button for exactly that status, so every queued row offered one.
Pressing it did nothing. start_pending() looks the id up in self.pending
first, and a queued download is not there; the fallback branch only acts
on 'scheduled' items, so the call fell through and still returned
{'status': 'ok'} — the UI reported success for a no-op.
One status name was covering two different states. A download in
self.queue waiting on the semaphore is now 'queued', leaving 'pending'
to mean only "waiting for you to press Start". The template condition is
unchanged and now excludes these rows by construction, and a 'Queued'
badge says why the row is idle instead of leaving a bare empty bar.
start_pending() notifies on promotion as well: with the slots saturated
the wait before Download.start() reports 'preparing' is unbounded, and
until something lands the client keeps showing the button it outgrew.
Persisted state needs no migration — __import_queue re-adds saved items
through __add_download, which sets the new status.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import copy
|
||||
import os
|
||||
import re
|
||||
@@ -311,6 +312,47 @@ async def test_start_pending_moves_to_queue(dq_env):
|
||||
with patch.object(DownloadQueue, "_DownloadQueue__start_download", AsyncMock()):
|
||||
await dq.start_pending([url])
|
||||
assert not dq.pending.exists(url)
|
||||
# It is in the queue now and starts on its own, so it must not keep
|
||||
# advertising the Start button the UI draws for 'pending' (#1081), and the
|
||||
# client has to be told before the concurrency slot frees up.
|
||||
assert dq.queue.get(url).info.status == "queued"
|
||||
assert notifier.updated.await_args[0][0].status == "queued"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_queued_download_is_not_offered_as_startable(dq_env):
|
||||
"""A download waiting on a MAX_CONCURRENT_DOWNLOADS slot used to sit at
|
||||
'pending', which is the status the UI draws a Start button for — but
|
||||
start_pending has nothing to do for an item already in the queue, so the
|
||||
button silently did nothing and still reported success (#1081).
|
||||
"""
|
||||
notifier = AsyncMock()
|
||||
dq_env.MAX_CONCURRENT_DOWNLOADS = "1"
|
||||
dq = DownloadQueue(dq_env, notifier)
|
||||
released = asyncio.Event()
|
||||
|
||||
def fake_extract(self, url, *_args, **_kwargs):
|
||||
return {"_type": "video", "id": url[-1], "title": f"Video {url[-1]}",
|
||||
"url": url, "webpage_url": url}
|
||||
|
||||
async def blocking_start(self, notifier_, executor=None):
|
||||
await released.wait()
|
||||
|
||||
first, second = "https://example.com/v1", "https://example.com/v2"
|
||||
with patch.object(DownloadQueue, "_DownloadQueue__extract_info", fake_extract), \
|
||||
patch("ytdl.Download.start", blocking_start), \
|
||||
patch("ytdl.Download.close", lambda self: None):
|
||||
for url in (first, second):
|
||||
await dq.add(url, "video", "auto", "any", "best", "", "", 0, auto_start=True)
|
||||
await asyncio.sleep(0)
|
||||
|
||||
# The first holds the only slot; the second is waiting behind it.
|
||||
assert dq.queue.get(second).info.status == "queued"
|
||||
assert not dq.pending.exists(second)
|
||||
|
||||
released.set()
|
||||
|
||||
await asyncio.sleep(0)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -1200,7 +1242,7 @@ async def test_probe_scheduled_starts_when_live(dq_env):
|
||||
|
||||
assert url not in dq._scheduled_probe_at
|
||||
assert download.info.live_status == "is_live"
|
||||
assert download.info.status == "pending"
|
||||
assert download.info.status == "queued"
|
||||
start_mock.assert_called_once_with(download)
|
||||
|
||||
|
||||
@@ -1345,7 +1387,7 @@ async def test_probe_recovers_after_transient_then_starts(dq_env):
|
||||
|
||||
assert url not in dq._scheduled_probe_at
|
||||
assert url not in dq._scheduled_probe_failures
|
||||
assert download.info.status == "pending"
|
||||
assert download.info.status == "queued"
|
||||
# Placeholder error/msg cleared now that a real download is starting.
|
||||
assert download.info.error is None
|
||||
assert download.info.msg is None
|
||||
|
||||
+15
-2
@@ -495,6 +495,12 @@ class DownloadInfo:
|
||||
self.folder = folder
|
||||
self.custom_name_prefix = custom_name_prefix
|
||||
self.msg = self.percent = self.speed = self.eta = None
|
||||
# 'pending' means "waiting for the user to press Start" — an item added
|
||||
# with auto_start=False, sitting in self.pending. A download that is in
|
||||
# self.queue waiting for a MAX_CONCURRENT_DOWNLOADS slot is 'queued'
|
||||
# instead: it starts on its own and there is nothing to press. Keeping
|
||||
# both under one name left the UI showing a Start button that silently
|
||||
# did nothing (#1081).
|
||||
self.status = "pending"
|
||||
self.size = None
|
||||
self.timestamp = time.time_ns()
|
||||
@@ -1484,7 +1490,7 @@ class DownloadQueue:
|
||||
return
|
||||
|
||||
self._unregister_scheduled(url)
|
||||
info.status = 'pending'
|
||||
info.status = 'queued'
|
||||
# Clear the "scheduled to start at ..." placeholder now that the stream
|
||||
# is live and a real download is about to begin.
|
||||
info.error = None
|
||||
@@ -1499,7 +1505,7 @@ class DownloadQueue:
|
||||
|
||||
def _force_start_scheduled(self, download: Download) -> None:
|
||||
self._unregister_scheduled(download.info.url)
|
||||
download.info.status = 'pending'
|
||||
download.info.status = 'queued'
|
||||
download.info.error = None
|
||||
download.info.msg = None
|
||||
bg_tasks.create_task(self.__start_download(download), name="start_download")
|
||||
@@ -1649,6 +1655,7 @@ class DownloadQueue:
|
||||
if is_upcoming:
|
||||
await self._schedule_upcoming_download(download)
|
||||
else:
|
||||
download.info.status = 'queued'
|
||||
await self.queue.put(download)
|
||||
bg_tasks.create_task(self.__start_download(download), name="start_download")
|
||||
else:
|
||||
@@ -2159,7 +2166,13 @@ class DownloadQueue:
|
||||
if getattr(dl.info, 'live_status', None) == 'is_upcoming':
|
||||
await self._schedule_upcoming_download(dl)
|
||||
else:
|
||||
dl.info.status = 'queued'
|
||||
await self.queue.put(dl)
|
||||
# Tell the client it moved out of 'pending' now, not when a
|
||||
# slot frees up: with MAX_CONCURRENT_DOWNLOADS saturated the
|
||||
# wait is unbounded, and until this lands the row still
|
||||
# offers the Start button it has already outgrown.
|
||||
await self.notifier.updated(dl.info)
|
||||
bg_tasks.create_task(self.__start_download(dl), name="start_download")
|
||||
continue
|
||||
if self.queue.exists(id):
|
||||
|
||||
@@ -728,6 +728,9 @@
|
||||
@if (download.value.live_status === 'is_live' && download.value.status !== 'scheduled') {
|
||||
<span class="badge bg-danger">LIVE</span>
|
||||
}
|
||||
@if (download.value.status === 'queued') {
|
||||
<span class="badge bg-secondary">Queued</span>
|
||||
}
|
||||
</div>
|
||||
@if (download.value.status === 'scheduled') {
|
||||
<span class="badge bg-warning text-dark">
|
||||
|
||||
@@ -544,4 +544,57 @@ describe('App', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// Issue #1081: a download waiting for a concurrency slot ('queued') starts on
|
||||
// its own, so it must not offer the Start button that a 'pending' row — one
|
||||
// added with auto-start off — legitimately has.
|
||||
describe('queued rows do not offer a dead Start button (#1081)', () => {
|
||||
const queueEntry = (status: string): Download => ({
|
||||
id: 'vid1',
|
||||
title: 'Test',
|
||||
url: 'https://example.com/v',
|
||||
download_type: 'video',
|
||||
quality: 'best',
|
||||
format: 'any',
|
||||
folder: '',
|
||||
custom_name_prefix: '',
|
||||
playlist_item_limit: 0,
|
||||
status,
|
||||
msg: '',
|
||||
percent: 0,
|
||||
speed: 0,
|
||||
eta: 0,
|
||||
filename: '',
|
||||
checked: false,
|
||||
} as Download);
|
||||
|
||||
const render = (status: string) => {
|
||||
const fixture = TestBed.createComponent(App);
|
||||
downloads.queue.set('https://example.com/v', queueEntry(status));
|
||||
downloads.queueChanged.next();
|
||||
fixture.detectChanges();
|
||||
return fixture;
|
||||
};
|
||||
|
||||
it('hides Start for a queued row but keeps it for a pending one', () => {
|
||||
const queued = render('queued');
|
||||
expect(
|
||||
(queued.nativeElement as HTMLElement).querySelector('[aria-label="Start download for Test"]')
|
||||
).toBeNull();
|
||||
|
||||
downloads.queue.clear();
|
||||
|
||||
const pending = render('pending');
|
||||
expect(
|
||||
(pending.nativeElement as HTMLElement).querySelector('[aria-label="Start download for Test"]')
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
it('says why the row is idle and counts it as queued', () => {
|
||||
const fixture = render('queued');
|
||||
expect((fixture.nativeElement as HTMLElement).textContent).toContain('Queued');
|
||||
expect(fixture.componentInstance.queuedDownloads).toBe(1);
|
||||
expect(fixture.componentInstance.activeDownloads).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
+1
-1
@@ -1728,7 +1728,7 @@ export class App implements AfterViewInit, OnInit, OnDestroy {
|
||||
speed += download.speed || 0;
|
||||
} else if (download.status === 'preparing' || download.status === 'postprocessing') {
|
||||
active++;
|
||||
} else if (download.status === 'pending' || download.status === 'scheduled') {
|
||||
} else if (download.status === 'queued' || download.status === 'pending' || download.status === 'scheduled') {
|
||||
queued++;
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user