add progress meters/cancellation for history paste/compress/discard

[ralfbrown]

This gives the user feedback when operating on large numbers of images, and allows early cancellation (plus faster undo) if the user realizes that wasn't what they meant to do.

[ralfbrown]

Release notes (for this and the related #17138):

Most bulk image operations (such as copy/duplicate/remove, sidecar writes, and history paste/compress/discard) now display a progress meter while working and allow the operation to be cancelled before completion.

[TurboGit]

Great !

[TurboGit]

Just a thought, a paste and/or discard history can change the current collection depending on the filter. Is that working correctly here? Is changing the collection while in darkroom and editing picture not too disturbing for users?

[ralfbrown]

The only existing filters which could cause a collection change on change of history would be the "history" and "module order" filters, and those update properly. The behavior hasn't changed, making an edit or metadata change which causes an image to no longer match the active filter(s) has always made the image immediately disappear from the collection in both lighttable and darkroom views.

[TurboGit]

The behavior hasn't changed, making an edit or metadata change which causes an image to no longer match the active filter(s) has always made the image immediately disappear from the collection in both lighttable and darkroom views.

Yes, sure I do understand this. I have not tested but my fear was that this could be a bit more disturbing now... As an example:

• Copy history and paste to all image in collection
• It takes some time to process
• While the collection is updating you double click on an image and go to darkroom
• This image will receive at some point the paste on the history...

What is happening for this picture on the darkroom? What about the history module, is that updated? If so, the image could change in the darkroom while editing, could be disturbing, no?

• Let's say that the history has also made the image now out of the collection
• It will be removed from filmstrip, isn't this not too disturbing?

Those are my concerns at the moment. Not tested yet, I cannot see if those are real issues or not.

[ralfbrown]

Ah, after some more testing I now see what you mean - even though I've put the job on a "foreground" queue, it doesn't actually block until complete. I'll need to check on how to wait for completion before actually returning from the function if we want to avoid the scenario you mention.

[ralfbrown]

@TurboGit What I've figured out so far is that it won't be possible to do blocking processing without some serious rework elsewhere - in order to get the progress meter to display at all and enable cancellation, I have to run the Gtk event loop, at which point one can do everything that's possible in the user interface, including switching modes e.g. between darkroom and lighttable.

Once I move the actual sidecar writes into a background process (next on my TODO, hopefully this weekend), the period during which the paste/discard/etc. runs will be dramatically less, greatly reducing the abillity for it to surprise the user if they've gone on to other work. If even that is unacceptable, then the alternative is to drop the progress meter and cancellability and just show a busy cursor.

[ralfbrown]

Rebased, conflicts fixed, and refined to avoid flashing a progress meter when operating on fewer than five images at once.

[TurboGit]

Once I move the actual sidecar writes into a background process (next on my TODO, hopefully this weekend), the period during which the paste/discard/etc. runs will be dramatically less, greatly reducing the abillity for it to surprise the user if they've gone on to other work. If even that is unacceptable, then the alternative is to drop the progress meter and cancellability and just show a busy cursor.

We probably still need an history reload or cancelling the history background change when in darkroom for the image-id being edited.

[ralfbrown]

I don't know how to do that. Can anybody give me pointers?

[TurboGit]

I don't know how to do that. Can anybody give me pointers?

Depends on whether we go for history reload or history cancellation. The later is easier, just checking if darktable.develop->image_storage.id is equal to the target image. But I'm still not sure what we should do there, reload is probably a better goal, but at the same time having the history changing automatically during a dev in darkroom seems a very bad UX. So if we all agree that cancellation is better we can do this with a log message to ensure that the user is then properly notified that a copy/paste done earlier has been skip for the currently developed image.

@victoryforce @jenshannoschwalm : What do you think?

Others are also welcome to give feedback on this.

[ralfbrown]

I think skipping the imatge being edited in darkroom is the way to go. Given that the database update runs at 1000+ images per second, it would take an extremely large batch for the sitution to arise in practice, anyway.

[TurboGit]

I think skipping the imatge being edited in darkroom is the way to go. Given that the database update runs at 1000+ images per second, it would take an extremely large batch for the sitution to arise in practice, anyway.

Yes, that's my current thinking too.

[jenshannoschwalm]

Yes i think so too. A note via log that image under development was excluded from action would be enough and by far safer.

Also I seems pretty safe to assume, that such an action as we cover here would be a corner case and even more likely not a user wish.

[ralfbrown]

Adding the check for whether the image being updated is the image in darkroom was easy enough, but that disables the ability to paste/compress/reset history on the image you're actively editing.... I tried putting in a check for whether the darkroom image is the same one as when the bulk operation started, but that just makes darktable crash.

I'm about to start my busy semester, so if I can't figure it out this weekend, I'll make a new PR which simply adds the busy cursor to those operations instead of progress meters and background execution.

[TurboGit]

We may want to store the view which has initiated the copy (lighttable or filmstrip) and skip if initiated from lighttable? But IIRC we already skip paste on the current edited image in darkroom. Not sure is the same function is used by the background job, if it is we have nothing to do.

[ralfbrown]

No, when I added the skipping of the current image, it became impossible to paste/compress/discard even on just the image being edited, initiated from darkroom while looking at that image in the center view.

[ralfbrown]

A quick check shows no more nested transaction warnings or missed updates, but....

• it's basically impossible to do anything while the bulk operation is in progress - switch images or views, make edits to current image, etc. all mostly block until it's done
• Ctrl-Z undo feels rather brittle, since so much that one could do while the background job is churning away causes the undo records to be cleared

I saw a case where the image apparently did not refresh, and it was due to its history stack getting multiple entries duplicated without changing the active index - setting it to the last item caused a correct update.

More and more, I think that this will be a dead-end without substantial reorganization of the codebase, and we should stick to foreground operation. But the busy cursor can be enhanced for batches of more than 100 or so with a modal popup containing a progress bar and cancel button.

There are a couple of commits in this PR which should be cherry-picked even if we otherwise abandon it.

[TurboGit]

• Ctrl-Z undo feels rather brittle, since so much that one could do while the background job is churning away causes the undo records to be cleared

It sounds as if we are missing some locking and are having race conditions between the crawler and the background jobs pasting history.

[jenshannoschwalm]

You mean the back thumbs crawler here?

[ralfbrown]

I don't see how it could be the background crawler, since in all my testing the pref has been at the default "never". But visible thumbnails do get regenerated essentially instantaneously after their settings change since we are running a full event loop at full speed when the history operations are happening in the background.

[TurboGit]

So maybe not the background crawler, so possibly only the update of the thumbs as it was invalidated.

[ralfbrown]

#17301 now allows for cancellation of the bulk operation, so it has all the advantages over the status quo of this PR except for being able to make additional edits while the bulk operation is in progress (with all the problems that is causing).

[TurboGit]

I see your point, sorry for being extra careful here, having a new UX is always controversial for some.

@jenshannoschwalm @zisoft @kmilos @victoryforce and other devs, what's your view on this?

[ralfbrown]

Personally, I'd recommend #17301 over this PR at this time, unless someone figures out how to bypass all of the problems concurrent processing of histories and user actions triggers.

[jenshannoschwalm]

Personally, I'd recommend #17301 over this PR at this time, unless someone figures out how to bypass all of the problems concurrent processing of histories and user actions triggers.

I agree here. #17301 is far better than we have so far and with less unexpected behaviour.

[victoryforce]

Personally, I'd recommend #17301 over this PR at this time

+1

[dterrahe]

gtk_grab_add(darktable.control->progress_system.proxy.module->widget) (and corresponding gtk_grab_remove) to only allow input in the "background jobs" module. Since you might have other background processes going on (export) that you might want to cancel even when started before the blocking process, this seems preferable to a modal popup. Similarly, even when blocking inputs/new user commands, it would be possible that a lua routine running in the background starts a new batch?

Since keyboard action is captured by dt_shortcut_dispatcher no matter where it originates, and gtk_grab doesn't impact midi etc., a gtk_widget_has_grab check will need to be added there if you want to go that way. Maybe you still want to capture Esc; my first instinct when I do something disastrous. This would be useful when the left panel is hidden too.

[ralfbrown]

I'm afraid that I don't understand just how I'd implement your suggestion. Could you give an example for one of the operations in control/jobs/control_jobs.c? (perhaps based off the changes I made in 73b2bc3)

[dterrahe]

I'm not going to spend all the time you did to as thoroughly understand the jobs system, but I imagine you might want to add PROGRESS_BLOCKING (an extended version of PROGRESS_CANCELLABLE) and then in dt_control_add_job for those call gtk_grab_add (and dt_gui_cursor_set_busy). You'd have to gtk_grab_remove in dt_control_job_dispose (there might be better places?) but presumably that's not running in the gui thread (?) so you'd have to use g_idle_add (or a signal).

For extra polishing, dt_gui_cursor_set_busy could be disabled when the cursor enters darktable.control->progress_system.proxy.module->widget and reenabled when leaving. You'd have to wrap the box (in backgroundjobs.c::gui_init) in an eventbox, gtk_widget_add_events(eventbox, GDK_ENTER_NOTIFY_MASK | GDK_LEAVE_NOTIFY_MASK) and connect both enter-notify-event and leave-notify-event signals to the same routine and switch based on event->type == GDK_ENTER_NOTIFY. See _resize_wrap_enter_leave for a slightly more complicated example.

As with everything gtk, the unexpected snags will only show up when you think you're 99% done...

[ralfbrown]

All of that appears to be above my current Gtk skills.... (the "extra polishing" for sure) Especially given that I have little spare time for the next month and none from then until Christmas.

[dterrahe]

The polishing is very non-essential and the rest is not a gtk issue but a job management issue (i.e. where should dt_gui_cursor_set_busy be called and where cleared after the job finishes, making sure that it is in the gui thread; the grab add/remove could simply be added to dt_gui_cursor...)

[dterrahe]

turns out you don't need to monitor enter/leave events anyway; you can simply set an overriding cursor for the progress bars.
in '_added_gui_thread', after gtk_widget_show(params->self_widget); insert:

  GdkCursor *cursor = gdk_cursor_new_for_display(gdk_display_get_default(), GDK_LEFT_PTR);
  gdk_window_set_cursor(gtk_widget_get_window(params->instance_widget), cursor);
  g_object_unref(cursor);

[dterrahe]

Could you give an example for one of the operations in control/jobs/control_jobs.c?

Submitted a PR to your PR in ralfbrown#2
My limited testing shows this working to suppress all mouse, keyboard and midi input. The only keypresses not intercepted are "hold" keys (like h or preview, which act on press rather than release.) Also, it seems safer not to block lua calls to dt.gui.action because that could easily break scripts. We could block them (but only all of them) so maybe @wpferguson wants to opine.

[wpferguson]

I'll look at it later today...

[ralfbrown]

@dterrahe Looks good, added to this PR.

[TurboGit]

@ralfbrown : Tested but seems buggy, I selected 600 pictures and double clicked on a style to apply it. The UI did freeze, no busy cursor, nothing changing and after some time the OS pop-up asking me if I wanted to "force quit" or "wait" for darktable process.

[ralfbrown]

I've discovered that the busy cursor doesn't get displayed if the mouse is in one of darktable's expandable text boxes (such as the list of styles in the styles modules, but also the list of recent collections, etc.). If the mouse is anywhere else when dt_gui_cursor_set_busy() gets called, the busy cursor shows up fine. So you'll see it when clicking the Apply button in the styles module, just not when double-clicking a style name since you'll be in one of those text boxes.

It isn't a case of that widget overriding the cursor while inside, because you can move the cursor out of the text box while the styles are being applied without getting the busy cursor.

[ralfbrown]

dt_ui_resize_wrap in gtk.c sets multiple signal handlers which force the mouse pointer back to the default cursor. When looking at the function used to change the cursor, I found that there's already a mechanism for disallowing cursor changes.

[TurboGit]

Looks good now, tested with 345 images and seems ok. Thanks!