Merge lp:~mc-return/compiz/compiz.fix1030473-part3 into lp:compiz/0.9.8

Proposed by MC Return
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 3307
Merged at revision: 3304
Proposed branch: lp:~mc-return/compiz/compiz.fix1030473-part3
Merge into: lp:compiz/0.9.8
Diff against target: 75 lines (+7/-8)
4 files modified
gtk/window-decorator/cairo.c (+2/-5)
kde/window-decorator-kde4/switcher.cpp (+1/-1)
libdecoration/decoration.c (+3/-1)
plugins/opengl/src/screen.cpp (+1/-1)
To merge this branch: bzr merge lp:~mc-return/compiz/compiz.fix1030473-part3
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Review via email: mp+118218@code.launchpad.net

Commit message

Reduced the scope of the variable 'icccm_version', removed a break that can never be executed, added the variables 'frameType', 'frameState' and 'frameActions' to decor_quads_to_property in the #else branch in KWD::Switcher::updateWindowProperties () as well and simplified calc_button_size (decor_t *d) in cairo.c.

Description of the change

Reduces the scope of the variable 'icccm_version', removes a break that can never be executed, adds the variables 'frameType', 'frameState' and 'frameActions' to decor_quads_to_property in the #else branch in KWD::Switcher::updateWindowProperties () as well and simplifies calc_button_size (decor_t *d) in cairo.c.

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

8 - unsigned int nOffset = 1, frameType = 0, frameState = 0, frameActions = 0;
9 + unsigned int nOffset = 1, frameType, frameState, frameActions;

Are these guaranteed to be assigned to something we know later in the function? If so, we can move them there and declare and assign them at the same time.

Revision history for this message
MC Return (mc-return) wrote :

> 8 - unsigned int nOffset = 1, frameType = 0, frameState = 0,
> frameActions = 0;
> 9 + unsigned int nOffset = 1, frameType, frameState, frameActions;
>
> Are these guaranteed to be assigned to something we know later in the
> function? If so, we can move them there and declare and assign them at the
> same time.

Hmm. It seems I made a mistake - Maybe cppcheck had a problem with the #ifdef.
But those variables are used only here in the function updateWindowProperties ():

#ifdef QT_45
    decor_quads_to_property (data, nOffset - 1, mX11Pixmap,
        &mBorder, &mBorder, &mBorder, &mBorder,
        0, 0,
        quads, nQuad, frameType, frameState, frameActions);

So theoretically we could remove the declaration of those variables completely and simplify this to:

#ifdef QT_45
    decor_quads_to_property (data, nOffset - 1, mX11Pixmap,
        &mBorder, &mBorder, &mBorder, &mBorder,
        0, 0,
        quads, nQuad, 0, 0, 0);

...as those variables are not used anywhere else in this function and are always 0 anyway.

Unfortunately I am having problems convincing cmake and make to compile the KDE part of Compiz here to test it, but it *should* be okay. Please check the diff.

3305. By MC Return

Simplified updateWindowProperties () by removing redundant declaration of the variables 'frameType', 'frameState' and 'frameActions'

Revision history for this message
MC Return (mc-return) wrote :

Another question:

In compizconfig/gsettings/src/gsettings.c:296] cppcheck throws a warning: Casting between double* and float* which have an incompatible binary data representation

