darktable-org / darktable-org/darktable

bauhaus: a combobox built from introspection stops at the first constant that repeats the last constant's value

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

Nobody has claimed this yet.

priority: low
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 at 080201ade6,
git rev-list --count HEAD..master is 0).

Summary

dt_bauhaus_combobox_from_params() fills an enum combobox by asking for the range of
constants that runs from the value of the first constant to the value of the last
constant (src/develop/imageop_gui.c:177-181):

dt_bauhaus_combobox_add_introspection(combobox, action, f->Enum.values,
                                      f->Enum.values[0].value,
                                      f->Enum.values[f->Enum.entries - 1].value);

dt_bauhaus_combobox_add_introspection() ends the range at the first constant whose
value equals that end marker, not at the last array element
(src/bauhaus/bauhaus.c:1857-1867).

C allows two constants of one enum to share a value: an alias kept for readability, or
a sentinel written as ..._LAST = ..._SOMETHING. If any constant before the last one
carries the last one's value, the loop returns there and every constant after it is
missing from the combobox. The user then cannot select those options at all.

This does not happen with any parameter enum in the tree today. I checked every
enum type used in an introspected params_t (see Evidence below); none repeats the
value of its last constant. The report is about a trap that is waiting for the next
module, not about a combobox that is wrong now.

Suggested fix: let dt_bauhaus_combobox_from_params() ask for "the whole list"
instead of naming an end value. Priority: minor.

Detail

What the two functions agree on

dt_bauhaus_combobox_add_introspection() takes a start value and an end value, and is
used in the tree to publish only part of an enum in one combobox: highlights.c:1259
and filtering.c:1963-1969 both name real sub-ranges. Its contract is therefore about
values, not positions, and stopping at the first match of the end value is a reasonable
reading of it for those callers.

  while(item->name && item->value != start) item++;
  for(; item->name; item++)
  {
    const char *text = item->description ? item->description : item->name;
    if(*text)
      dt_bauhaus_combobox_add_full(widget, Q_(text),
                                   DT_BAUHAUS_COMBOBOX_ALIGN_RIGHT,
                                   GUINT_TO_POINTER(item->value), NULL, TRUE);
    if(item->value == end) return TRUE;
  }
  return FALSE;

src/bauhaus/bauhaus.c:1857-1867.

Where the mismatch is

dt_bauhaus_combobox_from_params() does not want a sub-range. It wants the entire
array, and expresses that by naming the last element's value. That only works while the
values are unique. The array itself is well formed for the job: it is terminated by
{ NULL, 0 } and f->Enum.entries counts the real entries
(src/common/introspection.h:194-196), so "the whole list" is easy to express, but the
function has no way to say it.

The generator writes one tuple per enumerator, in declaration order, with that
enumerator's value (tools/introspection/ast.pm:975-985), so an alias or a duplicated
sentinel does reach the array.

Note that a constant without a $DESCRIPTION comment gets an empty description and is
skipped by the if(*text) test above, but it is not skipped by the item->value == end test on the next line. A hidden sentinel that repeats an earlier value still ends
the loop, and a hidden sentinel that repeats the last value still truncates the list.

Symptoms if an enum ever does this
  • the options after the duplicate never appear in the module's combobox
  • a stored history entry can still hold one of those values. dt_bauhaus_combobox_set_from_value()
    has a recovery path for that case: it re-adds the missing entry from the introspection
    data (src/bauhaus/bauhaus.c:2176-2185), so the value is not silently changed, but the
    option only exists on that one image. The recovery works because the widget was
    registered in darktable.bauhaus->combo_introspection when
    dt_bauhaus_combobox_from_params() passed a non-NULL action
    (src/bauhaus/bauhaus.c:1848-1849), and because the constant carries a
    $DESCRIPTION; for a constant without one the same call returns TRUE without having
    added anything, and the widget is then set to whatever entry happens to be last
  • an enum used by two modules, or by one module and by blending, misbehaves only in the
    combobox built by from_params(); the hand-written callers that pass their own end
    value are unaffected
Evidence that no current enum triggers it

I extracted every enum type named by a field of an introspected dt_iop_*params_t
struct under src/iop/, plus the blending enums, and computed the value of each
constant (resolving DT_DEMOSAIC_XTRANS, DT_DEMOSAIC_DUAL, the lcms INTENT_*
macros and the DT_NOISE_* macros by hand). Four enums contain duplicate values:

