[jeremypw]

Moved the delete_file and delete_dir functions from marlin_file_operations.c to Files.FileOperations.DeleteJob making minimal changes to the code structure.

• The methods are kept as static and synchronous
• No drying of repeated code

Some re-ordering was unavoidable because errors need to handled in try/catch clauses in Vala and cannot use goto

Later commits tidies the Vala code a little while attempting to keep logic the same or equivalent. Additional comments are added. The intention is to clarify the fairly complicated logic as far possible.

[jeremypw]

@danirabbit Is this a better approach? It can be made into more elegant Vala later.

[danirabbit]

Thanks for breaking out this part! Much more manageable to review. Lots of things to clean up here

[jeremypw]

@danirabbit Thanks for the review. I didn't want to spend too much time cleaning it up until I knew I was on the right track - its just cut and pasted from C and then minimal changes to get it to compile. I'll fix up the code style now.

[jeremypw]

Converting to draft while checking changes in code not introduced behaviour changes.

[danirabbit]

Appreciate the improved comments! This looks a lot better than it did

There's some more cleanup we can do with strings here:

• Make sure we always include the file or directory name and use typographical quotes “ ”
• Be consistent about saying see files in “%s” or read files in “%s”
• We also seem to have a mix of saying folder “%s” or just “%s”. I'm not sure if we need to say "folder" here unless there's some important disambiguation happening
• If we can't provide useful secondary text, we should just use the error details instead of saying "See the details below"

[danirabbit]

I really don't love this. This doesn't tell us anything. Secondary text should provide information like reassurance, explain why it happened (if we can know that), provide a suggestion. If we don't have useful secondary text, we should just make the error message the secondary text imo

[jeremypw]

Its hard to know what to put here that adds information (and is translatable) since we do not know what the error was. Is it OK to use an untranslated error message as secondary text?

[danirabbit]

Yeah I think thats better than a message that has no real content personally

[danirabbit]

Just one last place with the secondary text. Otherwise this looks good to me. Thanks for addressing all that feedback!

[danirabbit]

Suggested change

	secondary = _("See details below for further information.");
	details = e.message;
	secondary = e.message;

[danirabbit]

details is always null?

Suggested change

details,

[danirabbit]

Seems like we have plenty of lines longer than this?

Suggested change

	while (
	!job.aborted () && !local_skipped_file
	) {
	while (!job.aborted () && !local_skipped_file) {

[danirabbit]

My mistake!

[danirabbit]

Ah I'm sorry. I was wrong here. We already name the folder in the primary text, so we don't need to do it again here.

Suggested change

	/// TRANSLATORS: %s is a placeholder for the basename of a file.
	secondary = _("You do not have permissions to see files in “%s”").printf (dir_basename);
	secondary = _("You do not have permission to see files in this folder");

[danirabbit]

Same as the other. We already name the folder in the primary text, so we don't need to do it again here

Suggested change

	secondary = _("You do not have permission to read “%s”.").printf (dir_basename);
	secondary = _("You do not have permission to see this folder");