Add dialog to ask for overwrite

[mcclurgm]

Fixes #1955

Adds a new second-layer modal dialog that asks whether to overwrite a file when the name clashes with an existing file.

Known issues:

• When the selected file is renamed, deleted, or otherwise removed from the current directory, the dialog goes to the next selectable file and picks that. We need to preserve the user's filename choice in such cases, so the saved file remains consistent.
  • To demonstrate:
    1. Open a "Save As" dialog box
    2. Select an existing file to overwrite in a folder with multiple files (make sure you don't care about any of them).
    3. The conflict dialog will appear. Do not take action.
    4. In the file manager, delete the file that you selected.
    5. Back in the save dialog, the deleted file will disappear and the selection will move to a different file.
    6. Click the "Replace" button. The newly selected file, not the file that you originally picked or was displayed in the conflict dialog, will be overwritten.
  • Expected behavior: if the chosen file is removed, a new file should be saved with the name of the file originally chosen. (This may be up for debate, but that's what I expect.)
  • Related: if the file is deleted, is there any reason to change the behavior? I don't imagine we should dismiss the conflict dialog automatically, and I can't think of any other behavior change to make.
• When the dialog is closed by confirming and saving the file, the parent window remains dimmed.
• The save dialog is not dimmed when the overwrite dialog is presented.

Things to check:

• I'm not sure if the way I'm checking for existing files is preferable. The docs seem to suggest you don't use file tests for this functionality, but I'm not sure what else to do.
• Better wording could be nice. I did my best to explain (largely taken from Apple's built in wording), but there's always room for improvement.
• It does not handle failed URI→filename conversions. I'm not sure what situation would cause them, so I don't know what would be a sensible fallback.
• Do I need to handle different file types, like symlinks, differently? If you choose a symlink then the linked file would be overwritten, not the link file. I'm not sure if I should use different wording in that case.
• I'm not sure if it's possible to select multiple files in a replace dialog. If so, then that would almost certainly need special handling.
• Likewise, I have not checked the behavior with other FileActions, like creating a new folder. I assume no selection actions would be able to trigger an overwrite.

Any other testing, code, or design input would be hugely appreciated.

[jeremypw]

I havent tested yet but presumably this is intended to replace any dialog the calling app throws up when a file conflict occurs? Otherwise the user will be presented with two dialogs if the user confirms overwriting is intended.

[jeremypw]

I see that Gtk.FileChooser interface has "do_overwrite_confirmation" property to deal with this but does the freedesktop portal interface support this?

[jeremypw]

The documentation for the portal linked to above says

Supported keys in the options vardict include:

Does this mean other options can also be supported - in which case we need to support and honour the "do-overwrite-confirmation" property in the portal and default that to "false".

[mcclurgm]

Hmm. I agree, I don't see any such option in the freedesktop portal. Do you mean that it could be possible to pass it in the dictionary anyway out of spec? If so, then do we need to reimplement the whole GtkFileChooser overwrite interface?

FWIW I read that wording to mean that these are the only supported options. But I'm not familiar with portals or their documentation so that probably doesn't mean much.

[jeremypw]

Using the word "include" does not exclude the possibility of other options - to do that you would say "The supported keys in the options vardict are:". However it is possible that it is an unintentional miswording.

Would need to add some code to filechooser-portal/Main.vala like

if ("do-overwrite-confirmation" in options) {
    dialog.do_overwrite_confirmation = options["do-overwrite-confirmation"].get_boolean ();
}

and add do_overwrite_confirmation as a property of the Files FileChooserDialog implementation (defaulting to false), I would guess.

[mcclurgm]

Yeah, adding a simple key like that would be relatively easy. I guess I may be misunderstanding. I was referring to the Gtk.FileChooser interface, which is much more complex, involving signal callbacks. I don't understand the API from the docs, but it also looks like something that would not be possible to pass through the fd.o portal?

So are you thinking of just including a single key that turns on/off the overwrite dialog, or should we work on reaching more feature completeness with the Gtk interface? The second would be much harder.

[jeremypw]

I am assuming that the portal interface will expect a dictionary Variant for the options but not be concerned with what it contains. I would be very wary of changing the spec of the portal. Just need to try it with one extra boolean option first.

[Marukesu]

about the code, IMHO, it's better to do the check inside the response callback in the save_file and save_files methods, since in one we are selecting an file, and in the other we are selecting an folder, the code for handle this for both cases are different.

Does this mean other options can also be supported - in which case we need to support and honour the "do-overwrite-confirmation" property in the portal and default that to "false".

The frontend filter out unsupported options. so we cannot add an extra do-overwrite-confirmation option.

So are you thinking of just including a single key that turns on/off the overwrite dialog, or should we work on reaching more feature completeness with the Gtk interface? The second would be much harder.

Fature completeness with the Gtk.FileChooser shouldn't be an goal, we should only provide what's needed for the org.freedesktop.portal.impl.FileChooser interface.

[jeremypw]

The frontend filter out unsupported options. so we cannot add an extra do-overwrite-confirmation option.

That's a pity. I guess the client app will have to continue do the check then. So does this make this PR a non-starter then?

[mcclurgm]

Is your concern that apps calling the portal will check for duplicates, making a dialog in the portal redundant? I can't think of any app that does that. At least for a selected file (as opposed to a directory into which it places multiple files, like Audacity's bulk export). That's what led me to this issue.

Note that in GtkFileChooserNative, which is the only widget that would call the xdg portal afaik (?), the do_overwrite_confirmation callback is not supported:

There are a few things in the GtkFileChooser API that are not possible to use with FileChooserNative, as such use would prohibit the use of a native dialog.

There is no support for the signals that are emitted when the user navigates in the dialog, including: * current_folder_changed * selection_changed * file_activated * confirm_overwrite

You can also not use the methods that directly control user navigation: * unselect_filename * select_all * unselect_all

If you need any of the above you will have to use FileChooserDialog directly.

I guess we could see what GNOME's portal does, but if we don't handle it in the file chooser, then my experience is that files will be silently overwritten.

[jeremypw]

It depends on whether the portal expects the filechooser to always check for and confirm overwriting duplicates or never to check - since the option is not supported. You may be right that most apps leave it to the filechooser, but since, currently, that option defaults to false, always doing it in the filechooser would be a change in default behaviour. I don't know what other platforms do.

I notice that Code sets the do_overwrite_confirmation option (which will not work with the current portal) so that needs resolving at least. My app Gnonograms however does not set that option and confirms overwrite itself.

Either way some apps are going to have to change.

[danirabbit]

Since apps are supposedly sandboxed, they shouldn't be able to see files outside the sandbox and thus wouldn't be able to check for duplicates. So this responsibility should fall on the file chooser portal.

According to the spec:

If the selected folder already contains a file with one of the given names, the portal may prompt or take some other action to construct a unique file name and return that instead.

[jeremypw]

the portal may prompt or take some other action to construct a unique file name and return that instead

This does not sound like the portal must always take an action. However, for the elementary portal, I guess we can specify that it does and that apps do not have to do any checking. In this case, any app that does check will have to be amended to remove the checks.

Presumably the user could confirm that they want to overwrite the file with the same name? Or is that not allowed for sandboxed apps?

[danirabbit]

Yeah I think making things the responsibility of the portal makes sense anyways so developers don't have to worry about it

[mcclurgm]

So are there any design changes I should be making, or do you think this is good to continue how I have it broadly set up here? I'm not quite understanding where this conversation ended up, sorry.

@Marukesu I agree that in principle handling this in save_file makes sense. The way to implement it isn't very obvious to me though. If the user chooses to cancel in the overwrite dialog, is there a reasonable way to keep the file chooser open and continue as before? My first guess is that if we avoid the save_file.callback () line (say by returning early from the response callback before it gets there) it won't continue through the save_file code and close the window. But in my experience that's not working.

[Marukesu]

If the user chooses to cancel in the overwrite dialog, is there a reasonable way to keep the file chooser open and continue as before? My first guess is that if we avoid the save_file.callback () line (say by returning early from the response callback before it gets there) it won't continue through the save_file code and close the window. But in my experience that's not working.

@mcclurgm the FileChooser calls destroy () in response.connect_after () what closes the dialog anyway, that need to be moved to the portal methods, and the state saving code to be in dispose () instead.

However, i think having the dialog be like the FileConflictDialog offering to replace/rename (and a extra skip option, for the save_files case) with cancel beginning the same of clicking cancel in the FileChooser better, and more like the Files Application.

[mcclurgm]

So if I understand correctly, you're saying the code that's currently in response.connect_after needs to be moved to a different function. I'm not sure what you mean by dispose, could you be referring to destroy? If that's done, then maybe we could avoid destroying the window when a cancel button is clicked.

I see what you're saying about offering to rename the file in the dialog. Offering to rename does seem like a nice idea. But I'm wary of having a new dialog with no way to get back to the file chooser in its current state, for example if someone wanted to change the directory when they realized that there's a conflict. This could probably use input from UX.

Sorry for letting this PR languish. Since I started working on it things have come up and I don't really have time right now, so I'm going to have to leave it be for a while. I may be able to come back to it, but it may be a while, since there's quite a bit of work left to do. If someone else wants to pick up from my work here I'd welcome it.

[danirabbit]

Since the save file portal already has the ability to rename, we probably don't need to add that to this confirmation since "Cancel" for the confirmation should just take the user back to the save file portal where they can rename or change locations.

[jeremypw]

If the file to be overwritten is read-only then the operation seems to fail silently.

[jeremypw]

@Marukesu

The frontend filter out unsupported options.

Am I right in thinking that this means that there is no need to check that select_multiple is false when saving (and the assertion can be omitted)? In which case I will remove the dialog dealing with that.

[mcclurgm]

Thanks for the update. It does work, although performing the action on key press instead of release is inconsistent with other UI. I wonder if it would be possible to capture the key event in the replace dialog instead and then stop propagation there, so it would never reach the FileChooserDialog handler. That is how it intuitively would work for me anyway—unless the user chooses to accept, then the FileChooserDialog shouldn't be doing any handling. I gave that a try (using replace_dialog.key_release_event.connect, but that function is never called. The signal seems to be captured by the FileChooserDialog first, and I'm not sure why.

Also, your change was overwritten by the latest merge commit. If we want to use the key_press_event solution we'll need to add it back in.

[mcclurgm]

I think the replace dialog needs a transient parent. Currently it doesn't have one. That means that it appears in the window switcher, which it shouldn't.

I assume that it should be transient to the FileChooserDialog. The changes suggested here would implement that. This is a set of 3 changes that all need to be committed together to work, since we need to pass around a FileChooserDialog reference.

Update: I do remember doing this in my original code, but ran into some issues. The calling app window was never un-dimmed when the file chooser was dismissed. I don't see that with the changes I added here, so I am going to assume something about the way you've changed it fixed an issue with the replace dialog not being disposed properly. I'm not sure though.

[mcclurgm]

Sorry for the late review (again). This looks good to me for the most part.

I've done my best to collect a list of the outstanding issues here.

Potential blocking bugs:

• If the file you're overwriting is locked (not writeable) the process fails silently. I'm not sure if this should be the responsibility of the filechooser or the app to handle this. This is also separate from asking for conflicts, since the chooser should not allow you to select locked files to begin with (so may be something to address in a separate PR).

Requires design input:

• Wording for the link case. I think that the current copy is a bit confusing by referring to only the target file by name, and not the link that you selected. I may be wrong on this, but I could see that causing confusion when a completely different filename than you selected appears in the dialog.
• Should we still perform the conflict check on a URI provided by the app? The solution of checking only with a complete URI and not filename may make sense. To me this depends on how we view the save portal conceptually: is it the user selecting a file to be saved to, or is it the user selecting a file for the app to get access to? I hope this question makes sense. If it's a matter of access, then re-asking for something the app already knows of may be repetitive. However, even if the app has access, if it's a matter of a save location then we want to make sure the user is OK with saving to the app-provided URI.

There are also some minor cosmetic issues, none of which I think are blocking:

• The main filechooser dialog is not dimmed when the conflict dialog appears. This may be something in Gala, or it may be our fault. Either way, this isn't a big deal and definitely not blocking.
• The esc key interaction is strange. It dismisses both dialogs, the conflict and the filechooser, which is not ideal. It also dismisses the dialog on key down instead of key up unlike other similar interactions in the system. It seems you committed something to fix the behavior dismissing both dialogs, but I'm still getting it on my machine. Maybe we can just give this a pass, since it's a very small issue.

I will also note that there may be some strange behavior when a file is deleted/moved/renamed from under the conflict dialog and you then save to the original, conflicted file. In Code, a warning infobar appears wrongly saying that the file has been deleted. It was deleted, but is now reinstated by saving. This issue is probably outside of the filechooser, but it seems possible that the chooser is passing something strange back to Code that is causing it to present that infobar. I have not been able to check this yet.

Overall, I think this is essentially ready to go in my view. Just needs some input to determine whether we consider any of the above issues worth holding it up for longer.

[Marukesu]

If the file you're overwriting is locked (not writeable) the process fails silently. I'm not sure if this should be the responsibility of the filechooser or the app to handle this. This is also separate from asking for conflicts, since the chooser should not allow you to select locked files to begin with (so may be something to address in a separate PR).

If the application fails silently with an non-writable file, it's an application bug, the user can always mark the file as non-writable. Something to check is if the GNOME and KDE portals let you select non-writeable files too, but not for this PR.

Should we still perform the conflict check on a URI provided by the app? The solution of checking only with a complete URI and not filename may make sense. To me this depends on how we view the save portal conceptually: is it the user selecting a file to be saved to, or is it the user selecting a file for the app to get access to? I hope this question makes sense. If it's a matter of access, then re-asking for something the app already knows of may be repetitive. However, even if the app has access, if it's a matter of a save location then we want to make sure the user is OK with saving to the app-provided URI.

It's both, the user is selecting an file to the app get access, and then save somthing in it.

What i had in mind was "don't warn users about something they're are already aware", if an sandboxed application has an valid "current_file", it's mean that he got the path from an previous portal call.

The main filechooser dialog is not dimmed when the conflict dialog appears. This may be something in Gala, or it may be our fault. Either way, this isn't a big deal and definitely not blocking.

That's an gala issue, if the FileChooser has an parent, the dim is applied to the parent window again. Launching it without an parent window makes the FileChooserDialog be dimmed as expected.

The esc key interaction is strange. It dismisses both dialogs, the conflict and the filechooser, which is not ideal. It also dismisses the dialog on key down instead of key up unlike other similar interactions in the system. It seems you committed something to fix the behavior dismissing both dialogs, but I'm still getting it on my machine. Maybe we can just give this a pass, since it's a very small issue.

Maybe we should port the FileChooser to be a Granite/Gtk.Dialog (we would need to port away from Hdy for Gtk4 anyway), but not in this PR.

The key_release_event should be kept for now, since that's what the HIG recommends.

I will also note that there may be some strange behavior when a file is deleted/moved/renamed from under the conflict dialog and you then save to the original, conflicted file. In Code, a warning infobar appears wrongly saying that the file has been deleted. It was deleted, but is now reinstated by saving. This issue is probably outside of the filechooser, but it seems possible that the chooser is passing something strange back to Code that is causing it to present that infobar. I have not been able to check this yet.

All we do is return an URI to the frontend, There's nothing we can do if the file gets deleted after that. If code shows strange behaviour if the file get deleted by it back, it's should be fixed there.

[jeremypw]

@Marukesu This seems to work pretty well now. It may be better to merge and iterate with smaller PRs? There is a Files release coming up and it would be good to get this in.

[jeremypw]

@Marukesu @mcclurgm One question: Why is the response from the dialog functions in Main.vala set to a positive integer rather than a Gtk.ResponseType e.g. line 243? Where is the meaning of these numbers defined? Gtk.ResponseTypes are negative integers I think.

[jeremypw]

@mcclurgm I have now fixed the Esc key issue.

[Marukesu]

@jeremypw the portal frontend expect the response begin 0 on success, 1 on cancel, and 2 for any other reason, what we do here is what GNOME do in the Gtk/GNOME portal.

one last change: on key_press_event(), return base.key_press_event() instead of Gdk.EVENT_PROPAGATE. it's breaking the pathbar and name entry now.

[jeremypw]

@mcclurgm

The esc key interaction is strange. It dismisses both dialogs, the conflict and the filechooser, which is not ideal. It also dismisses the dialog on key down instead of key up unlike other similar interactions in the system. It seems you committed something to fix the behavior dismissing both dialogs, but I'm still getting it on my machine. Maybe we can just give this a pass, since it's a very small issue.

Both the filechooser dialog and the conflict dialog now dismiss separately with Esc key down. Before, the conflict dialog dismissed with key down (due to upstream code) and the file chooser dismissed with Esc key up. That was why both dialogs were being dismissed with Esc key down followed by key up.

Actions linked to accelerators fire on key down.