Additional button to remove the star rating of an image - #21199
Closed
deekayhd wants to merge 753 commits into
Closed
Additional button to remove the star rating of an image#21199deekayhd wants to merge 753 commits into
deekayhd wants to merge 753 commits into
Conversation
deekayhd
changed the title
Member
|
But then should we keep the double-click on 1 star to remove the rating? Note that this was done to avoid too much encumbered thumbs especially when using small ones. |
Contributor
Author
I would keep the double-click, because some may be used to it and get confused.
I do not think that this is a problem. When the thumb gets too small, the stars are not visible, anyway. |
Contributor
Author
Contributor
|
@deekayhd hey hello! Just a heads up, many things are changing with all this My last #21745 PR (still not in There is also this guide which can become handy: https://docs.gtk.org/gtk4/migrating-3to4.html If using an LLM / AI coding tool, here is some helpful tips that I use in my workflow:
I believe these guidelines are universal and would help plenty of other developers as well when it comes to merging new PRs in this phase. Of course, if you don't use any LLMs, well, you can always inspect the code yourself. It's the same process, but possibly a bit slower, although some people are pretty good at this! CC to @TurboGit because this message could be useful for other people as well |
Contributor
Author
|
@TurboGit Now that 5.6.1 is out I would like to ask you again, if you consider this PR for the next release. I understand that soem adjustments are necessary due to the GTK4 migration, but I do not want to spend the effort if this change is not worth to be included in 5.8. |
Member
|
@deekayhd : Indeed I'm not 100% convinced by this extra control and not many people have commented. Indeed I would add this into the bottom bar only for now. That would avoid another icon on the thumbs which are already crowded. A good compromise as I see it. |
- dt_shortcut_key_press() now flushes a still-armed delayed release before the hold takes over _sc: the pending pass requires c->key == s->key, so left armed it would resolve against the hold key and re-run the hold action, silently disengaging the hold while the key is still held. A same-key re-engage inside the window is that pending pass's own double-press check and is only cancelled. - dt_shortcut_key_release() cancels a still-armed delayed release before scheduling the hold cleanup, mirroring the ordinary key release, so g_timeout_add() cannot orphan a live source handle. - _delay_for_double_triple()'s stuck-key early return drops the _hold_release_pending flag and the stale hold key, or the next ordinary key release would silently skip its own dispatch. - _key_release_delayed() restores the identity of a still-engaged hold key when _pressed_keys is empty but _hold_keys is not, keeping move/scroll routing alive for the held key.
- the CAPTURE-phase gesture claims selectively (modifier presses, single-click mode, MONTH rule, folder/filmroll right-click) instead of unconditionally, so a plain click reaches the treeview's internal gesture again: single-click expander toggles, focus grab and cursor placement work as before the migration. - a plain primary double-click on the expander arrow claims only the second press and skips the module's own toggle: the internal gesture toggles on the first release, giving exactly one toggle with no expand/collapse flicker, while longer click bursts keep responding. - shift+ctrl+click on a folder/filmroll row activates again, making the folders<->filmrolls view switch reachable.
…close GTK3 synthesizes a fake enter crossing (GDK_CROSSING_GTK_UNGRAB) on every widget a grab was shadowing when the grab ends -- menus, popovers, modal dialogs -- so the button's internal enter handler marks the pointer as inside and the button keeps its hover state even after the pointer has left, only clearing on the next genuine crossing. Connect a grab-notify handler on dtgtk buttons that drops the stale hover/pressed flags when the grab ends and the pointer is not over the button. The lib module reset button additionally ran its action from a custom gesture's press, so the confirm dialog's grab split the button's press from its release (the press was processed after the dialog closed, the release was consumed by the dialog's grab), leaving the button stuck in its pressed state; wiring the action to the button's own clicked signal keeps the press/release cycle intact, and also avoids the cancel forwarding in dt_gui_connect_click() firing the reset twice.
The shortcut machinery grabs the pointer on every key press (gdk_seat_grab()); the synthetic crossings that the grab/ungrab dance generates reach the thumbtable and culling leave handlers, which cleared mouse_inside unconditionally -- the darktable-org#21734 hovered-image fix (2f24f86) only guarded the mouse_over_id clear. With mouse_inside FALSE, _cache_update_hover() skips the "hovered image is inside the selection" check (column 1) and acts on the hovered image only, so rating, rejecting, moving and color labels hit just the hovered thumbnail instead of the whole selection for every shortcut press until the pointer moves. Clear mouse_inside only on real leaves (same guard as mouse_over_id: not an inferior/grab crossing, and not while the pointer is grabbed), and restore it in the enter handler so the state self-heals.
The per device tracking of cl_mem allocation errors did not work as intended for long so it got removed. This is due to "lazy cl_mem allocation" as almost all drivers/hardware do. In short - the actual cl_mem is not allocated immediately but on first use by a kernel/cl function so all alloc tests like `cl_mem == NULL` almost never returned NULL as an error condition. This commit adds clEnqueueMigrateMemObjects() whenever cl_mem is allocated, this enforces the cl_mem being prepared on the device so we have a fair chance errors are reported. As there might be a perf penalty doing so we currently use a cli switch `--opencl-migrate` to enforce. dt_opencl_memory_statistics() and dt_opencl_get_device_info() are internal so not exposed and made static. Some improved logs
…ments in exposure, channelmixerrgb
In some cases it's important to know the darktable version. Without this, for example, downloading AI models will not work.
Enable scene refered editing of details in images. The core principle of the module is to modifiy the log difference between a luminance estimate and its low pass version filtered using the exposure invariant guided filter (eigf, same as in tone equalizer). Its in this way easy to target different detail sizes. Work based on initial idea by Co-authored-by: bjoern <wilecoyote2015@gmail.com> And further efforts by Co-authored-by: Christian Bouhon <christian.bouhon@outlook.be> Co-authored-by: Dan Zwell <devel@zwell.net>
shouldn't the color stuff (especially for graphs and colorlabels) move to gtk.h/.c and use dt_gui_gtk_set_source_rgb(a)? After those are renamed to dt_gui_set_color/_alpha.
…ll_should_pan() Mouse wheels no longer pan: GTK4 can deliver wheel scrolls as smooth events, so the old 'smooth without ctrl' check made the wheel pan in the culling layouts at 100% zoom instead of navigating to the next or previous image (issue darktable-org#21765, regression from f5d6acb). A wheel must always scroll/navigate; only a real touchpad swipe pans. Add a shared helper, dt_gui_scroll_should_pan(), that a scroll event must satisfy to be treated as a two-finger pan: the touchpad-gestures preference, a smooth non-stop scroll, no ctrl modifier, and a touchpad source device (or the device that just produced a pinch/swipe gesture, as some touchpads report the follow-up scroll stream from a different device). On macOS/Quartz the built-in trackpad reports as a mouse, so any non-ctrl smooth scroll pans there. Use the helper in the main view _scrolled(), the culling layouts and the darkroom second window, replacing the three duplicated smooth/ctrl checks. The second window had the same regression as the main view.
Zoom on up/left, out on down/right, everywhere: the darkroom, the culling layouts and the lighttable zoomable/filemanager modes. The direction is resolved from the GDK scroll direction rather than the sign of the raw delta. macOS delivers shift+wheel (which becomes left/right scroll) with inverted horizontal delta signs for some Bluetooth mice, but GDK still reports the correct LEFT/RIGHT direction, so up/left==zoom-in, down/right==zoom-out works there too. Previously the culling and zoomable paths computed 'right==up==zoom-in' from the delta sign, so ctrl+shift+wheel zoomed the wrong way (up zoomed out, down zoomed in). Smooth scrolls carry no direction, so their dominant delta is used with the same sign convention (negative==up/left==zoom-in). A shared _scroll_zoom_delta() helper replaces the duplicated logic in the culling layouts; the zoomable/filemanager path keeps its integer-step handling but uses the corrected delta sign.
- bauhaus: gtk_get_current_event() may return NULL for synthetic motions; guard the button-press opentime read (was a NULL deref) and fall back to dt_key_modifier_state() in _widget_motion where the event is only used for its modifier state. Free the event when present. - culling: add dt_culling_destroy() to cancel a pending deferred-zoom finalize timer, disconnect the control-signal connections that use the table as user_data, and destroy+unref the widget (tearing down the event-controller callbacks). Use it from the lighttable view cleanup so neither culling nor preview tables leak their timers and signals.
…ared helper Unify the zoom direction across the culling layouts, the lighttable filmstrip/zoomable modes, the darkroom main view and the second window, and settle the horizontal axis consistently (issue darktable-org#20843). With the shift modifier held, the OS turns a vertical wheel step into a left/right scroll on every platform (macOS, Windows, X11), and the event still carries the shift state. Such a scroll is really a rotated wheel step, so LEFT keeps meaning "wheel up": it zooms in, preserving the wheel's up==in/down==out muscle memory. Without shift (wheel tilt or a two-finger swipe), RIGHT is the increase direction and zooms in, LEFT zooms out -- the convention documented in darktable-org#20843. Discrete events are resolved from the normalized GDK direction rather than the raw delta sign, whose polarity is not canonical across platforms; smooth (fractional) scrolls use their dominant delta with the same convention. A single dt_gui_scroll_zoom_delta() helper in src/gui/gtk.c replaces the duplicated per-view logic (culling, darkroom second window, main-view fallback) and the filmstrip/zoomable sign derivation, which previously disagreed with the darkroom for horizontal scrolls (and still disagreed with the wheel when shift was held).
…ion widget The left-panel navigation widget zoomed with the same pre-helper pattern (-delta_x dominant axis, no shift handling) that the culling, darkroom and thumbtable paths had: without shift its behavior already matches dt_gui_scroll_zoom_delta() (right/tilt zooms in), so this is a no-op for those inputs, but shift+wheel (an OS-rotated wheel step delivered as a left/right scroll) now zooms in on up like everywhere else instead of zooming out.
test_hdr_alignment_internal was converted to C++ in 330661f but still includes win/main_wrapper.h. The wrapper relied on C pointer conversions and the test used a main signature that conflicts with the wrapper declaration in C++, preventing Windows builds with BUILD_TESTING enabled. Use g_new0() for a typed argv allocation, explicitly treat Windows wchar_t input as GLib UTF-16, and give the C++ test the standard argc/argv signature. This lets the full Windows test build compile again.
Not that such images are expected, but let's handle such situations properly. Before this change, when reading such a file, we would have garbage values as dimensions and would end up trying to allocate memory of potentially very large size. So we risked triggering an OOM killer in such a, albeit unlikely, case.
The TIFF specification notes that the field is mandatory and applications must not guess or default the value if the tag is missing; the file is considered invalid or non-compliant if it is omitted.
Each tile's seam strip spans sensor_O rows and columns past the tile edge, but the flush loops bounded only the horizontal strip's columns. A tile row ending within sensor_O of the image bottom then wrote past the end of cfa_out. Whether that happens depends on where the last tile row falls, so it crashed on a 44MP 8152x5432 sensor and not on a 24MP one, identically on every platform. Clamp both flushes to the working area, matching the bound the horizontal strip's columns already used. Fixes darktable-org#22023
restore_raw_linear.c is a near-copy of the bayer path and shares its seam-bounds bug: the accumulation bounds are not clamped to the image, the vertical flush does not clamp its columns, and the horizontal flush does not clamp its rows. The last two write past the end of rgb_out; the middle one also wraps columns into the next row, overwriting output that was already correct. Same geometry as darktable-org#22023, but nothing reported it, so this is fixed by inspection rather than from a reproducer. Related: darktable-org#22023
The guard admitted any tile larger than 2 * O, but two adjacent tiles' seam ramps only sum to 1 from 4 * O upward, so sizes in between seamed the output and left both flushes' start bounds outside the image. Raise it to 4 * O. Every start bound then sits at least 2 * O inside the working area, so the four flush sites are safe by construction rather than by clamping each one. Only a model declaring a tile size in 65..127 could reach this; the shipped raw denoise model uses 512.
…22042) * opencl: make the bilinear resampling kernels reusable The buffer bilinear resampler used to down- and upscale the working buffers of a guided filter lived inside colorequal.cl, so any other module needing it had to copy the three kernels and their channel-count dispatch. Its CPU counterpart, interpolate_bilinear() in common/fast_guided_filter.h, is already shared by several modules. In preparation for implementing an OpenCL code path for tone equalizer, those kernels are moved into their own program and exposed as dt_interpolate_bilinear_cl(), following the dt_gaussian_*_cl() pattern: a small global holding the kernels, initialised once in dt_opencl_init(). colorequal now calls that instead of carrying its own kernel handles. basic.cl keeps its own interpolate_bilinear(): it works on image2d_t rather than on buffers, so it is a different memory model rather than a copy of this one.
…raster dt_develop_blend_process_cl() skipped the device-to-host readback of the final mask when this module's own mask came from a raster mask, on the reasoning that such a mask is built on the host and uploaded, so the host buffer already holds it. That only holds while nothing touches it on the device afterwards. The mask post-processing immediately above -- feathering, blur, tone curve -- reads and writes dev_mask. With any of those active the host buffer is still the unrefined mask, and that is what gets published: a module that consumes a raster mask, refines it, and is itself a raster source for a later module hands every downstream consumer a different mask than it blended with, and a different one than the CPU path publishes for the same edit. Found by replaying harvested mask edits on both pipes and comparing. Bucketed by mask mode and by active post-processing, over 42,078 edits: raster with any post-op diverges in 61 of 61 cases; raster with no post-op in 0 of 590; drawn masks with the same feathering, blur and tone curve in 0 of 6,747 -- which rules out the post-processing implementations themselves and leaves the publication step. On a corpus of every distinct raster configuration found, this takes the divergent edits from 42 of 76 to 0, and the worst CPU-vs-OpenCL gap from 0.940 to 0.000153. CPU renders are unchanged; OpenCL renders change to match them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@TurboGit @LebedevRI I think I made a stupid mistake when I rebased this branch on master, and I do not understand why there are so many changed files, as I effectively only changed one. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a proposal for issue #21127 .
Currently, the only way to remove the star rating of an image is either by using the shortcut 0 or by clicking the first star in a toggle fashion. When more than one star has been set for an image, you have to click the first star twice. This is not very intuitive.
With this PR two new dedicated buttons are introduced to remove the star rating of an image in the same fashion as setting the rating.
At the bottom of the window
and on the thumbnail overlay
closes #21127