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 631091 - st-theme-node: Support non-uniform border-radii
st-theme-node: Support non-uniform border-radii
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Owen Taylor
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-10-01 13:09 UTC by Florian Müllner
Modified: 2010-10-04 22:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-theme-node: Support non-uniform border-radii (13.93 KB, patch)
2010-10-01 13:09 UTC, Florian Müllner
none Details | Review
st-theme-node: Support non-uniform border-radii (15.14 KB, patch)
2010-10-01 14:35 UTC, Florian Müllner
needs-work Details | Review
st-theme-node: Support non-uniform border-radii (15.11 KB, patch)
2010-10-01 16:08 UTC, Florian Müllner
none Details | Review
tests: Fix environment (1.43 KB, patch)
2010-10-03 12:06 UTC, Florian Müllner
none Details | Review
st-theme-node: Support non-uniform border-radii (19.85 KB, patch)
2010-10-03 12:07 UTC, Florian Müllner
reviewed Details | Review
tests: Fix environment (1.06 KB, patch)
2010-10-04 22:07 UTC, Florian Müllner
committed Details | Review
st-theme-node: Support non-uniform border-radii (17.67 KB, patch)
2010-10-04 22:21 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2010-10-01 13:09:34 UTC
See patch.
Comment 1 Florian Müllner 2010-10-01 13:09:36 UTC
Created attachment 171489 [details] [review]
st-theme-node: Support non-uniform border-radii

Non-uniform border-radii are already supported when using a gradient
background, this patch adds support for solid colors as well.

The currently applied technique of using corner textures and filling
the remaining area with rectangles is extended, so that each corner is
padded with rectangles to the size of the largest corner.
Comment 2 Florian Müllner 2010-10-01 14:35:29 UTC
Created attachment 171495 [details] [review]
st-theme-node: Support non-uniform border-radii

Looks like we currently leak the corner texture, so add unref.
Comment 3 Owen Taylor 2010-10-01 16:00:19 UTC
Review of attachment 171495 [details] [review]:

Except for one comment, looks good to me.

(Long term, I'm not sure this is tenable - I think we'll have to break down and spend the time to do something like https://bugzilla.gnome.org/show_bug.cgi?id=607500#c32 at some point.)

I would like to see some tests of this in tests/interactive - it appears however, that tests/interactive is broken at the moment because of the use of global.set_property_mutable.

I think it would be OK to make environment.js:_blockMethod() do:

 if (global === undefined) // test environment
     return;

and similar for the other reference to global in there. (separate patch)

::: src/st/st-theme-node-drawing.c
@@ +803,3 @@
+  /* We have painted all corners as if they had a radius of max_border_radius;
+   * now we add padding to those corners with a smaller radius, which allows
+   * us to treat corners as uniform further on.

'We have painted all corners as if they had a radius of max_border_radius;' - confused by that. It seems that you painted the corners with their natural size ofmax_width_radius[<corner>]
Comment 4 Florian Müllner 2010-10-01 16:08:47 UTC
Created attachment 171513 [details] [review]
st-theme-node: Support non-uniform border-radii

Updated comment - I'll have a look at writing a test later on.
Comment 5 Florian Müllner 2010-10-03 12:06:57 UTC
Created attachment 171622 [details] [review]
tests: Fix environment

Environment.init() uses Shell.Global, which is not accessible outside
the mutter process; allowing to run the function when window.global is
undefined fixes the environment for tests.
Comment 6 Florian Müllner 2010-10-03 12:07:21 UTC
Created attachment 171623 [details] [review]
st-theme-node: Support non-uniform border-radii

Added test case.
Comment 7 Owen Taylor 2010-10-04 21:37:41 UTC
Review of attachment 171622 [details] [review]:

::: js/ui/environment.js
@@ +85,3 @@
 
+    if (global === undefined) // test environment
+        return;

Early return here is error-prone for future additions. Obsoleted by my other comment, but otherwise I'd suggest moving all the blocking stuff into a function _blockMethods()

::: tests/testcommon/ui.js
@@ +10,2 @@
 function init() {
+    window.global = undefined;

Don't like this - it's pretending that referencing an undefined global works like I thought worked, but it doesn't.

I think my new preference for an approach is in the UI setup code:

 window.global = {
     set_property_mutable : function() {}
 };
Comment 8 Owen Taylor 2010-10-04 21:42:44 UTC
Review of attachment 171623 [details] [review]:

::: tests/interactive/border-radius.js
@@ +25,3 @@
+                               + 'background: white; '
+                               + 'border-radius: 0px 5px 10px 15px;'
+                               + 'padding: 5px;' }), { x_fill: false });

Doens't particularly matter here, but if these are actually all identical here, would be a lot easier to read if it was:

 add_test_case("0px 5px 10px 15px");

Or something like that
Comment 9 Florian Müllner 2010-10-04 22:07:06 UTC
Created attachment 171725 [details] [review]
tests: Fix environment

Updated according to suggestion in IRC.
Comment 10 Owen Taylor 2010-10-04 22:09:41 UTC
Review of attachment 171725 [details] [review]:

Fine
Comment 11 Florian Müllner 2010-10-04 22:21:30 UTC
Created attachment 171726 [details] [review]
st-theme-node: Support non-uniform border-radii

(In reply to comment #8)
> would be a lot easier to read if it was:
> 
>  add_test_case("0px 5px 10px 15px");
> 
> Or something like that

Done.
Comment 12 Florian Müllner 2010-10-04 22:23:09 UTC
Comment on attachment 171725 [details] [review]
tests: Fix environment

Attachment 171725 [details] pushed as b33ebb4 - tests: Fix environment
Comment 13 Owen Taylor 2010-10-04 22:31:46 UTC
Review of attachment 171726 [details] [review]:

Looks good.
Comment 14 Florian Müllner 2010-10-04 22:38:43 UTC
Attachment 171726 [details] pushed as 3d2d396 - st-theme-node: Support non-uniform border-radii