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 750338 - gtkrevealer: add css padding support
gtkrevealer: add css padding support
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-06-03 13:00 UTC by Carlos Soriano
Modified: 2015-06-05 03:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtkrevealer: add css padding support (13.01 KB, patch)
2015-06-03 13:00 UTC, Carlos Soriano
none Details | Review
gtkrevealer: add css padding support (13.03 KB, patch)
2015-06-03 14:17 UTC, Carlos Soriano
none Details | Review
gtkrevealer: add css padding support (15.07 KB, patch)
2015-06-03 15:20 UTC, Carlos Soriano
committed Details | Review

Description Carlos Soriano 2015-06-03 13:00:29 UTC
See patch, necesary for the work on GtkPlacesSidebar to port it to GtkListBox
Comment 1 Carlos Soriano 2015-06-03 13:00:35 UTC
Created attachment 304510 [details] [review]
gtkrevealer: add css padding support

Add css padding support to GtkRevealer.

As a future work, GtkRevealer still needs to support the border
property.
Comment 2 Carlos Soriano 2015-06-03 14:17:05 UTC
Created attachment 304520 [details] [review]
gtkrevealer: add css padding support

Add css padding support to GtkRevealer.

As a future work, GtkRevealer still needs to support the border
property.
Comment 3 Emmanuele Bassi (:ebassi) 2015-06-03 14:30:38 UTC
Review of attachment 304520 [details] [review]:

::: gtk/gtkrevealer.c
@@ +1,3 @@
 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /*
+ * Copyright (c) 2013, 2015 Red Hat, Inc.

Pet peeve: remove the '(c)'. It means 'Copyright', which makes the line:

  Copyright Copyright 2013, 2015 …

@@ +118,2 @@
 static void
+gtk_revealer_get_full_border (GtkRevealer *revealer,

It's a bit confusing to call this function 'get_full_border' and yet only get the padding.

Shouldn't it also get the border?

@@ +330,1 @@
   child_allocation->x = 0;

This should probably have a comment pointing to real_size_allocate() as well.

@@ +517,2 @@
       transition = effective_transition (revealer);
+      /* The child allocation is fixed (it's doesn't modifies with the animation),

"it is not modified by the animation"

@@ +517,3 @@
       transition = effective_transition (revealer);
+      /* The child allocation is fixed (it's doesn't modifies with the animation),
+       * and it's added to the bin_window at 0,0.

"and its origin is relative to bin_window"
Comment 4 Carlos Soriano 2015-06-03 15:20:04 UTC
Created attachment 304528 [details] [review]
gtkrevealer: add css padding support

Add css padding support to GtkRevealer.

As a future work, GtkRevealer still needs to support the border
property.
Comment 5 Carlos Soriano 2015-06-03 15:22:41 UTC
About the border/padding, it's true, the border property is not supported currently, so you are right the name is odd...

About everything else, thanks for the review, had a hard time making that comment for the next one hacking on GtkRevealer.

Also I improved a little the code (extract of the set_width) and also make sure we never ask for a width/height less than 0 when subtracting the paddings.
Comment 6 Matthias Clasen 2015-06-05 03:22:15 UTC
Trying this with tests/testrevealer shows some oddities:

1) The buttons at the bottom appear to hang out of the window by 1 pixel or so

2) When setting padding to 10, the top two buttons don't change their height, the others do. The 'None' button grows once it is clicked.
Comment 7 Matthias Clasen 2015-06-05 03:41:41 UTC
Attachment 304528 [details] pushed as c37f569 - gtkrevealer: add css padding support