After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775183 - Use GtkRevealer instead of GdNotification
Use GtkRevealer instead of GdNotification
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Region & Language
git master
Other Linux
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-27 16:24 UTC by Felipe Borges
Modified: 2016-11-30 12:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
region: Use GtkRevealer instead of GdNotification (7.04 KB, patch)
2016-11-27 16:24 UTC, Felipe Borges
none Details | Review
Screenshot: GdNotification (before) x GtkRevealer (after) (53.03 KB, image/png)
2016-11-29 14:13 UTC, Felipe Borges
  Details
region: Use GtkRevealer instead of GdNotification (7.15 KB, patch)
2016-11-29 14:14 UTC, Felipe Borges
committed Details | Review
configure: Drop dependency on GdNotification (884 bytes, patch)
2016-11-29 14:48 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2016-11-27 16:24:15 UTC
.
Comment 1 Felipe Borges 2016-11-27 16:24:39 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.
Comment 2 Bastien Nocera 2016-11-27 17:54:09 UTC
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.
Comment 3 Bastien Nocera 2016-11-28 00:41:55 UTC
(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.
Comment 4 Felipe Borges 2016-11-29 14:13:54 UTC
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.
Comment 5 Felipe Borges 2016-11-29 14:14:11 UTC
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.
Comment 6 Bastien Nocera 2016-11-29 14:26:40 UTC
(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])
Comment 7 Bastien Nocera 2016-11-29 14:28:31 UTC
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.
Comment 8 Felipe Borges 2016-11-29 14:48:17 UTC
(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.
Comment 9 Felipe Borges 2016-11-29 14:48:30 UTC
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 10 Felipe Borges 2016-11-29 14:52:03 UTC
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
Comment 11 Bastien Nocera 2016-11-29 15:11:25 UTC
Review of attachment 340980 [details] [review]:

Looks good, but can you please replace the bugzilla numbers with the commit ids?
Comment 12 Felipe Borges 2016-11-29 15:22:33 UTC
(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.
Comment 13 Felipe Borges 2016-11-30 12:08:33 UTC
Attachment 340980 [details] pushed as f328227 - configure: Drop dependency on GdNotification