Fix & refactor notifications (#284)

* fix 283 * refactor notifications * fix use-after-free in `pixbufFromVariant` * formatting * notifications: scale large images * notifications: optimize pixbufFromVariant
WayfireWM · Jan 13, 2025 · aaadead · aaadead
1 parent f49f719
commit aaadead
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 50 deletions.
diff --git a/src/panel/widgets/notifications/daemon.cpp b/src/panel/widgets/notifications/daemon.cpp
@@ -86,9 +86,9 @@ try {
     {
         signal_notification_new.emit(id);
     }
-} catch (const std::invalid_argument & err)
+} catch (const std::exception & err)
 {
-    std::cerr << "Error at " << __PRETTY_FUNCTION__ << ": " << err.what() << std::endl;
+    std::cerr << "Error at " << __PRETTY_FUNCTION__ << ": " << err.what() << '\n';
 }
 
 dbus_method(Daemon::CloseNotification)

diff --git a/src/panel/widgets/notifications/notification-info.cpp b/src/panel/widgets/notifications/notification-info.cpp
@@ -2,23 +2,23 @@
 
 #include <gdkmm.h>
 
-template<class T>
-static void iterTo(Glib::VariantIter & iter, T & to)
+namespace
 {
-    Glib::Variant<T> var;
-    iter.next_value(var);
-    to = var.get();
+template<class... T>
+void extractValues(const Glib::VariantBase & variant, T&... values)
+{
+    std::tie(values...) = Glib::VariantBase::cast_dynamic<Glib::Variant<std::tuple<T...>>>(variant).get();
 }
 
 template<class K>
-static K getHint(const std::map<std::string, Glib::VariantBase> & map, const std::string & key)
+K getHint(const std::map<std::string, Glib::VariantBase> & map, const std::string & key)
 {
     if (map.count(key) != 0)
     {
         const auto & val = map.at(key);
         if (val.is_of_type(Glib::Variant<K>::variant_type()))
         {
-            return Glib::VariantBase::cast_dynamic<Glib::Variant<K>>(map.at(key)).get();
+            return Glib::VariantBase::cast_dynamic<Glib::Variant<K>>(val).get();
         }
     }
 
@@ -27,41 +27,29 @@ static K getHint(const std::map<std::string, Glib::VariantBase> & map, const std
 
 Glib::RefPtr<Gdk::Pixbuf> pixbufFromVariant(const Glib::VariantBase & variant)
 {
-    if (!variant.is_of_type(Glib::VariantType("iiibiiay")))
-    {
-        throw std::invalid_argument("Cannot create pixbuf from variant.");
-    }
-
-    auto iter = Glib::VariantIter(variant);
     gint32 width;
     gint32 height;
     gint32 rowstride;
     bool has_alpha;
     gint32 bits_per_sample;
     gint32 channels;
-
-    iterTo(iter, width);
-    iterTo(iter, height);
-    iterTo(iter, rowstride);
-    iterTo(iter, has_alpha);
-    iterTo(iter, bits_per_sample);
-    iterTo(iter, channels);
-
-    Glib::VariantBase data_var;
-    iter.next_value(data_var);
+    std::vector<guint8> data;
+    extractValues(variant, width, height, rowstride, has_alpha, bits_per_sample, channels, data);
 
     // for integer positive A, floor((A + 7) / 8) = ceil(A / 8)
     gulong pixel_size = (channels * bits_per_sample + 7) / 8;
-    if (data_var.get_size() != ((gulong)height - 1) * (gulong)rowstride + (gulong)width * pixel_size)
+    if (data.size() != ((gulong)height - 1) * (gulong)rowstride + (gulong)width * pixel_size)
     {
         throw std::invalid_argument(
             "Cannot create pixbuf from variant: expected data size doesn't equal actual one.");
     }
 
-    const auto *data = (guint8*)(g_memdup2(data_var.get_data(), data_var.get_size()));
-    return Gdk::Pixbuf::create_from_data(data, Gdk::COLORSPACE_RGB, has_alpha, bits_per_sample, width, height,
-        rowstride);
+    auto *data_ptr = new auto(std::move(data));
+    return Gdk::Pixbuf::create_from_data(data_ptr->data(),
+        Gdk::COLORSPACE_RGB, has_alpha, bits_per_sample, width, height,
+        rowstride, [data_ptr] (auto*) { delete data_ptr; });
 }
+} // namespace
 
 Notification::Hints::Hints(const std::map<std::string, Glib::VariantBase> & map)
 {
@@ -73,7 +61,7 @@ Notification::Hints::Hints(const std::map<std::string, Glib::VariantBase> & map)
         image_data = pixbufFromVariant(map.at("image-data"));
     } else if (map.count("icon_data") != 0)
     {
-        image_data = pixbufFromVariant(map.at("image_data"));
+        image_data = pixbufFromVariant(map.at("icon_data"));
     }
 
     image_path = getHint<Glib::ustring>(map, "image-path");
@@ -94,29 +82,13 @@ Notification::Hints::Hints(const std::map<std::string, Glib::VariantBase> & map)
 
 Notification::Notification(const Glib::VariantContainerBase & parameters, const Glib::ustring & sender)
 {
-    static const auto REQUIRED_TYPE = Glib::VariantType("(susssasa{sv}i)");
-    if (!parameters.is_of_type(REQUIRED_TYPE))
-    {
-        throw std::invalid_argument("NotificationInfo: parameters type must be (susssasa{sv}i)");
-    }
-
-    Glib::VariantBase params_var;
-    parameters.get_normal_form(params_var);
-    auto iter = Glib::VariantIter(params_var);
-    iterTo(iter, app_name);
-    iterTo(iter, id);
+    std::map<std::string, Glib::VariantBase> hints_map;
+    extractValues(parameters, app_name, id, app_icon, summary, body, actions, hints_map, expire_time);
     if (id == 0)
     {
         id = ++Notification::notifications_count;
     }
 
-    iterTo(iter, app_icon);
-    iterTo(iter, summary);
-    iterTo(iter, body);
-    iterTo(iter, actions);
-
-    std::map<std::string, Glib::VariantBase> hints_map;
-    iterTo(iter, hints_map);
     hints = Hints(hints_map);
 
     additional_info.recv_time = std::time(nullptr);

diff --git a/src/panel/widgets/notifications/single-notification.cpp b/src/panel/widgets/notifications/single-notification.cpp
@@ -97,17 +97,29 @@ WfSingleNotification::WfSingleNotification(const Notification & notification)
 
     child.add(top_bar);
 
+    Gtk::IconSize body_image_size = Gtk::ICON_SIZE_DIALOG;
     if (notification.hints.image_data)
     {
-        image.set(notification.hints.image_data);
+        int width;
+        int height;
+        Gtk::IconSize::lookup(body_image_size, width, height);
+
+        auto image_pixbuf = notification.hints.image_data;
+        if (image_pixbuf->get_width() > width)
+        {
+            image_pixbuf = image_pixbuf->scale_simple(width,
+                width * image_pixbuf->get_height() / image_pixbuf->get_width(), Gdk::INTERP_BILINEAR);
+        }
+
+        image.set(image_pixbuf);
     } else if (!notification.hints.image_path.empty())
     {
         if (is_file_uri(notification.hints.image_path))
         {
             image.set(path_from_uri(notification.hints.image_path));
         } else
         {
-            image.set_from_icon_name(notification.hints.image_path, Gtk::ICON_SIZE_DIALOG);
+            image.set_from_icon_name(notification.hints.image_path, body_image_size);
         }
     }