FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

navigation, darkroom: the thumbnail composes once per state, a same-size configure does nothing by aurelienpierre · Pull Request #1395 · aurelienpierreeng/ansel · GitHub

  1. aurelienpierreeng
  2. ansel

Repository navigation

navigation, darkroom: the thumbnail composes once per state, a same-size configure does nothing - #1395

Merged
Jiyone merged 3 commits into
masterfrom
navigation-redraw
Oct 29, 2026
ConversationCommits (3)ChecksFiles changed
Merged

navigation, darkroom: the thumbnail composes once per state, a same-size configure does nothing#1395
Jiyone merged 3 commits into
masterfrom
navigation-redraw

Conversation

aurelienpierre commented Sep 9, 2026

Copy link
Copy Markdown
Member

Reported after #1394: the navigation thumbnail repaints on every pointer hover over a notebook tab or a panel edge. Two ways a hover over an unrelated widget reaches it, both closed; the darkroom cannot be driven here, so which of the two fires on which widget is for the tester to read off the trace below.

The grip over the navigation area

The resize grips are translucent GtkEventBox overlay children floating over the area they resize. GTK3 composites overlay children over the main child, so invalidating the grip (its :hover state, its cursor, its own redraw on enter and leave) exposes the navigation area under it with a clip of the grip's strip, and the area's draw handler runs. It answered every expose with a fresh full-size surface, a background render, the thumbnail copy, a pango layout for the zoom label and a blit.

The picture is now composed once per state, keyed on the preview frame's hash, the widget's size, the image, the viewport state and the preview's plan, and an expose with an equal key is one blit. A miss on the preview cacheline keeps the previous picture instead of leaking the surface and its context, as the early return used to.

A configure that changes nothing

A layout pass allocates the centre again and GTK delivers it a configure-event; dt_dev_configure_real() re-planned both pipes, recomputed the thumbnail size and raised DT_SIGNAL_CONTROL_REDRAW_ALL (every panel, the navigation, the scopes) whether or not anything changed. The theme gives every hovered tab and button font-weight: 500, which changes the widget's size request, so a layout pass is every hover. The viewport box the pipes plan from is published by a setter that already reports whether it changed; when it did not, the configure returns. Only the first configure and a real size, border or zoom change get past it.

What to look at

  • -d perf: [navigation] redraw: cached|composed, clip WxH at x,y of WxH on every expose. Hovering a grip should print cached with a strip-shaped clip; a tab hover should print nothing at all now. If it still prints, the clip says whether it is an overlay expose (strip) or a request of the module's own (whole widget).
  • -d dev: [pixelpipe] Darkroom requested … unchanged on a same-size configure; a hover that used to re-plan the pipes shows up here.
  • ansel -d signal --d-signal-act raise,print-trace --d-signal DT_SIGNAL_CONTROL_REDRAW_ALL backtraces every whole-window redraw request, for whatever remains.
  • Behaviour to re-check, since the configure no-op is on the darkroom's entry path: enter and leave the darkroom, switch images of different sizes, resize the window, toggle ISO 12646, move the border-size slider, zoom. Each must still re-plan the pipes and repaint.

Validation

GCC, clang build-warnsweep and build-nofeatures; the tools/check_* gates; the unit tests (the six LensSerious tests report "Not Run" in this tree, as before, their binaries are not built here); the image smoke test. The GUI itself is not verifiable here.

Docs: doc/darkroom-redraw.md (a new section on both), CLAUDE.md (the overlay rule and the trace).

🤖 Generated with Claude Code

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

aurelienpierre added a commit that referenced this pull request Sep 9, 2026
navigation: answer SonarCloud's finding on #1395
c3a9b3f 
The picture key reads the dev and writes nothing: a pointer to const.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

sonarqubecloud Bot commented Sep 9, 2026

Copy link
Copy Markdown

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