Investigation shows that this is correct, but it could be by design:
First we find this:

    case TypeFloat:
 {
     double *array = malloc (nItems * sizeof (double));

then in line 296:

     list = ccsGetValueListFromFloatArray ((float *) array, nItems, setting);

In compizconfig/libcompizconfig/include/ccs.h line 978 says:

CCSSettingValueList ccsGetValueListFromFloatArray (float *array,
         int num,
         CCSSetting *parent);

Q: Should we fix this and if so how ?

Revision history for this message
MC Return (mc-return) wrote :

Also in /gtk/window-decorator/cairo.c:732 in static void
calc_button_size (decor_t *d) we can find this:

    if (d->actions & (WNCK_WINDOW_ACTION_MINIMIZE |
        WNCK_WINDOW_ACTION_MINIMIZE))
 button_width += 17;

In the function static void calc_button_size (decor_t *d) button_width is first declared with

    gint button_width;

then assigned the value

    button_width = 0;

which could also be reduced to

    gint button_width = 0;

and then button_width is increased by 17 (pixels) for respective available window actions, so if the close action is valid, it is increased by 17 points, then for the maximize/restore button if any of the window actions for maximizing and unmaximizing are available, the third button would always be minimize then and we should fix this by changing:

    if (d->actions & (WNCK_WINDOW_ACTION_MINIMIZE |
        WNCK_WINDOW_ACTION_MINIMIZE))
 button_width += 17;

to

    if (d->actions & WNCK_WINDOW_ACTION_MINIMIZE)
 button_width += 17;

3306. By MC Return

Simplified calc_button_size (decor_t *d) in cairo.c

Revision history for this message
MC Return (mc-return) wrote :

cppcheck also warns about this in /compizconfig/libcompizconfig/src/main.c:4305: (error) Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour.

3307. By MC Return

Redeclared and reassigned frameType, frameState, frameActions unsigned int variables andalso added those to decor_quads_to_property in the #else branch in KWD::Switcher::updateWindowProperties () (switcher.cpp)

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Looks good, and passed smoke testing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'gtk/window-decorator/cairo.c'
2--- gtk/window-decorator/cairo.c 2012-06-22 06:11:26 +0000
3+++ gtk/window-decorator/cairo.c 2012-08-05 14:28:18 +0000
4@@ -716,9 +716,7 @@
5 static void
6 calc_button_size (decor_t *d)
7 {
8- gint button_width;
9-
10- button_width = 0;
11+ gint button_width = 0;
12
13 if (d->actions & WNCK_WINDOW_ACTION_CLOSE)
14 button_width += 17;
15@@ -729,8 +727,7 @@
16 WNCK_WINDOW_ACTION_UNMAXIMIZE_VERTICALLY))
17 button_width += 17;
18
19- if (d->actions & (WNCK_WINDOW_ACTION_MINIMIZE |
20- WNCK_WINDOW_ACTION_MINIMIZE))
21+ if (d->actions & WNCK_WINDOW_ACTION_MINIMIZE)
22 button_width += 17;
23
24 if (button_width)
25
26=== modified file 'kde/window-decorator-kde4/switcher.cpp'
27--- kde/window-decorator-kde4/switcher.cpp 2011-05-07 08:58:43 +0000
28+++ kde/window-decorator-kde4/switcher.cpp 2012-08-05 14:28:18 +0000
29@@ -248,7 +248,7 @@
30 decor_quads_to_property (data, nOffset - 1, mPixmap.handle (),
31 &mBorder, &mBorder, &mBorder, &mBorder,
32 0, 0,
33- quads, nQuad);
34+ quads, nQuad, frameType, frameState, frameActions);
35 #endif
36 KWD::trapXError ();
37 XChangeProperty (QX11Info::display (), mId, Atoms::netWindowDecor,
38
39=== modified file 'libdecoration/decoration.c'
40--- libdecoration/decoration.c 2012-05-10 15:40:25 +0000
41+++ libdecoration/decoration.c 2012-08-05 14:28:18 +0000
42@@ -3137,7 +3137,6 @@
43 #define N_TARGETS 4
44
45 Atom conversion_targets[N_TARGETS];
46- long icccm_version[] = { 2, 0 };
47
48 conversion_targets[0] = XInternAtom (xdisplay, "TARGETS", 0);
49 conversion_targets[1] = XInternAtom (xdisplay, "MULTIPLE", 0);
50@@ -3153,9 +3152,12 @@
51 XA_INTEGER, 32, PropModeReplace,
52 (unsigned char *) &dm_sn_timestamp, 1);
53 else if (target == conversion_targets[3])
54+ {
55+ long icccm_version[] = { 2, 0 };
56 XChangeProperty (xdisplay, w, property,
57 XA_INTEGER, 32, PropModeReplace,
58 (unsigned char *) icccm_version, 2);
59+ }
60 else
61 return 0;
62
63
64=== modified file 'plugins/opengl/src/screen.cpp'
65--- plugins/opengl/src/screen.cpp 2012-07-18 04:03:58 +0000
66+++ plugins/opengl/src/screen.cpp 2012-08-05 14:28:18 +0000
67@@ -656,7 +656,7 @@
68 GLWindow::get (w)->priv->icons.clear ();
69 }
70 break;
71- break;
72+
73 default:
74 if (event->type == cScreen->damageEvent () + XDamageNotify)
75 {

Subscribers

People subscribed via source and target branches