Fix issues reported by clang-tidy bugprone checks (GTK client) (#4183)

* Fix `bugprone-unchecked-optional-access` clang-tidy issues

* Fix `bugprone-easily-swappable-parameters` clang-tidy issues

* Extend clang-tidy configuration

gtk/.clang-tidy

@@ -1,6 +1,8 @@
 ---
 Checks: >
   -*,
+  bugprone-*,
+  -bugprone-narrowing-conversions,
   cert-*,
   cppcoreguidelines-*,
   -cppcoreguidelines-avoid-magic-numbers,

gtk/FaviconCache.cc

@@ -141,6 +141,13 @@ void gtr_get_favicon_from_url(
     Glib::ustring const& url,
     std::function<void(Glib::RefPtr<Gdk::Pixbuf> const&)> const& pixbuf_ready_func)
 {
-    auto const host = std::string{ tr_urlParse(url.c_str())->host };
+    if (auto const parsed_url = tr_urlParse(url.c_str()); parsed_url.has_value())
-    gtr_get_favicon(session, host, pixbuf_ready_func);
+    {
+        auto const host = std::string{ parsed_url->host };
+        gtr_get_favicon(session, host, pixbuf_ready_func);
+    }
+    else
+    {
+        pixbuf_ready_func({});
+    }
 }

gtk/MakeDialog.cc

@@ -43,15 +43,20 @@ public:
     MakeProgressDialog(
         BaseObjectType* cast_item,
         Glib::RefPtr<Gtk::Builder> const& builder,
-        Gtk::Window& parent,
         tr_metainfo_builder& metainfo_builder,
         std::future<tr_error*> future,
-        std::string_view const& target,
+        std::string_view target,
         Glib::RefPtr<Session> const& core);
     ~MakeProgressDialog() override;
 
     TR_DISABLE_COPY_MOVE(MakeProgressDialog)
 
+    static std::unique_ptr<MakeProgressDialog> create(
+        std::string_view target,
+        tr_metainfo_builder& metainfo_builder,
+        std::future<tr_error*> future,
+        Glib::RefPtr<Session> const& core);
+
     [[nodiscard]] bool success() const
     {
         return success_;
@@ -108,8 +113,7 @@ private:
 
     void setFilename(std::string_view filename);
 
-    void makeProgressDialog(std::string_view target, std::future<tr_error*> future);
+    void configurePieceSizeScale(uint32_t piece_size);
-    void configurePieceSizeScale();
     void onPieceSizeUpdated();
 
 private:
@@ -251,10 +255,9 @@ void MakeProgressDialog::onProgressDialogResponse(int response)
 MakeProgressDialog::MakeProgressDialog(
     BaseObjectType* cast_item,
     Glib::RefPtr<Gtk::Builder> const& builder,
-    Gtk::Window& parent,
     tr_metainfo_builder& metainfo_builder,
     std::future<tr_error*> future,
-    std::string_view const& target,
+    std::string_view target,
     Glib::RefPtr<Session> const& core)
     : Gtk::Dialog(cast_item)
     , builder_(metainfo_builder)
@@ -264,7 +267,6 @@ MakeProgressDialog::MakeProgressDialog(
     , progress_label_(gtr_get_widget<Gtk::Label>(builder, "progress_label"))
     , progress_bar_(gtr_get_widget<Gtk::ProgressBar>(builder, "progress_bar"))
 {
-    set_transient_for(parent);
     signal_response().connect(sigc::mem_fun(*this, &MakeProgressDialog::onProgressDialogResponse));
 
     progress_tag_ = Glib::signal_timeout().connect_seconds(
@@ -273,17 +275,62 @@ MakeProgressDialog::MakeProgressDialog(
     onProgressDialogRefresh();
 }
 
-void MakeDialog::Impl::makeProgressDialog(std::string_view target, std::future<tr_error*> future)
+std::unique_ptr<MakeProgressDialog> MakeProgressDialog::create(
+    std::string_view target,
+    tr_metainfo_builder& metainfo_builder,
+    std::future<tr_error*> future,
+    Glib::RefPtr<Session> const& core)
 {
     auto const builder = Gtk::Builder::create_from_resource(gtr_get_full_resource_path("MakeProgressDialog.ui"));
-    progress_dialog_ = std::unique_ptr<MakeProgressDialog>(gtr_get_widget_derived<MakeProgressDialog>(
+    return std::unique_ptr<MakeProgressDialog>(gtr_get_widget_derived<MakeProgressDialog>(
         builder,
         "MakeProgressDialog",
-        dialog_,
+        metainfo_builder,
-        *builder_,
         std::move(future),
         target,
-        core_));
+        core));
+}
+
+void MakeDialog::Impl::onResponse(int response)
+{
+    if (response == TR_GTK_RESPONSE_TYPE(CLOSE))
+    {
+        dialog_.close();
+        return;
+    }
+
+    if (response != TR_GTK_RESPONSE_TYPE(ACCEPT) || !builder_.has_value())
+    {
+        return;
+    }
+
+    // destination file
+    auto const dir = destination_chooser_->get_filename();
+    auto const base = Glib::path_get_basename(builder_->top());
+    auto const target = fmt::format("{:s}/{:s}.torrent", dir, base);
+
+    // build the announce list
+    auto trackers = tr_announce_list{};
+    trackers.parse(announce_text_buffer_->get_text(false).raw());
+    builder_->setAnnounceList(std::move(trackers));
+
+    // comment
+    if (comment_check_->get_active())
+    {
+        builder_->setComment(comment_entry_->get_text().raw());
+    }
+
+    // source
+    if (source_check_->get_active())
+    {
+        builder_->setSource(source_entry_->get_text().raw());
+    }
+
+    builder_->setPrivate(private_check_->get_active());
+
+    // build the .torrent
+    progress_dialog_ = MakeProgressDialog::create(target, *builder_, builder_->makeChecksums(), core_);
+    progress_dialog_->set_transient_for(dialog_);
     gtr_window_on_close(
         *progress_dialog_,
         [this]()
@@ -298,57 +345,15 @@ void MakeDialog::Impl::makeProgressDialog(std::string_view target, std::future<t
     progress_dialog_->show();
 }
 
-void MakeDialog::Impl::onResponse(int response)
-{
-    if (response == TR_GTK_RESPONSE_TYPE(ACCEPT))
-    {
-        if (builder_)
-        {
-            // destination file
-            auto const dir = destination_chooser_->get_filename();
-            auto const base = Glib::path_get_basename(builder_->top());
-            auto const target = fmt::format("{:s}/{:s}.torrent", dir, base);
-
-            // build the announce list
-            auto trackers = tr_announce_list{};
-            trackers.parse(announce_text_buffer_->get_text(false).raw());
-            builder_->setAnnounceList(std::move(trackers));
-
-            // comment
-            if (comment_check_->get_active())
-            {
-                builder_->setComment(comment_entry_->get_text().raw());
-            }
-
-            // source
-            if (source_check_->get_active())
-            {
-                builder_->setSource(source_entry_->get_text().raw());
-            }
-
-            builder_->setPrivate(private_check_->get_active());
-
-            // build the .torrent
-            makeProgressDialog(target, builder_->makeChecksums());
-        }
-    }
-    else if (response == TR_GTK_RESPONSE_TYPE(CLOSE))
-    {
-        dialog_.close();
-    }
-}
-
 /***
 ****
 ***/
 
 void MakeDialog::Impl::updatePiecesLabel()
 {
-    auto const filename = builder_ ? builder_->top() : ""sv;
-
     auto gstr = Glib::ustring();
 
-    if (std::empty(filename))
+    if (!builder_.has_value() || std::empty(builder_->top()))
     {
         gstr += _("No source selected");
         piece_size_scale_->set_visible(false);
@@ -372,10 +377,10 @@ void MakeDialog::Impl::updatePiecesLabel()
     pieces_lb_->set_text(gstr);
 }
 
-void MakeDialog::Impl::configurePieceSizeScale()
+void MakeDialog::Impl::configurePieceSizeScale(uint32_t piece_size)
 {
     // the below lower & upper bounds would allow piece size selection between approx 1KiB - 64MiB
-    auto adjustment = Gtk::Adjustment::create(log2(builder_->pieceSize()), 10, 26, 1.0, 1.0);
+    auto adjustment = Gtk::Adjustment::create(log2(piece_size), 10, 26, 1.0, 1.0);
     piece_size_scale_->set_adjustment(adjustment);
     piece_size_scale_->set_visible(true);
 }
@@ -387,7 +392,7 @@ void MakeDialog::Impl::setFilename(std::string_view filename)
     if (!filename.empty())
     {
         builder_.emplace(filename);
-        configurePieceSizeScale();
+        configurePieceSizeScale(builder_->pieceSize());
     }
 
     updatePiecesLabel();

gtk/PrefsDialog.cc

@@ -725,6 +725,7 @@ void RemotePage::refreshWhitelist()
     core_->set_pref(TR_KEY_rpc_whitelist, str);
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 void RemotePage::onAddressEdited(Glib::ustring const& path, Glib::ustring const& address)
 {
     if (auto const iter = store_->get_iter(path); iter)
@@ -836,7 +837,7 @@ RemotePage::RemotePage(BaseObjectType* cast_item, Glib::RefPtr<Gtk::Builder> con
 
         /* ip address column */
         auto* r = Gtk::make_managed<Gtk::CellRendererText>();
-        r->signal_edited().connect([this](auto const& path, auto const& new_text) { onAddressEdited(path, new_text); });
+        r->signal_edited().connect(sigc::mem_fun(*this, &RemotePage::onAddressEdited));
         r->property_editable() = true;
         auto* c = Gtk::make_managed<Gtk::TreeViewColumn>("", *r);
         c->add_attribute(r->property_text(), whitelist_cols.address);

gtk/Session.cc

@@ -313,6 +313,7 @@ bool is_valid_eta(int t)
     return t != TR_ETA_NOT_AVAIL && t != TR_ETA_UNKNOWN;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_eta(int a, int b)
 {
     bool const a_valid = is_valid_eta(a);
@@ -337,11 +338,13 @@ int compare_eta(int a, int b)
 }
 
 template<typename T>
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_generic(T&& a, T&& b)
 {
     return a < b ? -1 : (a > b ? 1 : 0);
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_ratio(double a, double b)
 {
     int ret = 0;
@@ -366,11 +369,13 @@ int compare_ratio(double a, double b)
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_name(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     return a->get_value(torrent_cols.name_collated).compare(b->get_value(torrent_cols.name_collated));
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_queue(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto const* const sa = tr_torrentStatCached(static_cast<tr_torrent*>(a->get_value(torrent_cols.torrent)));
@@ -379,6 +384,7 @@ int compare_by_queue(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::co
     return sb->queuePosition - sa->queuePosition;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_ratio(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     int ret = 0;
@@ -399,6 +405,7 @@ int compare_by_ratio(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::co
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_activity(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     int ret = 0;
@@ -427,6 +434,7 @@ int compare_by_activity(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel:
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_age(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto* const ta = static_cast<tr_torrent*>(a->get_value(torrent_cols.torrent));
@@ -441,6 +449,7 @@ int compare_by_age(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::cons
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_size(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto const size_a = tr_torrentTotalSize(static_cast<tr_torrent*>(a->get_value(torrent_cols.torrent)));
@@ -455,6 +464,7 @@ int compare_by_size(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::con
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_progress(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto const* const sa = tr_torrentStatCached(static_cast<tr_torrent*>(a->get_value(torrent_cols.torrent)));
@@ -474,6 +484,7 @@ int compare_by_progress(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel:
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_eta(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto const* const sa = tr_torrentStatCached(static_cast<tr_torrent*>(a->get_value(torrent_cols.torrent)));
@@ -488,6 +499,7 @@ int compare_by_eta(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::cons
     return ret;
 }
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int compare_by_state(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b)
 {
     auto const sa = a->get_value(torrent_cols.activity);
@@ -1283,6 +1295,7 @@ void Session::clear()
 namespace
 {
 
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 int gtr_compare_double(double const a, double const b, int decimal_places)
 {
     auto const ia = int64_t(a * pow(10, decimal_places));

gtk/TorrentCellRenderer.cc

@@ -324,8 +324,8 @@ public:
 
     TR_DISABLE_COPY_MOVE(Impl)
 
-    void get_size_compact(Gtk::Widget& widget, int& width, int& height) const;
+    Gtk::Requisition get_size_compact(Gtk::Widget& widget) const;
-    void get_size_full(Gtk::Widget& widget, int& width, int& height) const;
+    Gtk::Requisition get_size_full(Gtk::Widget& widget) const;
 
     void render_compact(
         SnapshotPtr const& snapshot,
@@ -442,7 +442,7 @@ void TorrentCellRenderer::Impl::set_icon(
 #endif
 }
 
-void TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget, int& width, int& height) const
+Gtk::Requisition TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget) const
 {
     int xpad = 0;
     int ypad = 0;
@@ -478,11 +478,11 @@ void TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget, int& width
     *** LAYOUT
     **/
 
-    width = xpad * 2 + get_width(icon_size) + GUI_PAD + CompactBarWidth + GUI_PAD + get_width(stat_size);
+    return { xpad * 2 + get_width(icon_size) + GUI_PAD + CompactBarWidth + GUI_PAD + get_width(stat_size),
-    height = ypad * 2 + std::max(get_height(name_size), property_bar_height_.get_value());
+             ypad * 2 + std::max(get_height(name_size), property_bar_height_.get_value()) };
 }
 
-void TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget, int& width, int& height) const
+Gtk::Requisition TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget) const
 {
     int xpad = 0;
     int ypad = 0;
@@ -526,29 +526,20 @@ void TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget, int& width, i
     *** LAYOUT
     **/
 
-    width = xpad * 2 + get_width(icon_size) + GUI_PAD + std::max(get_width(prog_size), get_width(stat_size));
+    return { xpad * 2 + get_width(icon_size) + GUI_PAD + std::max(get_width(prog_size), get_width(stat_size)),
-    height = ypad * 2 + get_height(name_size) + get_height(prog_size) + GUI_PAD_SMALL + property_bar_height_.get_value() +
+             ypad * 2 + get_height(name_size) + get_height(prog_size) + GUI_PAD_SMALL + property_bar_height_.get_value() +
-        GUI_PAD_SMALL + get_height(stat_size);
+                 GUI_PAD_SMALL + get_height(stat_size) };
 }
 
 void TorrentCellRenderer::get_preferred_width_vfunc(Gtk::Widget& widget, int& minimum_width, int& natural_width) const
 {
     if (impl_->property_torrent().get_value() != nullptr)
     {
-        int w = 0;
+        auto const size = impl_->property_compact().get_value() ? impl_->get_size_compact(widget) :
-        int h = 0;
+                                                                  impl_->get_size_full(widget);
 
-        if (impl_->property_compact().get_value())
+        minimum_width = get_width(size);
-        {
+        natural_width = minimum_width;
-            impl_->get_size_compact(widget, w, h);
-        }
-        else
-        {
-            impl_->get_size_full(widget, w, h);
-        }
-
-        minimum_width = w;
-        natural_width = w;
     }
 }
 
@@ -556,20 +547,11 @@ void TorrentCellRenderer::get_preferred_height_vfunc(Gtk::Widget& widget, int& m
 {
     if (impl_->property_torrent().get_value() != nullptr)
     {
-        int w = 0;
+        auto const size = impl_->property_compact().get_value() ? impl_->get_size_compact(widget) :
-        int h = 0;
+                                                                  impl_->get_size_full(widget);
 
-        if (impl_->property_compact().get_value())
+        minimum_height = get_height(size);
-        {
+        natural_height = minimum_height;
-            impl_->get_size_compact(widget, w, h);
-        }
-        else
-        {
-            impl_->get_size_full(widget, w, h);
-        }
-
-        minimum_height = h;
-        natural_height = h;
     }
 }