darktable-org / darktable-org/darktable

history: the change tooltip formats every array element with element 0's widget

Open
#22,254 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

priority: low scope: UI
Dominant language
C
Stars
13.1k
Forks
1.4k
Avg merge
22h 14m
Merged PRs (30d)
198

Description

Found by coding agents via static analysis, not verified by executing code.
Date: 2026-09-12.
Validated against commit c20f5e356615432b3ff46eba91e7b91dbc0f3e4d, branch
dev-doc/gui-data-sharing-pr (level with master, git rev-list --count HEAD..master
is 0).

Summary

The history panel's change tooltip formats a changed parameter the way the bauhaus
widget bound to it formats it. It finds that widget by address, and the address it
builds for an element of an array parameter is always element 0's.

src/libs/history.c:747-750:

#define CHANGE_TEXT_BAUHAUS(format, fieldtype)                            \
  _lib_history_bauhaus_text((void*)module->params + field->header.offset, \
                            d, module, *(fieldtype*)o, *(fieldtype*)p)    \
  ?: CHG_STR(format, d, *(fieldtype*)o, *(fieldtype*)p)

When _lib_history_change_text() walks an array it recurses once per element, with the
params pointers advanced by the element offset, but the descriptor it passes down is the
one descriptor the introspection generator emits for the array's first element, whose
header.offset is the offset of the array itself. The two pointers o and p therefore
address element i correctly, while the widget lookup address does not move off element
0.

A slider made by dt_bauhaus_slider_from_params(self, "Dmin[1]") is bound to that
element's own address, so the lookup matches element 0's widget, or no widget at all.

What a user sees today. negadoctor is the only module in the tree that binds
widgets to array elements, and within each of its arrays the three sliders are
configured identically, so elements 1 and 2 print the same text they would print after a
fix. The difference is visible on the fourth element, which has no widget of its own and
is formatted with element 0's anyway: a change of Dmin[3] from 1.0 to 0.0, which every
one of negadoctor's own presets makes, is printed as 100.0000% -> 0.0000% instead of
1.0000 -> 0.0000, because element 0's slider carries a factor of 100 and a % unit.

Suggested fix. Carry the offset accumulated by the array recursion into the lookup
address (details below).

Priority: minor: the outcome is cosmetic, one line of a tooltip formatted with
another element's unit and precision, and it is deterministic for the parameters that
reach it. Its main value is that the wrong address is a trap for the next module that
gives array elements different slider formats, where the same code would print a
plainly wrong number.

Why the lookup address is element 0's

The introspection generator gives an in-array field the array's offset

For float Dmin[4]; the generator emits two descriptors, and both take their offset
from the array name:

  • the element descriptor, type FLOAT, field name Dmin[0], size
    sizeof(((params_t*)NULL)->Dmin[0]), offset G_STRUCT_OFFSET(params_t, Dmin)
    (tools/introspection/ast.pm:1083)
  • the array descriptor, type ARRAY, field name Dmin, with the element descriptor as
    Array.field and the same G_STRUCT_OFFSET(params_t, Dmin)
    (tools/introspection/ast.pm:1099)

So the element descriptor's header.offset locates element 0 and nothing else. Code that
wants another element has to step from the array's own descriptor by the element size,
which is what the helpers in src/common/introspection.h do
(dt_introspection_access_array()).

The recursion moves the data pointers, not the descriptor

src/libs/history.c:744-745 computes the two value pointers from the descriptor:

void *p = (void*)params + field->header.offset;
void *o = (void*)oldpar + field->header.offset;

and the array case recurses with the params pointers advanced by the element offset
(src/libs/history.c:797-806):

for(int i = 0, item_offset = 0;
    i < field->Array.count;
    i++, item_offset += field->Array.field->header.size)
{
  char *description = g_strdup_printf("%s[%d]", d, i);
  char *element_text =
    _lib_history_change_text(field->Array.field, description, module,
                             (uint8_t *)params + item_offset,
                             (uint8_t *)oldpar + item_offset);

params + item_offset + header.offset is element i, which is right. The lookup
address in CHANGE_TEXT_BAUHAUS, however, is built from module->params, which the
recursion never advances, plus the same header.offset: element 0, for every i.

Widgets are bound to the element's own address

dt_bauhaus_slider_from_params() parses a trailing [i], looks the field up under the
name base[0], and adds the index itself (src/develop/imageop_gui.c:78):

offset = f->header.offset + param_index * sizeof(float);

then binds that address (src/develop/imageop_gui.c:109):

dt_bauhaus_widget_set_field(slider, (uint8_t *)p + offset, f->header.type);

_lib_history_bauhaus_text() compares its argument against exactly that stored pointer
(src/libs/history.c:721, dt_bauhaus_widget_get_field() at
src/bauhaus/bauhaus.c:1397-1401):

if(dt_bauhaus_widget_get_field(widget) == bhfield)

so an element whose index is not 0 never matches its own widget. What it matches is
element 0's widget, when element 0 has one.

What reaches this

The tooltip is built by _changes_tooltip_callback(), which diffs a history item's
params against the previous item for the same module, or against default_params for
the first one, and calls _lib_history_change_text() on the introspection root
(src/libs/history.c:900-928). Hovering a history entry is all it takes.

The mistake only shows up where element 0 of an array has a bound widget. Searching the
tree, the only module that binds anything to an array element is negadoctor:
Dmin[0..2], wb_low[0..2] and wb_high[0..2] (src/iop/negadoctor.c:855, :865,
:875, :917, :924, :931, :951, :958, :965). All three arrays are declared
float [4] (src/iop/negadoctor.c:71-77). The only other direct callers of
dt_bauhaus_widget_set_field() are in src/develop/blend_gui.c, on scalar blend
parameters.

Two cases follow:

  • elements 1 and 2 of each array: the wrong widget is found, but it is configured
    exactly like the right one. The Dmin sliders all get 4 digits, a factor of 100 and
    the % format (src/iop/negadoctor.c:856-858, :866-868, :876-878); the wb_low
    and wb_high sliders all keep what dt_bauhaus_slider_from_params() computed from the
    element descriptor's $MIN and $MAX, which is the same descriptor for every element.
    The printed text is therefore the same as it would be after a fix. Nothing is visible
    here today
  • element 3 of each array: it has no widget, so the correct result is the plain
    CHG_STR() fallback, %.4f. What happens instead is that element 0's widget is found
    and dt_bauhaus_slider_get_text() applies its digits, factor and unit
    (src/bauhaus/bauhaus.c:3555-3563). For Dmin[3] that is a factor of 100 and a %
    sign on a number that is not a percentage of anything. init() sets the default
    Dmin[3] to 1.0 (src/iop/negadoctor.c:386-389) and the module's presets set it to
    0.0 (src/iop/negadoctor.c:395, :410), so applying any negadoctor preset and
    hovering the resulting history entry shows it. The film-material area picker reaches
    it as well: apply_auto_Dmin() copies all four channels of the picked color into
    Dmin (src/iop/negadoctor.c:641)

Beyond the tree as it stands, the same lookup is wrong for any array element bound to a
widget with its own digits, factor, format or offset, and for any widget bound inside
element 0 of an array of structs. That is the part worth fixing: the case that hurts is
the next one somebody writes, not negadoctor.

Suggested fix

Give _lib_history_change_text() the offset the array recursion has accumulated, and
add it to the lookup address:

static gchar *_lib_history_change_text(dt_introspection_field_t *field,
                                       const char *d,
                                       dt_iop_module_t *module,
                                       gpointer params,
                                       gpointer oldpar,
                                       size_t array_offset)
  • the struct case passes array_offset on unchanged
  • the array case passes array_offset + item_offset
  • CHANGE_TEXT_BAUHAUS looks up
    (void*)module->params + array_offset + field->header.offset
  • the top-level call at src/libs/history.c:927 passes 0

The equivalent without a new parameter is to pass the history item's params block down
unchanged and derive the delta as (uint8_t *)params - (uint8_t *)params_root, which
needs the root pointer carried instead. Either way one value has to travel through the
recursion, because the element descriptor alone cannot say which element it is standing
for.

A smaller variant, if a signature change is unwelcome: format array elements with
CHG_STR() only and never take the bauhaus path for them. That is correct rather than
wrong, but it also drops the unit and digits from the Dmin[0..2] lines, which is a
loss.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/libs/history.c at _lib_history_change_text(), CHANGE_TEXT_BAUHAUS, and _changes_tooltip_callback(), then review the array recursion and the widget lookup address. Verify the behavior with negadoctor's Dmin[0..3] parameters and history tooltip, especially the fourth element. Done means array-element lookups use the matching widget when present and fall back to the plain formatting when no widget exists.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
desktop
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.