From e330f9db70d05ed168273bb2c276f2f85d33ca73 Mon Sep 17 00:00:00 2001 From: Kacper Paczos Date: Thu, 27 Aug 2026 08:58:55 +0200 Subject: [PATCH] fix(settings): size the unused downloads list to its real rows (#686) * fix(settings): size the unused downloads list to its real rows The list reserved 56px per row, but a row renders taller than that once its margins and the Delete button are counted. With two leftover models the header read "2 leftover models" while only the first row fitted in the viewport, and the second could neither be seen nor deleted. The overlay scrollbar gave no hint either, since it stays invisible until hovered. Measure the list once its rows are in place and cap that at the existing 220px, and keep the scrollbar in the layout so a list that does reach the cap looks scrollable. * fix(settings): test the list sizing without reimporting the dialog Review follow-ups on #686. The test helper popped vocalinux.ui.settings_dialog from sys.modules at collection time and reimported it under a patched Gtk, restoring only sys.modules and not the attribute on the package. Whichever module object a later test reached then depended on collection order, which is why test_model_deletion::test_whisper_list_and_delete failed on 3.9/3.10 with assert [] == ['tiny'] while main stayed green. Move the arithmetic into a module-level _clamp_unused_downloads_height() and unit-test that directly, the way _list_downloaded_whisper_models is already tested. Nothing touches sys.modules now. The dialog cannot be instantiated under the mocked-GTK harness, so the wiring the old tests never covered is asserted against the method source, following test_settings_mode_change.py: refresh calls the fit, the fit measures the listbox and feeds set_min_content_height, the per-row estimate stays gone, and the scrollbar stays out of overlay mode. * fix(settings): remeasure the unused-downloads list when it expands The refresh measures while the expander is collapsed and the dialog may not be mapped; if that first measurement ever comes back short, the expand handler remeasures so the rows cannot stay clipped. --- src/vocalinux/ui/settings_dialog.py | 35 ++++++++++++- tests/test_unused_downloads_height.py | 72 +++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 tests/test_unused_downloads_height.py diff --git a/src/vocalinux/ui/settings_dialog.py b/src/vocalinux/ui/settings_dialog.py index a0ab08a..49d224d 100644 --- a/src/vocalinux/ui/settings_dialog.py +++ b/src/vocalinux/ui/settings_dialog.py @@ -147,6 +147,15 @@ ENGINE_MODELS = { "remote_api": [], # Remote API does not need local models } +# Tallest the "Unused downloads" list may grow before it starts scrolling +_UNUSED_DOWNLOADS_MAX_HEIGHT = 220 + + +def _clamp_unused_downloads_height(natural_height: int) -> int: + """Clamp a measured list height to the height the page gives the list.""" + return min(_UNUSED_DOWNLOADS_MAX_HEIGHT, natural_height) + + # Engine display name mapping ENGINE_DISPLAY_NAMES = { "vosk": "Vosk", @@ -2498,7 +2507,10 @@ class SettingsDialog(Gtk.Dialog): self.unused_models_scroll.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC) self.unused_models_scroll.set_shadow_type(Gtk.ShadowType.NONE) self.unused_models_scroll.set_propagate_natural_height(True) - self.unused_models_scroll.set_max_content_height(220) + self.unused_models_scroll.set_max_content_height(_UNUSED_DOWNLOADS_MAX_HEIGHT) + # Keep the scrollbar in the layout: an overlay one stays invisible until + # hovered, so a list clipped by the cap looks like it has no more rows. + self.unused_models_scroll.set_overlay_scrolling(False) # ListBox is GtkScrollable, so wrapping it in a Box forces a Viewport # and lets the expander child report a real height. @@ -2507,6 +2519,12 @@ class SettingsDialog(Gtk.Dialog): list_holder.pack_start(self.unused_models_group.listbox, False, False, 0) self.unused_models_scroll.add(list_holder) self.unused_expander.add(self.unused_models_scroll) + # Backstop: the refresh measures while the expander is collapsed and + # the dialog may not be mapped yet. Remeasure when the user expands, + # so a short first measurement can never leave the list clipped. + self.unused_expander.connect( + "notify::expanded", lambda *_args: self._fit_unused_downloads_height() + ) self.unused_models_group.pack_start(self.unused_expander, False, False, 0) self.content_box.pack_start(self.unused_models_group, False, False, 0) @@ -4610,7 +4628,6 @@ class SettingsDialog(Gtk.Dialog): count = len(unused) leftover = "leftover model" if count == 1 else "leftover models" self.unused_expander.set_label(f"Unused downloads ({count} {leftover})") - self.unused_models_scroll.set_min_content_height(min(220, 56 * count)) was_expanded = self.unused_expander.get_expanded() for model_id, title, size_label in unused: @@ -4633,6 +4650,20 @@ class SettingsDialog(Gtk.Dialog): self.unused_models_group.show_all() self.unused_expander.set_expanded(was_expanded) + self._fit_unused_downloads_height() + + def _fit_unused_downloads_height(self): + """Size the list from the rows it actually holds. + + A per-row estimate cannot know how tall a row renders: with margins and + a Delete button the rows are taller than the estimate, so the last one + was cut off below the edge of the viewport while the header still + counted it (#683). + """ + _, natural_height = self.unused_models_group.listbox.get_preferred_height() + self.unused_models_scroll.set_min_content_height( + _clamp_unused_downloads_height(natural_height) + ) def _confirm_model_delete(self, text: str, secondary: str) -> bool: """Ask before deleting model files from disk.""" diff --git a/tests/test_unused_downloads_height.py b/tests/test_unused_downloads_height.py new file mode 100644 index 0000000..8fc4496 --- /dev/null +++ b/tests/test_unused_downloads_height.py @@ -0,0 +1,72 @@ +"""Tests for sizing the "Unused downloads" list to the rows it holds. + +The settings dialog cannot be instantiated under the mocked-GTK harness (see +test_settings_mode_change.py), so the arithmetic lives in a module-level +helper that is unit-tested directly, and the wiring around it is asserted +against the source of the two methods. +""" + +import inspect +import re + +from vocalinux.ui import settings_dialog +from vocalinux.ui.settings_dialog import ( + _UNUSED_DOWNLOADS_MAX_HEIGHT, + _clamp_unused_downloads_height, +) + + +def _method_source(name: str) -> str: + src = inspect.getsource(settings_dialog) + match = re.search(rf"\n def {name}\(.*?(?=\n def )", src, re.DOTALL) + assert match, f"could not locate method {name}" + return match.group(0) + + +def test_list_is_as_tall_as_its_rows_render(): + """Two rows that measure 116px must not be squeezed into an estimate. + + 56px per row was the old guess; real rows render taller than that once + margins and the Delete button are counted, which cut off the last row + while the header still counted it (#683). + """ + assert _clamp_unused_downloads_height(116) == 116 + + +def test_long_lists_stop_growing_at_the_cap(): + """Beyond the cap the list scrolls instead of pushing the page down.""" + assert _clamp_unused_downloads_height(_UNUSED_DOWNLOADS_MAX_HEIGHT * 3) == ( + _UNUSED_DOWNLOADS_MAX_HEIGHT + ) + + +def test_refresh_sizes_the_list_after_building_the_rows(): + """A measured height is worthless if the refresh never asks for one.""" + body = _method_source("_refresh_unused_downloads") + assert "self._fit_unused_downloads_height()" in body + # The per-row estimate is what clipped the list; it must not come back. + assert "56 *" not in body + + +def test_fit_measures_the_listbox_and_feeds_the_scroll(): + body = _method_source("_fit_unused_downloads_height") + assert "self.unused_models_group.listbox.get_preferred_height()" in body + assert "self.unused_models_scroll.set_min_content_height(" in body + assert "_clamp_unused_downloads_height(natural_height)" in body + + +def test_scrollbar_stays_in_the_layout(): + """An overlay scrollbar hides until hovered, so a capped list looks whole.""" + src = inspect.getsource(settings_dialog) + assert "self.unused_models_scroll.set_overlay_scrolling(False)" in src + + +def test_expanding_the_list_remeasures_it(): + """The refresh measures while collapsed and possibly unmapped; expanding + must remeasure so a short first measurement can never leave rows clipped.""" + src = inspect.getsource(settings_dialog) + assert re.search( + r"self\.unused_expander\.connect\(\s*\"notify::expanded\"," + r"\s*lambda \*_args: self\._fit_unused_downloads_height\(\)", + src, + )