Re: [PATCH] Double Window Borders

[Bastien Dejean <nihilhill _at_ gmail _dot_ com>]

Thorsten WiÃmann:

> was that feature initially requested by you?

Yes it was.

> the code itself works as expected.

While it works as expected, I noticed some flickering when switching the
focus from one window to the other.

I thought it might be related to the *set_double_window_border* function
being called multiple times, and unfortunately it seems it is the case:
I've added an *HSDebug* call in that function and when the focus moves
from window A to window B, the function is called 2 times on A and 3
times on B!

I read the comment in the *window_focus* function: âwindow_focus is
always called twiceâ, why is that? What's the difference between
`window_unfocus(lastfocus)` and `window_unfocus_last()`?

> On Sat, Jul 21, 2012 at 08:30:43PM +0200, Bastien Dejean wrote:
> > +    * new setting: double_window_borders
> > +    * new setting: window_border_inner_color
> 
> It is confusing that the feature is enabled with the "double window
> borders" but controlled via the window_border_inner_color. Maybe the
> "double" vs. "inner" problem is solved simply by adding a sentence like
> "The inner color is controlled via the window_border_inner_color
> setting."
> 
> Another thing that causes confusion is the usage of the plural here.
> It's "window_border_width" and not "window_borders_width", so it should
> be "double_window_border" too (For every window it controls if there's a
> double window border or not).

I'll fix that.

> So we maybe also should rename the smart_*_surroundings to
> smart_*_surrounding. (But maybe surroundings is OK here; if we rename
> that, that would be another commit of course)

No, it should remain like that: *surroundings* is a (singular) noun
while *surrounding* is an adjective.

> >  void window_unfocus(Window window) {
> > -    XSetWindowBorder(g_display, window, g_window_border_normal_color);
> > +    if (*g_double_window_borders)
> > +        set_window_double_border(window, g_window_border_inner_color, g_window_border_normal_color);
> > +    else
> > +        XSetWindowBorder(g_display, window, g_window_border_normal_color);
> 
> You copied this if-else-thing very very often, so you should move that
> part to a helper function
> 
>     void window_update_border(Window window, unsigned long color);
> 
> which performs the if-check. Please also use brackets { } after the if
> and the else statement. (I've forgot to add that rule to the HACKING
> file)
> 
> > +    unsigned int g_depth = DefaultDepth(g_display, DefaultScreen(g_display));
> 
> g_depth neither is global nor a glib Function, so it must not have a g_
> prefix.

Ok, I'll fix that too.

Cheers,
-- 
 b.d
(| |)
 ^ ^


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/