aurelienpierre and others added 2 commits October 29, 2026 15:51
navigation, darkroom: the thumbnail composes once per state, a config…
91107c4 
…ure that changes nothing does nothing

Reported: the navigation thumbnail repaints on every pointer hover over a
notebook tab or a panel edge, widgets connected to it neither logically nor
programmatically. Two ways such a hover reaches it, both closed:

- The resize grip floating over the navigation area is a translucent overlay
  child, so GTK repaints what lies under it every time the grip is hovered --
  its :hover state, its cursor, its own queue_draw on enter and leave -- and
  the area's draw handler ran with a clip of the grip's strip. It answered
  with a fresh full-size surface, a background render, the thumbnail copy, a
  pango layout for the zoom label and a blit, on every expose. The picture is
  now composed once per state -- keyed on the preview frame's hash, the
  widget's size, the image, the viewport state and the preview's plan -- and
  an expose with an equal key is one blit of the composed surface. A miss on
  the preview cacheline keeps the previous picture instead of leaking the
  surface and its context, as the early return used to. `-d perf' prints
  `[navigation] redraw: cached|composed, clip WxH at x,y of WxH', which also
  says who asked: the whole widget is a request of the module's own, a strip
  along an edge is the grip.

- A layout pass allocates the centre again, GTK delivers it a configure-event,
  and dt_dev_configure_real() re-planned both pipes, recomputed the thumbnail
  size and raised DT_SIGNAL_CONTROL_REDRAW_ALL -- every panel, the navigation,
  the scopes -- whether or not anything changed. The theme gives every hovered
  tab and button a heavier font weight, which changes the widget's size
  request, so a layout pass is every hover. The box the pipes plan from is
  published by a setter that already reports whether it changed; when it did
  not, the configure now returns, `-d dev' printing the request with
  `unchanged' appended. Only the first configure and a real size, border or
  zoom change get past it.

