GNOME Bugzilla – Bug 732567
Run with closed Window (was: Restore Disk Image completes only partially if gnome-disks is closed)
Last modified: 2018-05-24 10:29:55 UTC
* Begin writing a disk image to a device using Restore Disk Image * Close gnome-disks Expected: either a warning message appears to tell me that the operation will be canceled, OR the operation continues in the background and I can see the progress when I reopen gnome-disks Actual: it looks like it just gets canceled without any warning I did this accidentally once (not sure what possessed me, I should have known better), nearing the end of a long write, and was not happy.
Confirming this also on v3.14.0!
Created attachment 351898 [details] [review] Prompt to cancel jobs when closing Closing Disks used to silently discontinue running jobs like restoring or creating disk images. Now a message dialog warns that closing Disk will cancel the jobs. Also they are canceled explicitly if the user decides to close Disks.
Review of attachment 351898 [details] [review]: Thanks for the patch! ::: src/disks/gduapplication.c @@ +84,3 @@ + { + GHashTableIter iter; + GList *local_jobs, *jobs_to_cancel = NULL, *l; nitpick: I would reorder the declarations to move the initializer to the end, or add it on the new line...: GList *l, *local_jobs, *jobs_to_cancel = NULL; @@ +88,3 @@ + g_hash_table_iter_init (&iter, app->local_jobs); + + while (g_hash_table_iter_next (&iter, NULL /* object*/, (gpointer) &local_jobs)) nitpick: /* object*/ -> /* object */ @@ +101,3 @@ +gdu_application_should_exit (GduApplication *app) +{ + GtkDialogFlags flags = GTK_DIALOG_DESTROY_WITH_PARENT; nitpick: Why not use this directly in the gtk_message_dialog_new? @@ +113,3 @@ + _("Cancel running jobs?")); + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + _("Closing the application now will cancel the running jobs.")); I think that this needs better wording, because you are asking on canceling of jobs and then you have two options cancel and ok, but the cancel means canceling of dialog, not the jobs, which is pretty confusing to me... Try to find what other applications do in similar situation. @@ +121,3 @@ + return FALSE; + + cancel_jobs (app); Hmm, it seems to me that this is probably not needed, because it more or less just calls gdu_application_destroy_local_job, which will be called from gdu_application_finalize anyway... what do you think? @@ +377,3 @@ gpointer user_data) { GduApplication *app = GDU_APPLICATION (user_data); nitpick: I would add an empty line between declaration and the following code since we are going to enlarge the code...
Created attachment 351956 [details] backtrace of segfault after click to ok It segfaulted once when I clicked to ok in the new dialog. Please take a look at the attached backtrace...
Created attachment 351987 [details] [review] Prompt to stop jobs when closing Thanks, trying to cancel or destroy the jobs during the close phase is indeed a bad idea and triggers also assertions to fail in some cases. For that to work it would need to first reject the delete-event, wait for all job cancel signals to be processed and then close the window. But since canceling jobs and finalize is all about cleaning memory I think we can just leave this unnecessary complexity. I chose »stop« as new term instead of having »cancel« appear twice.
Review of attachment 351987 [details] [review]: Looks good to me, however, I still see segfaults when closing... I think we can push this, because the segfaults are not caused by this patch, however, it would be nice to fix them as a part of this bug report... note: Nautilus shows notification "1 file operation active" after closing without any dialog and continue with the operation... this would be nice also for Disks for future :-) ::: src/disks/gduapplication.c @@ +89,3 @@ + GTK_DIALOG_DESTROY_WITH_PARENT, + GTK_MESSAGE_WARNING, + GTK_BUTTONS_OK_CANCEL, note: "Close" is usually used instead of "Ok" in such case (e.g. "Close without saving"), but "Ok" makes sense in this context as an answer to the question, so ok :-)
Acutally the AppData description tells that jobs would continue when being closed. But that only make sense if closing the window does not quit the whole gapplication as currently happening – either is was keeping it running was never working or just broken some day during code rewrites… Anyway there must be a notification like with Nautilus. I think it's good to keep the current patch until we have it. Should I rename this bug?
Ah, no. This bug is older than the AppData text.
As a simple solution the window could just be hidden and later shown again from gduapplication. Then it would be possible to do what Nautilus does. Canceling should still be shown as possible reaction on closing the window (maybe in the notification, maybe as question whether to run in the background or cancel the jobs).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-disk-utility/issues/22.