From 37e621a3c7156adb6e0a4bffa72a92945a71f101 Mon Sep 17 00:00:00 2001 From: thankyouverycool <66646555+thankyouverycool@users.noreply.github.com> Date: Thu, 18 May 2023 08:50:32 -0400 Subject: [PATCH] LibFileSystemAccessClient: Improve error propagation Previously FSAC displayed some but not all errors and always rejected directories and devices. This has led most apps to ignore response errors in open/save actions or show redundant messages. Now FSAC displays all errors including fd failures and has the ability to silence messages for directories, devices and ENOENT, which some apps handle differently. Silenced directory and device errors now return files on success. A request's access mode is now stored in RequestData to format more accurate error messages from the user's perspective. Resolved promises don't require callback propagation so they're voided --- .../LibFileSystemAccessClient/Client.cpp | 61 ++++++++++++------- .../LibFileSystemAccessClient/Client.h | 21 ++++++- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/Userland/Libraries/LibFileSystemAccessClient/Client.cpp b/Userland/Libraries/LibFileSystemAccessClient/Client.cpp index 703f7d9f3b..4ddf2aa2f2 100644 --- a/Userland/Libraries/LibFileSystemAccessClient/Client.cpp +++ b/Userland/Libraries/LibFileSystemAccessClient/Client.cpp @@ -26,7 +26,7 @@ Client& Client::the() Result Client::request_file_read_only_approved(GUI::Window* parent_window, DeprecatedString const& path) { auto const id = get_new_id(); - m_promises.set(id, PromiseAndWindow { { Core::Promise::construct() }, parent_window }); + m_promises.set(id, RequestData { { Core::Promise::construct() }, parent_window, Core::File::OpenMode::Read }); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto child_window_server_client_id = expose_window_server_client_id(); @@ -51,7 +51,7 @@ Result Client::request_file_read_only_approved(GUI::Window* parent_window, Depre Result Client::request_file(GUI::Window* parent_window, DeprecatedString const& path, Core::File::OpenMode mode) { auto const id = get_new_id(); - m_promises.set(id, PromiseAndWindow { { Core::Promise::construct() }, parent_window }); + m_promises.set(id, RequestData { { Core::Promise::construct() }, parent_window, mode }); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto child_window_server_client_id = expose_window_server_client_id(); @@ -76,7 +76,7 @@ Result Client::request_file(GUI::Window* parent_window, DeprecatedString const& Result Client::open_file(GUI::Window* parent_window, DeprecatedString const& window_title, StringView path, Core::File::OpenMode requested_access, Optional> const& allowed_file_types) { auto const id = get_new_id(); - m_promises.set(id, PromiseAndWindow { { Core::Promise::construct() }, parent_window }); + m_promises.set(id, RequestData { { Core::Promise::construct() }, parent_window, requested_access }); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto child_window_server_client_id = expose_window_server_client_id(); @@ -96,7 +96,7 @@ Result Client::open_file(GUI::Window* parent_window, DeprecatedString const& win Result Client::save_file(GUI::Window* parent_window, DeprecatedString const& name, DeprecatedString const ext, Core::File::OpenMode requested_access) { auto const id = get_new_id(); - m_promises.set(id, PromiseAndWindow { { Core::Promise::construct() }, parent_window }); + m_promises.set(id, RequestData { { Core::Promise::construct() }, parent_window, requested_access }); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto child_window_server_client_id = expose_window_server_client_id(); @@ -119,26 +119,39 @@ void Client::handle_prompt_end(i32 request_id, i32 error, Optional co VERIFY(potential_data.has_value()); auto& request_data = potential_data.value(); - if (error != 0) { - // We don't want to show an error message for non-existent files since some applications may want - // to handle it as opening a new, named file. - if (error != ECANCELED && error != ENOENT) - GUI::MessageBox::show_error(request_data.parent_window, DeprecatedString::formatted("Opening \"{}\" failed: {}", *chosen_file, strerror(error))); - request_data.promise->resolve(Error::from_errno(error)).release_value_but_fixme_should_propagate_errors(); - return; + auto action = "Requesting"sv; + if (has_flag(request_data.mode, Core::File::OpenMode::Read)) + action = "Opening"sv; + else if (has_flag(request_data.mode, Core::File::OpenMode::Write)) + action = "Saving"sv; + + if (ipc_file.has_value()) { + if (FileSystem::is_device(ipc_file->fd())) + error = is_silencing_devices() ? ESUCCESS : EINVAL; + else if (FileSystem::is_directory(ipc_file->fd())) + error = is_silencing_directories() ? ESUCCESS : EISDIR; } - if (FileSystem::is_device(ipc_file->fd())) { - GUI::MessageBox::show_error(request_data.parent_window, DeprecatedString::formatted("Opening \"{}\" failed: Cannot open device files", *chosen_file)); - request_data.promise->resolve(Error::from_string_literal("Cannot open device files")).release_value_but_fixme_should_propagate_errors(); - return; + switch (error) { + case ESUCCESS: + case ECANCELED: + break; + case ENOENT: + if (is_silencing_nonexistent_entries()) + break; + [[fallthrough]]; + default: + auto maybe_message = ErrorOr({}); + if (error == ECONNRESET) + maybe_message = String::formatted("FileSystemAccessClient: {}", Error::from_errno(error)); + else + maybe_message = String::formatted("{} \"{}\" failed: {}", action, *chosen_file, Error::from_errno(error)); + if (!maybe_message.is_error()) + (void)GUI::MessageBox::try_show_error(request_data.parent_window, maybe_message.release_value()); } - if (FileSystem::is_directory(ipc_file->fd())) { - GUI::MessageBox::show_error(request_data.parent_window, DeprecatedString::formatted("Opening \"{}\" failed: Cannot open directory", *chosen_file)); - request_data.promise->resolve(Error::from_errno(EISDIR)).release_value_but_fixme_should_propagate_errors(); - return; - } + if (error != ESUCCESS) + return (void)request_data.promise->resolve(Error::from_errno(error)); auto file_or_error = [&]() -> ErrorOr { auto stream = TRY(Core::File::adopt_fd(ipc_file->take_fd(), Core::File::OpenMode::ReadWrite)); @@ -146,11 +159,13 @@ void Client::handle_prompt_end(i32 request_id, i32 error, Optional co return File({}, move(stream), filename); }(); if (file_or_error.is_error()) { - request_data.promise->resolve(file_or_error.release_error()).release_value_but_fixme_should_propagate_errors(); - return; + auto maybe_message = String::formatted("{} \"{}\" failed: {}", action, *chosen_file, file_or_error.error()); + if (!maybe_message.is_error()) + (void)GUI::MessageBox::try_show_error(request_data.parent_window, maybe_message.release_value()); + return (void)request_data.promise->resolve(file_or_error.release_error()); } - request_data.promise->resolve(file_or_error.release_value()).release_value_but_fixme_should_propagate_errors(); + (void)request_data.promise->resolve(file_or_error.release_value()); } void Client::die() diff --git a/Userland/Libraries/LibFileSystemAccessClient/Client.h b/Userland/Libraries/LibFileSystemAccessClient/Client.h index 36e8718a2e..3d9c5cba7f 100644 --- a/Userland/Libraries/LibFileSystemAccessClient/Client.h +++ b/Userland/Libraries/LibFileSystemAccessClient/Client.h @@ -20,6 +20,14 @@ namespace FileSystemAccessClient { +enum ErrorFlag : u32 { + Devices = 1 << 0, + Directories = 1 << 1, + NoEntries = 1 << 2, + + None = 0, +}; + class Client; class File { public: @@ -51,6 +59,13 @@ public: Result open_file(GUI::Window* parent_window, DeprecatedString const& window_title = {}, StringView path = Core::StandardPaths::home_directory(), Core::File::OpenMode requested_access = Core::File::OpenMode::Read, Optional> const& = {}); Result save_file(GUI::Window* parent_window, DeprecatedString const& name, DeprecatedString const ext, Core::File::OpenMode requested_access = Core::File::OpenMode::Write | Core::File::OpenMode::Truncate); + void set_silence_errors(u32 flags) { m_silenced_errors = flags; } + u32 silenced_errors() const { return m_silenced_errors; } + + bool is_silencing_devices() { return m_silenced_errors & ErrorFlag::Devices; } + bool is_silencing_directories() { return m_silenced_errors & ErrorFlag::Directories; } + bool is_silencing_nonexistent_entries() { return m_silenced_errors & ErrorFlag::NoEntries; } + static Client& the(); protected: @@ -70,13 +85,15 @@ private: template using PromiseType = RefPtr>; - struct PromiseAndWindow { + struct RequestData { PromiseType promise; GUI::Window* parent_window { nullptr }; + Core::File::OpenMode mode { Core::File::OpenMode::NotOpen }; }; - HashMap m_promises {}; + HashMap m_promises {}; int m_last_id { 0 }; + u32 m_silenced_errors { ErrorFlag::None }; }; }