GNOME Bugzilla – Bug 775183
Use GtkRevealer instead of GdNotification
Last modified: 2016-11-30 12:08:38 UTC
.
Created attachment 340850 [details] [review] region: Use GtkRevealer instead of GdNotification GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays.
Got before and after screenshots? For a cursory glance, it would be better if the UI code was still in the GtkBuilder file rather than in C. You'll also want to check whether that's the last user of GdNotification, and remove it from the configure.ac in a follow-up patch. Otherwise, make a note that it's not the last user in the commit log.
(In reply to Bastien Nocera from comment #2) > For a cursory glance, it would be better if the UI code was still in the > GtkBuilder file rather than in C. Read the patch the wrong way around. The code is fine, but the other 2 requests below stand. > Got before and after screenshots? and > You'll also want to check whether that's the last user of GdNotification, > and remove it from the configure.ac in a follow-up patch. Otherwise, make a > note that it's not the last user in the commit log.
Created attachment 340975 [details] Screenshot: GdNotification (before) x GtkRevealer (after) (In reply to Bastien Nocera from comment #2) > Got before and after screenshots? here it goes. > You'll also want to check whether that's the last user of GdNotification, > and remove it from the configure.ac in a follow-up patch. Otherwise, make a > note that it's not the last user in the commit log. not yet. We are still using it on the shell/cc-window.c for the GdStyledTextRenderer (to dim-label it). It will eventually go away with the new shell, I guess.
Created attachment 340976 [details] [review] region: Use GtkRevealer instead of GdNotification GtkRevealer combined with the "app-notification" class is enough to represent the notification concept nowadays. This is NOT the last user of libgd in control-center. shell/cc-window.c still requires it for the cell renderer.
(In reply to Felipe Borges from comment #4) > Created attachment 340975 [details] > Screenshot: GdNotification (before) x GtkRevealer (after) > > (In reply to Bastien Nocera from comment #2) > > Got before and after screenshots? > > here it goes. Is it me, or is the transparency level different on those 2 screenshots? > > You'll also want to check whether that's the last user of GdNotification, > > and remove it from the configure.ac in a follow-up patch. Otherwise, make a > > note that it's not the last user in the commit log. > > not yet. We are still using it on the shell/cc-window.c for the > GdStyledTextRenderer (to dim-label it). It will eventually go away with the > new shell, I guess. I meant just GdNotification, so we could do: -LIBGD_INIT([_view-common notification static]) +LIBGD_INIT([_view-common static])
Review of attachment 340976 [details] [review]: Tip-top. Will need to fix the commit message, the reference to the libgd user wasn't what I was mentioning.
(In reply to Bastien Nocera from comment #6) > (In reply to Felipe Borges from comment #4) > > Created attachment 340975 [details] > > Screenshot: GdNotification (before) x GtkRevealer (after) > > > > (In reply to Bastien Nocera from comment #2) > > > Got before and after screenshots? > > > > here it goes. > > Is it me, or is the transparency level different on those 2 screenshots? my screen ain't good enough to see that. :/ anyhow, this is something to tweak in the theme. "app-notification" class. > I meant just GdNotification, so we could do: > -LIBGD_INIT([_view-common notification static]) > +LIBGD_INIT([_view-common static]) Alright. After Bug 775178, there's no need for GdNotification anymore. Patch below.
Created attachment 340980 [details] [review] configure: Drop dependency on GdNotification Since the port to GtkRevealer in bgo#775183 and bgo#775178, GdNotification is no longer needed in control-center.
Comment on attachment 340976 [details] [review] region: Use GtkRevealer instead of GdNotification commit message updated. Attachment 340976 [details] pushed as 6f51428 - region: Use GtkRevealer instead of GdNotification
Review of attachment 340980 [details] [review]: Looks good, but can you please replace the bugzilla numbers with the commit ids?
(In reply to Bastien Nocera from comment #11) > Review of attachment 340980 [details] [review] [review]: > > Looks good, but can you please replace the bugzilla numbers with the commit > ids? sure. I'm gonna push as soon as we are ok to push the patch at bug 775178.
Attachment 340980 [details] pushed as f328227 - configure: Drop dependency on GdNotification