The signal layer's own trace is the tool for the next such report:
`ansel -d signal --d-signal-act raise,print-trace --d-signal
DT_SIGNAL_CONTROL_REDRAW_ALL' backtraces every raise; a redraw with no signal
behind it is GTK's own, an overlay exposed or an allocation that moved.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
navigation: answer SonarCloud's finding on #1395
7d81941 
The picture key reads the dev and writes nothing: a pointer to const.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Jiyone force-pushed the navigation-redraw branch from c3a9b3f to 7d81941 Compare October 29, 2026 17:53

Jiyone commented Oct 29, 2026

Copy link
Copy Markdown
Member

Rebased onto fc54f74b5d (master, 2026-10-29) and force-pushed: c3a9b3fa56 → 7d819416b4. The code applied cleanly; the conflicts were in the docs only:

  • doc/darkroom-redraw.md: master's hit-test paragraph kept, this PR's new section after it.
  • CLAUDE.md: master's version taken. CLAUDE.md now holds the rules only, so the two paragraphs this PR added there ("A widget under a translucent overlay is repainted whenever the overlay is…") are no longer in the branch. They need a home in doc/ (doc/gtk-patterns.md fits).

Build (clang-19, RelWithDebInfo) green; navigation.c and develop.c compile with -Werror; the blocking gates pass (check_layering, check_module_boundaries, check_return_types, check_list_order, pragma_once_to_guards --verify, check_conditional_includes, check_statelessness, check_unused_includes --changed: 0 introduced).

Measured in the GUI

Same config copy, same image, -d perf -d dev -d signal --d-signal-act raise --d-signal DT_SIGNAL_CONTROL_REDRAW_ALL, master fc54f74b5d against this branch, pointer moved by hand, the log cut at the end of each phase. Navigation widget 385×179, device scale 1.

master this branch
hovering the module-group notebook tabs, ~60 s 0 configure, 0 REDRAW_ALL, 3 navigation redraws 0 configure, 0 REDRAW_ALL, 6 navigation exposes, all cached
entering and leaving the grip under the navigation, ~10 times 29 navigation redraws, took 0.001 secs each 22 exposes, all cached, took 0.000 secs
an actual composition — 2 ms with no preview, 13 ms (39 ms CPU) when the preview lands

Then, on this branch: a window resize (372 configures, every one re-planned), ISO 12646 on and off, the margin slider, zoom, switching between landscape and portrait images, a lighttable round trip. All repainted and re-framed correctly. The configure guard fired 4 times, all on image switches, where _darkroom_image_loaded_callback() has already run dt_dev_pixelpipe_rebuild_all() and dt_dev_get_thumbnail_size(); each was followed by a fresh main backbuffer. studio_capture (read, not run) does the same rebuild before it configures.

So:

  • The cache works: every expose that follows no state change is now one blit. What it saves is about 1 ms per expose at this size.
  • The mechanism the configure guard was written against did not reproduce: hovering tabs produced no configure at all, on either build. The guard is correct on every path above; it is just not what stops anything that could be observed here.
  • The leak fix stands on its own: master's two early return TRUE in _lib_navigation_draw_callback() leak the surface and its cairo context every time the preview cacheline cannot be read.

Claims in the description and the doc that the measurement contradicts

  1. ansel -d signal --d-signal-act raise,print-trace --d-signal … prints the usage and exits 1: each --d-signal-act takes one value, compared with strcmp (src/darktable.c:1122). It has to be --d-signal-act raise --d-signal-act print-trace, and print-trace is compiled in for GCC builds only (src/CMakeLists.txt:773); a clang build prints [signal] print-trace not available, skipping.
  2. "A whole-widget clip is a request of the module's own, a strip is the grip": 15 of the 22 exposes of the grip phase had the whole widget as their clip. *:hover { font-weight: 500; } (data/themes/ansel.css:1744) restyles the navigation area itself when the pointer enters or leaves it, which crossing onto the overlay grip does, and that invalidates the whole area; the grip's own strip merges into it. The one strip that did show, 5x179 at 380,0, is the left panel's edge grip.
  3. "A tab hover is a layout pass that reaches the centre's configure-event": not observed, 0 configures in ~60 s of tab hovering on both builds.
  4. The new section of doc/darkroom-redraw.md carries no date or commit.

Minor, in the code

  • gui_reset() frees image_surface but not composed.
  • The key covers no style input: after a theme switch, or a move to a screen of another scale, the previous picture stays until the next state change.
  • The function-scope statics of the composition (image_hash, wd, ht, scale, imgid) now sit next to the per-instance d->composed_*; and there is a double blank line where the blit comment was.

The doc corrections are not in this push: it is the rebase only.

doc: the navigation section says what was measured, and the overlay p…
9578eef 
…attern finds a home

The rebase onto master took master's CLAUDE.md, which now holds the rules only, so the two paragraphs this branch had added there are gone. The overlay one moves to doc/gtk-patterns.md as its own dated section; the layout-pass one is folded into doc/darkroom-redraw.md, where it is recorded as not reproduced.

The navigation section of doc/darkroom-redraw.md is dated and carries the numbers measured on 7d81941 against master fc54f74. It corrects three claims the measurement contradicts: a whole-widget clip is not only the module's own request, since the area's own :hover restyle gives one too; hovering the notebook tabs gives no configure of the centre on either build; and --d-signal-act takes one value per flag, print-trace being compiled for GCC builds only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

sonarqubecloud Bot commented Oct 29, 2026

Copy link
Copy Markdown

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Jiyone merged commit 5f49c8b into master Oct 29, 2026
22 checks passed
Jiyone pushed a commit that referenced this pull request Oct 29, 2026
navigation: answer SonarCloud's finding on #1395
2804f11 
The picture key reads the dev and writes nothing: a pointer to const.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Jiyone deleted the navigation-redraw branch October 29, 2026 20:51
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

Ansel development management
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL