darktable-org / darktable-org/darktable
history: the change tooltip formats every array element with element 0's widget
Nobody has claimed this yet.
- 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 nameDmin[0], size
sizeof(((params_t*)NULL)->Dmin[0]), offsetG_STRUCT_OFFSET(params_t, Dmin)
(tools/introspection/ast.pm:1083) - the array descriptor, type
ARRAY, field nameDmin, with the element descriptor as
Array.fieldand the sameG_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. TheDminsliders all get 4 digits, a factor of 100 and
the%format (src/iop/negadoctor.c:856-858,:866-868,:876-878); thewb_low
andwb_highsliders all keep whatdt_bauhaus_slider_from_params()computed from the
element descriptor's$MINand$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
anddt_bauhaus_slider_get_text()applies its digits, factor and unit
(src/bauhaus/bauhaus.c:3555-3563). ForDmin[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_offseton unchanged - the array case passes
array_offset + item_offset CHANGE_TEXT_BAUHAUSlooks up
(void*)module->params + array_offset + field->header.offset- the top-level call at
src/libs/history.c:927passes 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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