Browser: Convert BookmarksBarWidget to use failable functions

Author: https://github.com/cammo1123
Commit: https://github.com/SerenityOS/serenity/commit/2df0eeaa20
Pull-request: https://github.com/SerenityOS/serenity/pull/18409
Reviewed-by: https://github.com/gmta
Reviewed-by: https://github.com/krkk

Userland/Applications/Browser/BookmarksBarWidget.cpp

@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2020, Emanuel Sprung <emanuel.sprung@gmail.com>
  * Copyright (c) 2022, networkException <networkexception@serenityos.org>
+ * Copyright (c) 2023, Cameron Youell <cameronyouell@gmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -15,6 +16,7 @@
 #include <LibGUI/Event.h>
 #include <LibGUI/JsonArrayModel.h>
 #include <LibGUI/Menu.h>
+#include <LibGUI/MessageBox.h>
 #include <LibGUI/Model.h>
 #include <LibGUI/TextBox.h>
 #include <LibGUI/Widget.h>
@@ -151,12 +153,14 @@ BookmarksBarWidget::BookmarksBarWidget(DeprecatedString const& bookmarks_file, b
     m_context_menu->add_separator();
     m_context_menu->add_action(GUI::Action::create(
         "&Edit...", g_icon_bag.rename, [this](auto&) {
-            edit_bookmark(m_context_menu_url);
+            if (auto result = edit_bookmark(m_context_menu_url); result.is_error())
+                GUI::MessageBox::show_error(this->window(), MUST(String::formatted("Failed to edit bookmark: {}", result.error())));
         },
         this));
     m_context_menu->add_action(GUI::CommonActions::make_delete_action(
         [this](auto&) {
-            remove_bookmark(m_context_menu_url);
+            if (auto result = remove_bookmark(m_context_menu_url); result.is_error())
+                GUI::MessageBox::show_error(this->window(), MUST(String::formatted("Failed to remove bookmark: {}", result.error())));
         },
         this));
 
@@ -271,7 +275,7 @@ void BookmarksBarWidget::update_content_size()
     }
 }
 
-bool BookmarksBarWidget::contains_bookmark(DeprecatedString const& url)
+bool BookmarksBarWidget::contains_bookmark(StringView url)
 {
     for (int item_index = 0; item_index < model()->row_count(); ++item_index) {
 
@@ -283,7 +287,7 @@ bool BookmarksBarWidget::contains_bookmark(DeprecatedString const& url)
     return false;
 }
 
-bool BookmarksBarWidget::remove_bookmark(DeprecatedString const& url)
+ErrorOr<void> BookmarksBarWidget::remove_bookmark(StringView url)
 {
     for (int item_index = 0; item_index < model()->row_count(); ++item_index) {
 
@@ -291,46 +295,41 @@ bool BookmarksBarWidget::remove_bookmark(DeprecatedString const& url)
         if (item_url == url) {
             auto& json_model = *static_cast<GUI::JsonArrayModel*>(model());
 
-            auto const item_removed = json_model.remove(item_index);
-            if (!item_removed)
-                return false;
-
-            json_model.store();
+            TRY(json_model.remove(item_index));
+            TRY(json_model.store());
 
             if (on_bookmark_change)
                 on_bookmark_change();
 
-            return true;
+            return {};
         }
     }
 
-    return false;
+    return Error::from_string_view("Bookmark not found"sv);
 }
 
-bool BookmarksBarWidget::add_bookmark(DeprecatedString const& url, DeprecatedString const& title)
+ErrorOr<void> BookmarksBarWidget::add_bookmark(StringView url, StringView title)
 {
     Vector<JsonValue> values;
-    values.append(title);
-    values.append(url);
+    TRY(values.try_append(title));
+    TRY(values.try_append(url));
 
-    auto was_bookmark_added = update_model(values, [](auto& model, auto&& values) {
-        return model.add(move(values));
-    });
-
-    if (!was_bookmark_added)
-        return false;
+    TRY(update_model(values, [](auto& model, auto&& values) -> ErrorOr<void> {
+        return TRY(model.add(move(values)));
+    }));
 
     if (on_bookmark_change)
         on_bookmark_change();
 
-    if (edit_bookmark(url, PerformEditOn::NewBookmark))
-        return true;
+    if (auto result = edit_bookmark(url, PerformEditOn::NewBookmark); result.is_error()) {
+        (void)remove_bookmark(url);
+        return Error::copy(result.release_error());
+    }
 
-    remove_bookmark(url);
-    return false;
+    return {};
 }
 
-bool BookmarksBarWidget::edit_bookmark(DeprecatedString const& url, PerformEditOn perform_edit_on)
+ErrorOr<void> BookmarksBarWidget::edit_bookmark(StringView url, PerformEditOn perform_edit_on)
 {
     for (int item_index = 0; item_index < model()->row_count(); ++item_index) {
         auto item_title = model()->index(item_index, 0).data().to_deprecated_string();
@@ -338,33 +337,32 @@ bool BookmarksBarWidget::edit_bookmark(DeprecatedString const& url, PerformEditO
 
         if (item_url == url) {
             auto values = BookmarkEditor::edit_bookmark(window(), item_title, item_url, perform_edit_on);
-            auto was_bookmark_changed = update_model(values, [item_index](auto& model, auto&& values) {
-                return model.set(item_index, move(values));
-            });
 
-            if (was_bookmark_changed && on_bookmark_change)
+            TRY(update_model(values, [item_index](auto& model, auto&& values) {
+                return model.set(item_index, move(values));
+            }));
+
+            if (on_bookmark_change)
                 on_bookmark_change();
 
-            return was_bookmark_changed;
+            return {};
         }
     }
 
-    return false;
+    return Error::from_string_view("Bookmark not found"sv);
 }
 
-bool BookmarksBarWidget::update_model(Vector<JsonValue>& values, Function<bool(GUI::JsonArrayModel& model, Vector<JsonValue>&& values)> perform_model_change)
+ErrorOr<void> BookmarksBarWidget::update_model(Vector<JsonValue>& values, Function<ErrorOr<void>(GUI::JsonArrayModel& model, Vector<JsonValue>&& values)> perform_model_change)
 {
-    bool has_model_changed = false;
+    if (values.is_empty())
+        return Error::from_string_view("No values to update model with"sv);
 
-    if (!values.is_empty()) {
-        auto& json_model = *static_cast<GUI::JsonArrayModel*>(model());
-        has_model_changed = perform_model_change(json_model, move(values));
+    auto& json_model = *static_cast<GUI::JsonArrayModel*>(model());
 
-        if (has_model_changed)
-            json_model.store();
-    }
+    TRY(perform_model_change(json_model, move(values)));
+    TRY(json_model.store());
 
-    return has_model_changed;
+    return {};
 }
 
 }

Userland/Applications/Browser/BookmarksBarWidget.h

@@ -32,20 +32,19 @@ public:
         InNewWindow
     };
 