enum duplicates last constant
dt_develop_mask_combine_mode_t (src/develop/blend.h) DEVELOP_COMBINE_EXCL and DEVELOP_COMBINE_NORM_EXCL are both 0, DEVELOP_COMBINE_NORM_INCL is 2, DEVELOP_COMBINE_INV_EXCL is 1 DEVELOP_COMBINE_INV_INCL = 3, unique
dt_develop_blendif_channels_t (src/develop/blend.h) many, the Lab, RGB and Jz aliases DEVELOP_BLENDIF_OUTPUT_MASK = 0xF0F0, unique
dt_image_orientation_t (src/common/image.h) ORIENTATION_FLIP_HORIZONTALLY, ORIENTATION_FLIP_VERTICALLY, ORIENTATION_TRANSPOSE ORIENTATION_TRANSVERSE = 7, unique
dt_iop_ashift_linetype_t (src/iop/ashift.c) ASHIFT_LINE_MASK = 7 repeats ASHIFT_LINE_VERTICAL_SELECTED last constant is the duplicate, but this type is not a params field and gets no combobox

None of the enums that from_params() actually builds a combobox from repeats its last
value, so no in-tree combobox is truncated. flip.c stores a dt_image_orientation_t
in its params but builds buttons, not a combobox; the two blend.h enums are published
by blend_gui.c, which passes its own end markers.

Relation to the defpos report

This is a separate defect from
/tmp/dt-issues/open/22248-bauhaus-combobox-changed-state-compares-index-with-value/,
but the two are neighbours and should be read together.

What they share. dt_bauhaus_combobox_add_introspection() is what makes an entry's
position and its data value differ (src/bauhaus/bauhaus.c:1864 stores the enum
constant as the entry data, while dt_bauhaus_combobox_add() gives each entry a value
equal to its position, src/bauhaus/bauhaus.c:1903-1904). Both reports are about a
caller that uses one of the two numbers where the other is meant. The defpos report
compares a value with a position; this one uses a value to express "the end of the
array".

Why they are not one report. Different code, different fix, and neither fix removes
the other defect:

defpos report (22248) this report
site src/bauhaus/bauhaus.c:1433, :2120, :236-241 src/develop/imageop_gui.c:177-181 with src/bauhaus/bauhaus.c:1865
wrong number defpos, a value, compared against active, a position the end marker, a value, used to mean "last element"
visible today yes, channelmixerrgb's CAT tab no, no parameter enum repeats its last value
fix convert with dt_bauhaus_combobox_get_from_value() before comparing give the helper a way to say "to the end of the list"

Where they would meet. If an enum ever did truncate, the entries dropped from the
end would be missing while defpos still held one of their values. A reset would then
call dt_bauhaus_combobox_set_from_value() with a value that has no entry, and the
recovery path above would re-add it at the end of the list, at a position that matches
neither the enum order nor defpos. Scrolling a combobox in that state reaches the
second site of the defpos report (src/bauhaus/bauhaus.c:236-241), which reads
defpos as a position.

If they are fixed together, the renaming that report 22248 raises as a separate
decision (defpos to defval) is worth doing in the same pass, together with a comment
in src/bauhaus/bauhaus.h saying which of the combobox functions take a position and
which take a value. The confusion is in the API's naming, and both defects follow from
it.

Suggested fix

Give the helper a way to mean "to the end of the array", and use it from
dt_bauhaus_combobox_from_params(). Either:

  • a second entry point, for example dt_bauhaus_combobox_add_introspection_all(),
    sharing the body but running the loop to !item->name, or
  • a reserved end marker in the existing function, documented in the header, that makes
    the loop run to the end of the array

blend_gui.c:3069 already approximates the second form by passing -1 as the end
value, which works only because no blending enum has a negative constant. Whatever form
is chosen, it would be worth saying in src/bauhaus/bauhaus.h that start and end
are values and that the range ends at the first constant that matches end.

A smaller alternative is to leave the code alone and add a note next to the helper, and
in dev-doc/, that a parameter enum shown in a combobox must not repeat the value of
its last constant. That keeps a rule the compiler cannot enforce, so the code fix is
preferable.

Priority

minor

  • what the resulting code does: malfunction. Options a user needs would be absent
    from a module's combobox for as long as the enum stays that way
  • how it shows up: rare. It needs a parameter enum whose last constant's value is
    repeated earlier, which no module has today

malfunction x rare = minor.

Related

#22248 - but not the same

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 by reading dt_bauhaus_combobox_from_params() in src/develop/imageop_gui.c and dt_bauhaus_combobox_add_introspection() in src/bauhaus/bauhaus.c, then check the API declarations in src/bauhaus/bauhaus.h. Done means the whole introspected enum array can be represented without stopping at an earlier duplicate value, with the value-based range behavior documented for existing callers.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
desktop
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.