Code review comment for lp:~smspillaz/compiz-core/compiz-core.work_923683

Sam Spilsbury (smspillaz) wrote :

For the sake of not making this diff any bigger, I'm not going to introduce unit tests here.

As for testing this I'd say the following is appropriate:

 * Add testcase for PendingConfigureEvent
 * Add testcase for rectsToRegion (serverGeometry should really be DI'd here)

Notes for the future:

 * As alan has said - the whole priv->geometry ().width () + priv->geometry ().border () * 2 is /really/ awkward and error prone, but we need it to support windows like xterm that still use this (stupid, legacy) attribute on their windows. I would say that the role of compiz::window::Geometry should thus be expanded somewhat
   - it should also encapsulate priv->region and really, geometry::x and friends should be made with reference to that. Ideally we'll store two regions, one with borders and one without. That sucks, since its a little memory hungry, but at least the cost is only really born whenever those regions need to be updated (::translate on a region with one rectangle is basically free, slightly more complicated on regions with multiple rects (eg, shaped window). The default should be to return the size /without/ borders, and have a default-parameter enum to get the size with borders.

« Back to merge proposal