diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01cf26c..41781d6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,11 +22,22 @@ jobs: - name: Install test dependencies run: python -m pip install pytest + - name: Install GUI dependencies (for the headless UI regression tests) + run: python -m pip install customtkinter darkdetect Pillow + - name: Byte-compile all sources run: python -m compileall -q main.py app.py scanner.py file_utils.py analysis.py updater.py version.py registry_installer.py simple_installer.py make_version_info.py - name: Validate version resource generation run: python make_version_info.py - - name: Run tests + - name: Run tests (Linux, headless display for the UI tests) + if: runner.os == 'Linux' + run: | + sudo apt-get update -qq + sudo apt-get install -y -qq xvfb + xvfb-run -a python -m pytest tests -v + + - name: Run tests (Windows) + if: runner.os == 'Windows' run: python -m pytest tests -v diff --git a/app.py b/app.py index 91dca1e..0bb8b2b 100644 --- a/app.py +++ b/app.py @@ -303,14 +303,20 @@ def __init__(self, initial_path: Optional[str] = None): self._search_after = None # tree-view state + self.tree: Optional[ttk.Treeview] = None self.iid_to_node: Dict[str, Node] = {} self.sort_key = "size" self.sort_reverse = True + # largest-files view state + self.largest_tree: Optional[ttk.Treeview] = None + self.largest_map: Dict[str, Node] = {} + # treemap state self.treemap_stack: List[Node] = [] self._tile_items: Dict[int, analysis.Tile] = {} self.tooltip: Optional[Tooltip] = None + self._treemap_redraw_after = None self._build_toolbar() self._build_body() @@ -507,8 +513,15 @@ def _on_view_change(self, value): def _clear_body(self): if self.tooltip: self.tooltip.hide() + if self._treemap_redraw_after: + self.after_cancel(self._treemap_redraw_after) + self._treemap_redraw_after = None for child in self.body.winfo_children(): child.destroy() + # drop references to the widgets we just destroyed so nothing + # reaches for a stale one later + self.tree = None + self.largest_tree = None def _render_active_view(self): self._clear_body() @@ -730,7 +743,9 @@ def _render_largest(self): "path": (560, 240, "w", True), } self.largest_tree = self._make_treeview(("size", "type", "path"), headings, widths) - self.largest_map: Dict[str, Node] = {} + self.largest_map = {} + self.largest_tree.bind("<>", self._on_tree_select) + self.largest_tree.bind("", lambda e: self._delete_selected()) files = analysis.largest_files(self.root_node, 100) if self.search_query: @@ -828,11 +843,29 @@ def _render_treemap(self): if self.tooltip is None: self.tooltip = Tooltip(self) self.treemap_node = node - self.treemap_canvas.bind("", lambda e: self._draw_treemap()) + self.treemap_canvas.bind("", lambda e: self._schedule_treemap_redraw()) self.treemap_canvas.bind("", self._treemap_hover) self.treemap_canvas.bind("", lambda e: self.tooltip.hide()) self.treemap_canvas.bind("", self._treemap_click) + def _schedule_treemap_redraw(self): + """Coalesce the burst of events a window resize produces + into a single re-layout.""" + if self._treemap_redraw_after: + self.after_cancel(self._treemap_redraw_after) + self._treemap_redraw_after = self.after(80, self._do_treemap_redraw) + + def _do_treemap_redraw(self): + self._treemap_redraw_after = None + if getattr(self, "treemap_canvas", None) is None: + return + try: + if not self.treemap_canvas.winfo_exists(): + return + except tk.TclError: + return + self._draw_treemap() + def _draw_treemap(self): canvas = self.treemap_canvas canvas.delete("all") @@ -858,8 +891,12 @@ def _draw_treemap(self): self._tile_items[item] = tile if tile.w > 46 and tile.h > 16: text_color = "#f8fafc" if (self.settings.dark_mode or not n.is_dir) else "#1f2937" - canvas.create_text(tile.x + 4, tile.y + 3, anchor="nw", text=n.name[:40], - fill=text_color, font=("Segoe UI", 8)) + label = canvas.create_text(tile.x + 4, tile.y + 3, anchor="nw", text=n.name[:40], + fill=text_color, font=("Segoe UI", 8)) + # map the label to the same tile: hit-testing resolves to the + # topmost item, so without this the tooltip/click would be lost + # exactly where the pointer most naturally lands + self._tile_items[label] = tile def _treemap_hover(self, event): items = self.treemap_canvas.find_closest(event.x, event.y) @@ -906,22 +943,46 @@ def _clear_search(self): # --------------------------------------------------------------- actions - def _selected_nodes(self) -> List[Node]: - return [self.iid_to_node[iid] for iid in self.tree.selection() if iid in self.iid_to_node] + def _selection_context(self): + """Return (tree_widget, iid->Node map) for the view that currently owns + a selection, or (None, {}) when the active view has none. - def _top_level_selection(self) -> List[str]: - selection = set(self.tree.selection()) + Views are rebuilt on every switch, so the widget references are only + valid for the view that is on screen right now. + """ + if self.active_view == "Tree" and self.tree is not None: + return self.tree, self.iid_to_node + if self.active_view == "Largest Files" and self.largest_tree is not None: + return self.largest_tree, self.largest_map + return None, {} + + def _selected_nodes(self) -> List[Node]: + tree, mapping = self._selection_context() + if tree is None: + return [] + return [mapping[iid] for iid in tree.selection() if iid in mapping] + + def _top_level_selection(self) -> List[tuple]: + """Selected (iid, node) pairs, with anything nested under another + selected row removed so we never act on the same bytes twice.""" + tree, mapping = self._selection_context() + if tree is None: + return [] + + selection = set(tree.selection()) result = [] - for iid in self.tree.selection(): - parent = self.tree.parent(iid) + for iid in tree.selection(): + if iid not in mapping: + continue + parent = tree.parent(iid) nested = False while parent: if parent in selection: nested = True break - parent = self.tree.parent(parent) + parent = tree.parent(parent) if not nested: - result.append(iid) + result.append((iid, mapping[iid])) return result def _reveal(self, path: str): @@ -962,15 +1023,15 @@ def worker(): threading.Thread(target=worker, daemon=True).start() def _zip_selected(self): - iids = self._top_level_selection() - if not iids: - messagebox.showwarning("No selection", "Select files or folders to zip.") + selection = self._top_level_selection() + if not selection: + messagebox.showwarning("No selection", self._no_selection_hint()) return save_path = filedialog.asksaveasfilename(defaultextension=".zip", filetypes=[("ZIP files", "*.zip")], title="Save ZIP as") if not save_path: return - paths = [self.iid_to_node[i].path for i in iids if i in self.iid_to_node] + paths = [node.path for _, node in selection] self._set_status("Creating ZIP…") def worker(): @@ -996,21 +1057,29 @@ def worker(): messagebox.showerror("Error", f"Failed to create ZIP: {msg}"))) threading.Thread(target=worker, daemon=True).start() + def _no_selection_hint(self) -> str: + if self.active_view in ("Tree", "Largest Files"): + return "Select files or folders first." + return "Switch to the Tree or Largest Files view to select items." + def _delete_selected(self): - iids = self._top_level_selection() - if not iids: - messagebox.showwarning("No selection", "Select files or folders to delete.") + selection = self._top_level_selection() + if not selection: + messagebox.showwarning("No selection", self._no_selection_hint()) return - nodes = [self.iid_to_node[i] for i in iids if i in self.iid_to_node] - total = sum(n.size for n in nodes) + total = sum(node.size for _, node in selection) if not messagebox.askyesno("Confirm delete", - f"Delete {len(nodes)} item(s) ({format_size(total)})?\nThis cannot be undone."): + f"Delete {len(selection)} item(s) ({format_size(total)})?\nThis cannot be undone."): return self._set_status("Deleting…") + # remember which view started this so the async result never touches a + # widget the user has since navigated away from + tree, mapping = self._selection_context() + def worker(): deleted, errors = [], [] - for iid, node in zip(iids, nodes): + for iid, node in selection: try: if os.path.isdir(node.path): shutil.rmtree(node.path) @@ -1019,10 +1088,17 @@ def worker(): deleted.append((iid, node)) except Exception as e: errors.append(f"{node.name}: {e}") - self.after(0, lambda: self._apply_deletions(deleted, errors)) + self.after(0, lambda: self._apply_deletions(deleted, errors, tree, mapping)) threading.Thread(target=worker, daemon=True).start() - def _apply_deletions(self, deleted, errors): + def _apply_deletions(self, deleted, errors, tree=None, mapping=None): + rows_alive = False + if tree is not None: + try: + rows_alive = bool(tree.winfo_exists()) + except tk.TclError: + rows_alive = False + for iid, node in deleted: removed_items = (1 + node.item_count) if node.is_dir else 1 parent = node.parent @@ -1033,9 +1109,14 @@ def _apply_deletions(self, deleted, errors): walk.size -= node.size walk.item_count -= removed_items walk = walk.parent - if self.tree.exists(iid): - self.tree.delete(iid) - self.iid_to_node.pop(iid, None) + if rows_alive: + try: + if tree.exists(iid): + tree.delete(iid) + except tk.TclError: + rows_alive = False + if mapping is not None: + mapping.pop(iid, None) if self.root_node: self.status_right.configure(text=f"Total: {format_size(self.root_node.size)}") diff --git a/tests/test_gui.py b/tests/test_gui.py new file mode 100644 index 0000000..73ef641 --- /dev/null +++ b/tests/test_gui.py @@ -0,0 +1,219 @@ +"""GUI regression tests. + +These cover crashes that pure-logic tests can't reach, so they need a real Tk +display. They skip themselves cleanly when tkinter or a display is missing +(e.g. a plain CI runner without xvfb), and never block on a mainloop. +""" +import os +import sys +import threading +import time + +import pytest + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +tk = pytest.importorskip("tkinter", reason="tkinter not available") +pytest.importorskip("customtkinter", reason="customtkinter not installed") + +if sys.platform != "win32" and not os.environ.get("DISPLAY"): + pytest.skip("no display available", allow_module_level=True) + +try: + _probe = tk.Tk() + _probe.destroy() +except Exception as exc: # pragma: no cover - environment dependent + pytest.skip(f"cannot open a Tk display: {exc}", allow_module_level=True) + +from scanner import TreeScanner + + +def scan_sync(path): + """Scan without needing a mainloop for the completion callback.""" + scanner = TreeScanner() + holder = {} + done = threading.Event() + scanner.scan( + str(path), + on_complete=lambda root, errors, t: (holder.update(root=root), done.set()), + on_error=lambda msg: (holder.update(error=msg), done.set()), + ) + assert done.wait(timeout=30), "scan did not finish" + return holder.get("root") + + +@pytest.fixture +def sample_tree(tmp_path): + (tmp_path / "big.mp4").write_bytes(b"v" * 60000) + (tmp_path / "notes.txt").write_bytes(b"t" * 800) + sub = tmp_path / "sub" + sub.mkdir() + (sub / "pic.png").write_bytes(b"p" * 9000) + return tmp_path + + +@pytest.fixture +def gui(tmp_path, sample_tree, monkeypatch): + """A FolderLensApp with an already-scanned tree and isolated settings.""" + # keep the test off the developer's real settings file + monkeypatch.setenv("APPDATA", str(tmp_path / "cfg")) + monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "cfg")) + + import app as appmod + + root = scan_sync(sample_tree) + assert root is not None + + win = appmod.FolderLensApp(initial_path=None) # no scan kicked off + win.geometry("1000x700+0+0") + win.root_node = root + try: + yield win + finally: + try: + win.destroy() + except tk.TclError: + pass + + +def show(win, view): + win.active_view = view + win.view_switch.set(view) + win._render_active_view() + win.update_idletasks() + + +def test_toolbar_actions_safe_in_view_without_selection(gui): + """Regression: the app remembers the last view, so it can start in Treemap. + The always-visible Zip/Delete buttons used to raise AttributeError there.""" + show(gui, "Treemap") + + assert gui._top_level_selection() == [] + assert gui._selected_nodes() == [] + # and the user gets a hint pointing at a view that does have a selection + assert "Tree" in gui._no_selection_hint() + + +def test_selection_helpers_survive_view_switch(gui): + """Regression: switching views destroys the treeview widget; the stale + reference used to raise TclError on the next toolbar action.""" + show(gui, "Tree") + assert gui.tree is not None + gui.tree.selection_set(gui.tree.get_children()[0]) + assert len(gui._top_level_selection()) == 1 + + show(gui, "Treemap") + assert gui.tree is None + assert gui._top_level_selection() == [] # must not raise + + show(gui, "Tree") # and it comes back + assert gui.tree is not None + assert gui._top_level_selection() == [] + + +def test_selection_works_in_largest_files_view(gui): + """Largest Files is where you find space hogs, so Zip/Delete must work + against that view's selection too.""" + show(gui, "Largest Files") + assert gui.largest_tree is not None + + rows = gui.largest_tree.get_children() + assert rows + gui.largest_tree.selection_set(rows[0]) + + selection = gui._top_level_selection() + assert len(selection) == 1 + iid, node = selection[0] + assert node.name == "big.mp4" # largest file first + assert gui._selected_nodes() == [node] + + +def test_top_level_selection_drops_nested_rows(gui): + """A folder and something inside it must not both be acted on.""" + show(gui, "Tree") + folder_iid = next(i for i, n in gui.iid_to_node.items() if n.is_dir) + gui.tree.item(folder_iid, open=True) + + # expand: replace the lazy placeholder with the real rows + kids = gui.tree.get_children(folder_iid) + if len(kids) == 1 and gui._is_dummy(kids[0]): + gui.tree.delete(kids[0]) + gui._insert_tree_children(folder_iid, gui.iid_to_node[folder_iid]) + + child_iid = gui.tree.get_children(folder_iid)[0] + gui.tree.selection_set(folder_iid, child_iid) + + selection = gui._top_level_selection() + assert [n.name for _, n in selection] == [gui.iid_to_node[folder_iid].name] + + +def test_treemap_labels_are_hit_testable(gui): + """Regression: hit-testing resolves to the topmost canvas item, so tiles + with a name label swallowed the tooltip/click exactly where the pointer + naturally lands.""" + show(gui, "Treemap") + gui.treemap_canvas.configure(width=820, height=520) + gui.update_idletasks() + gui._draw_treemap() + + labeled = [t for t in gui._tile_items.values() if t.w > 46 and t.h > 16] + assert labeled, "expected at least one tile large enough to be labelled" + + for tile in labeled: + hit = gui.treemap_canvas.find_closest(int(tile.x + 8), int(tile.y + 6)) + assert hit and gui._tile_items.get(hit[0]) is not None + + +def test_treemap_redraw_is_wired_to_resize(gui): + """The canvas must actually ask for a redraw when it is resized.""" + show(gui, "Treemap") + assert gui.treemap_canvas.bind(""), "no handler bound" + + +def test_treemap_resize_is_debounced(gui): + """A window resize fires a burst of events; they must collapse + into exactly one re-layout instead of one per event. + + Driven through the scheduler directly rather than by resizing a widget: + whether a toolkit emits for a given geometry change differs + between platforms, but the debouncing itself must not. + """ + show(gui, "Treemap") + + calls = [] + original = gui._draw_treemap + gui._draw_treemap = lambda: calls.append(1) + try: + for _ in range(40): + gui._schedule_treemap_redraw() + + assert calls == [], "redrew synchronously during the burst" + assert gui._treemap_redraw_after is not None, "no redraw was scheduled" + + deadline = time.time() + 5 + while not calls and time.time() < deadline: + gui.update() + time.sleep(0.02) + + assert len(calls) == 1, f"expected exactly 1 redraw for 40 events, got {len(calls)}" + assert gui._treemap_redraw_after is None, "pending redraw was not cleared" + finally: + gui._draw_treemap = original + + +def test_deletion_updates_model_without_the_original_widget(gui, sample_tree): + """Deleting is async; the user may switch views before it lands. The model + must still update and nothing may touch the destroyed widget.""" + show(gui, "Tree") + iid = next(i for i, n in gui.iid_to_node.items() if n.name == "notes.txt") + node = gui.iid_to_node[iid] + tree, mapping = gui._selection_context() + + before = gui.root_node.size + os.remove(node.path) # stand in for the worker thread + show(gui, "Treemap") # navigate away -> widget destroyed + + gui._apply_deletions([(iid, node)], [], tree, mapping) + + assert gui.root_node.size == before - node.size + assert node not in gui.root_node.children diff --git a/version.py b/version.py index 7ca5e2c..2f7a6b2 100644 --- a/version.py +++ b/version.py @@ -1,4 +1,4 @@ -VERSION = "3.0.0" +VERSION = "3.0.1" GITHUB_OWNER = "MrHakan" GITHUB_REPO = "FolderLens"