-    Function<void(DeprecatedString const& url, Open)> on_bookmark_click;
-    Function<void(DeprecatedString const&, DeprecatedString const&)> on_bookmark_hover;
-    Function<void()> on_bookmark_change;
-
-    bool contains_bookmark(DeprecatedString const& url);
-    bool remove_bookmark(DeprecatedString const& url);
-    bool add_bookmark(DeprecatedString const& url, DeprecatedString const& title);
-
     enum class PerformEditOn {
         NewBookmark,
         ExistingBookmark
     };
 
-    bool edit_bookmark(DeprecatedString const& url, PerformEditOn perform_edit_on = PerformEditOn::ExistingBookmark);
+    Function<void(DeprecatedString const& url, Open)> on_bookmark_click;
+    Function<void(DeprecatedString const&, DeprecatedString const&)> on_bookmark_hover;
+    Function<void()> on_bookmark_change;
+
+    bool contains_bookmark(StringView url);
+    ErrorOr<void> remove_bookmark(StringView url);
+    ErrorOr<void> add_bookmark(StringView url, StringView title);
+    ErrorOr<void> edit_bookmark(StringView url, PerformEditOn perform_edit_on = PerformEditOn::ExistingBookmark);
 
     virtual Optional<GUI::UISize> calculated_min_size() const override
     {
@@ -64,7 +63,7 @@ private:
 
     void update_content_size();
 
-    bool update_model(Vector<JsonValue>& values, Function<bool(GUI::JsonArrayModel& model, Vector<JsonValue>&& values)> perform_model_change);
+    ErrorOr<void> update_model(Vector<JsonValue>& values, Function<ErrorOr<void>(GUI::JsonArrayModel& model, Vector<JsonValue>&& values)> perform_model_change);
 
     RefPtr<GUI::Model> m_model;
     RefPtr<GUI::Button> m_additional;

Userland/Applications/Browser/Tab.cpp

@@ -4,6 +4,7 @@
  * Copyright (c) 2021, Sam Atkins <atkinssj@serenityos.org>
  * Copyright (c) 2022, the SerenityOS developers.
  * Copyright (c) 2022, networkException <networkexception@serenityos.org>
+ * Copyright (c) 2023, Cameron Youell <cameronyouell@gmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -195,7 +196,8 @@ Tab::Tab(BrowserWindow& window)
 
     auto bookmark_action = GUI::Action::create(
         "Bookmark current URL", { Mod_Ctrl, Key_D }, [this](auto&) {
-            bookmark_current_url();
+            if (auto result = bookmark_current_url(); result.is_error())
+                GUI::MessageBox::show_error(this->window().main_widget()->window(), MUST(String::formatted("Failed to bookmark URL: {}", result.error())));
         },
         this);
 
@@ -596,17 +598,19 @@ void Tab::update_actions()
     window.go_forward_action().set_enabled(m_history.can_go_forward());
 }
 
-void Tab::bookmark_current_url()
+ErrorOr<void> Tab::bookmark_current_url()
 {
     auto url = this->url().to_deprecated_string();
     if (BookmarksBarWidget::the().contains_bookmark(url)) {
-        BookmarksBarWidget::the().remove_bookmark(url);
+        TRY(BookmarksBarWidget::the().remove_bookmark(url));
     } else {
-        BookmarksBarWidget::the().add_bookmark(url, m_title);
+        TRY(BookmarksBarWidget::the().add_bookmark(url, m_title));
     }
+
+    return {};
 }
 
-void Tab::update_bookmark_button(DeprecatedString const& url)
+void Tab::update_bookmark_button(StringView url)
 {
     if (BookmarksBarWidget::the().contains_bookmark(url)) {
         m_bookmark_button->set_icon(g_icon_bag.bookmark_filled);

Userland/Applications/Browser/Tab.h

@@ -108,8 +108,8 @@ private:
     BrowserWindow& window();
 
     void update_actions();
-    void bookmark_current_url();
-    void update_bookmark_button(DeprecatedString const& url);
+    ErrorOr<void> bookmark_current_url();
+    void update_bookmark_button(StringView url);
     void start_download(const URL& url);
     void view_source(const URL& url, DeprecatedString const& source);
     void update_status(Optional<DeprecatedString> text_override = {}, i32 count_waiting = 0);