GNOME Bugzilla – Bug 777497
Port Boxes.Notification from Gd.Notification to GtkRevealer
Last modified: 2017-01-30 11:50:38 UTC
In the latest Core Apps Hackfest in November 2016, we discussed moving away from libgd to new Gtk+ widgets which are capable to provide the same features (the same way we did in 2530459), in doing so reducing more our dependency on libgd. GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays.
Created attachment 343803 [details] [review] notificationbar: Remove usage of Gd.Notification GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement the notification widget.
Created attachment 343804 [details] [review] machine: Remove usage of Gd.Notification GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement the notification widget.
Created attachment 343805 [details] [review] libvirt-machine: Remove usage of Gd.Notification GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement the notification widget.
Created attachment 343806 [details] [review] notification: Port Boxes.Notification to GtkRevealer GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. We no longer need to use Gd.Notification.
(In reply to Felipe Borges from comment #2) > Created attachment 343804 [details] [review] [review] > machine: Remove usage of Gd.Notification > > GtkRevealer combined with the "app-notification" class is enough > to represent the notification concept nowadays. > > This patch removes the references to the Boxes.Notification base > class: Gd.Notification. The following patches will reimplement > the notification widget. this one specifically can wait for the port of Boxes.AuthNotification from Gd.Notification to Gtk.Revealer as well.
Review of attachment 343803 [details] [review]: 1. Good work with commit messages but when writing the commit message, keep in mind that your primary focus should be the patch itself, rather than the bigger picture. This first para in this (and other similar patches) is confusing cause it's talking about the final change, rather than this one: "GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays." 2. Patches removing Gd.Notification references seem one logical change to me so I'd suggest squashing them.
Created attachment 344133 [details] [review] Remove references to Gd.Notification This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement the notification widget.
Created attachment 344134 [details] [review] notification: Port Boxes.Notification to GtkRevealer GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. We no longer need to use Gd.Notification.
Review of attachment 344133 [details] [review]: Almost there but you know me. :) Need to be a bit more specific than "the notification widget".
Review of attachment 344134 [details] [review]: Cool, would be nice to add reason for wanting to remove Gd.Notification dependency. E.g "This is part of the effort to drop the dependency on libgd". ::: src/notification.vala @@ +27,3 @@ owned DismissFunc? dismiss_func, int timeout) { + set_reveal_child (true); // FIXME: Why this won't work from .ui file? Recommend no "FIXME" without a bug reference so it can be removed when it's not needed or if explained the reason.
(In reply to Zeeshan Ali (Khattak) from comment #10) > Review of attachment 344134 [details] [review] [review]: > > Cool, would be nice to add reason for wanting to remove Gd.Notification > dependency. E.g "This is part of the effort to drop the dependency on libgd". > > ::: src/notification.vala > @@ +27,3 @@ > owned DismissFunc? dismiss_func, > int timeout) { > + set_reveal_child (true); // FIXME: Why this won't work from .ui > file? > > Recommend no "FIXME" without a bug reference so it can be removed when it's > not needed or if explained the reason. IMO it is more like a semantic bug. Since we are using templates and setting the property in the template, "reveal-child" as a "set construct" type of property might be prevented from being set by a child class. I would just go with the setter method (set_reveal_child) here, since we are also calling it the same way in the dismiss method.
Created attachment 344203 [details] [review] Remove references to Gd.Notification This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement Boxes.Notification using Gtk.Revealer instead.
Created attachment 344204 [details] [review] notification: Port Boxes.Notification to GtkRevealer GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This is part of the effort to drop the dependency on libgd.
(In reply to Felipe Borges from comment #11) > (In reply to Zeeshan Ali (Khattak) from comment #10) > > Review of attachment 344134 [details] [review] [review] [review]: > > > > Cool, would be nice to add reason for wanting to remove Gd.Notification > > dependency. E.g "This is part of the effort to drop the dependency on libgd". > > > > ::: src/notification.vala > > @@ +27,3 @@ > > owned DismissFunc? dismiss_func, > > int timeout) { > > + set_reveal_child (true); // FIXME: Why this won't work from .ui > > file? > > > > Recommend no "FIXME" without a bug reference so it can be removed when it's > > not needed or if explained the reason. > > IMO it is more like a semantic bug. Since we are using templates and setting > the property in the template, "reveal-child" as a "set construct" type of > property might be prevented from being set by a child class. Well if you know the reasons, then comment should have a summary of your reasons rather than FIXME. :)
Created attachment 344512 [details] [review] Remove references to Gd.Notification This patch removes the references to the Boxes.Notification base class: Gd.Notification. The following patches will reimplement Boxes.Notification using Gtk.Revealer instead.
Created attachment 344513 [details] [review] notification: Port Boxes.Notification to GtkRevealer GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This is part of the effort to drop the dependency on libgd.
Created attachment 344514 [details] [review] notification: Port Boxes.Notification to GtkRevealer GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This is part of the effort to drop the dependency on libgd.
Review of attachment 344512 [details] [review]: "Boxes.Notification base class" -> "base class of Boxes.Notification"
Review of attachment 344514 [details] [review]: ack
Attachment 344512 [details] pushed as 597c6e0 - Remove references to Gd.Notification Attachment 344514 [details] pushed as 2c85bc0 - notification: Port Boxes.Notification to GtkRevealer