Toolbox install: report crawl progress and explain an empty install list - #44
Toolbox install: report crawl progress and explain an empty install list#44mojie126 wants to merge 4 commits into
Conversation
Fixes TypesettingTools#43 - Call aegisub.progress.task before crawling feeds and before loading feed data, so the background script progress dialog no longer sits blank for the whole crawl. - When nothing new can be installed, show a notice dialog instead of the script list dialog with only a Cancel button.
line0
left a comment
There was a problem hiding this comment.
Thanks you for the contribution, please have a look at my findings.
| unless next(modules) or next(macros) | ||
| aegisub.dialog.display {{class: "label", x: 0, y: 0, width: 1, height: 1, label: msgs.install.empty}}, | ||
| {buttons.close}, {ok: buttons.close, cancel: buttons.close} | ||
| return | ||
|
|
There was a problem hiding this comment.
Did you test that? I don't think it can actually work because addAvailableToInstall() adds a channel table to modules or macros for each available package, installed or not. The only situation in which the "All available scripts are already installed; nothing new to install." message dialog would actually show is when no scripts are (currently) available to install at all (no internet and no feed cache, all known feeds blocked, other freak occurrences).
At the very least, you'll have to do this check on the length of the flattened moduleList and macroList below, but even then you can't just draw the conclusion that all available feeds must be installed. You also want to collect metadata from FeedInventory.crawl() that tells you if any feed wasn't fetched and for what reason. If we couldn't crawl all feeds that should have been crawled we should point out as such, because that may be the reason why there's nothing to install.
| scanning: "Scanning %d available feeds...", | ||
| scanningTask: "Scanning available feeds..." | ||
| loadingTask: "Loading feed data..." | ||
| empty: "All available scripts are already installed; nothing new to install." |
There was a problem hiding this comment.
the second clause just restates what obviously follows from the first one, so just drop it.
| -- FeedInventory crawls the known feeds, which are trust-gated and bounded. The shared feed loader then | ||
| -- serves each reachable feed's data from the cache the crawl just populated. | ||
| macros, modules = {}, {} | ||
| aegisub.progress.task msgs.install.scanningTask |
There was a problem hiding this comment.
it would be even better if this one actually displayed a moving progress bar but that would involve adding a progress callback to FeedInventory.crawl(), so if you'd rather keep it simple, leave it is and I'll add it to my TODO list as a low priority item.
| aegisub.progress.task msgs.install.scanningTask | ||
| entries = crawlWithPrompt buildFeedInventory! | ||
|
|
||
| aegisub.progress.task msgs.install.loadingTask |
There was a problem hiding this comment.
does this one show for any appreciable time for you? It doesn't hurt to have this, but if loading the feeds after fetching is near-instant for you and you haven't disabled the feed cache then it's worth investigating why.
addAvailableToInstall creates a (possibly empty) table for every known package regardless of install state, so next(modules)/next(macros) is almost always truthy and the empty notice could never fire. Check the flattened moduleList/macroList lengths instead, and when some feeds weren't fetched say so, since that may be why there is nothing to install. Also drop the redundant second clause of the empty message.
|
Thanks for the review! Pushed
|
Called after each feed is fetched, with the running fetch count and the number of feeds currently known to the crawl. The crawl discovers more feeds as it goes, so the known count may grow over time.
Drive aegisub.progress.set from the new crawl callback so the scan shows actual progress, and abort the crawl when the user cancels: the callback raises, crawlWithPrompt traps it (resetting the prompter), re-raises real errors, and install returns quietly on cancellation.
|
Went ahead and implemented the crawl progress callback after all (
One structural limitation remains, which I don't think the script can do anything about: while the (modal) script-list dialog is up, the background progress dialog's Cancel button has no one to poll it, so it stays on "Cancelling..." until the dialog is dismissed — closing the list dialog is the way out there. Tested on Windows against v0.9.0 with the feed cache cleared: the bar advances over the ~9 s crawl, tops up at the end, and cancelling mid-crawl aborts immediately. |
- 修复 $ToolboxPatchEmpty 替换文本漏带 buildDlgList macros 行, 导致 getScriptListDlg 内 unpack(nil) 运行时错误 - 空列表判断对齐上游 PR TypesettingTools/DependencyControl#44 评审意见:改为 buildDlgList 展平后长度, 并区分部分 feed 未抓取场景(不再误报全部已安装) - FeedInventory 增加 crawl 进度回调(可选 onProgress 参数), Install Script 爬取期间显示动态进度条并收尾满格 - 支持下载期间取消:回调检查 aegisub.progress.is_cancelled 中断 crawl, crawlWithPrompt 以 pcall 捕获静默退出并保证 prompter 复位 实测通过:进度反馈、下载中取消、空列表提示、正常安装四场景
Fixes #43.
What
Two small UX fixes for the Toolbox Install Script macro:
Progress feedback during the feed crawl. The macro now calls
aegisub.progress.taskwith "Scanning available feeds..." beforecrawlWithPrompt buildFeedInventory!and "Loading feed data..." before loading the feeds, so the background-script progress dialog shows what is happening instead of sitting blank for the whole crawl (~9 s with the default feed set).logger\logalone doesn't help here since it only writes to the DependencyControl log file.Explain an empty install list. When both
macrosandmodulesare empty — the common case, since installed scripts are registered automatically andaddAvailableToInstallfilters them out — the macro now shows a small notice dialog ("All available scripts are already installed; nothing new to install.") and returns, instead of popping the script list dialog with only a Cancel button.Messages go through the existing
msgs.installtable, and the notice dialog reuses the label-dialog shape ofconfirmDialog.Testing
Verified against v0.9.0 running in Aegisub (Windows): the progress dialog shows both task messages during the crawl, and with everything already installed the notice appears instead of the empty list.