Merge lp://qastaging/~compiz-team/compiz/compiz.gtk-window-decorator-gsettings into lp://qastaging/compiz/0.9.8

Proposed by Sam Spilsbury
Status: Superseded
Proposed branch: lp://qastaging/~compiz-team/compiz/compiz.gtk-window-decorator-gsettings
Merge into: lp://qastaging/compiz/0.9.8
Diff against target: 8539 lines (+6874/-913)
48 files modified
cmake/CompizGSettings.cmake (+8/-2)
gtk/window-decorator/CMakeLists.txt (+70/-1)
gtk/window-decorator/blurprops.c (+5/-2)
gtk/window-decorator/cairo.c (+2/-2)
gtk/window-decorator/decorator.c (+17/-33)
gtk/window-decorator/events.c (+23/-10)
gtk/window-decorator/frames.c (+7/-5)
gtk/window-decorator/gtk-window-decorator.c (+52/-122)
gtk/window-decorator/gtk-window-decorator.h (+22/-65)
gtk/window-decorator/gwd-settings-interface.c (+162/-0)
gtk/window-decorator/gwd-settings-interface.h (+102/-0)
gtk/window-decorator/gwd-settings-notified-interface.c (+37/-0)
gtk/window-decorator/gwd-settings-notified-interface.h (+63/-0)
gtk/window-decorator/gwd-settings-notified.c (+264/-0)
gtk/window-decorator/gwd-settings-notified.h (+37/-0)
gtk/window-decorator/gwd-settings-storage-gconf.c (+387/-0)
gtk/window-decorator/gwd-settings-storage-gconf.h (+55/-0)
gtk/window-decorator/gwd-settings-storage-gsettings.c (+531/-0)
gtk/window-decorator/gwd-settings-storage-gsettings.h (+84/-0)
gtk/window-decorator/gwd-settings-storage-interface.c (+63/-0)
gtk/window-decorator/gwd-settings-storage-interface.h (+66/-0)
gtk/window-decorator/gwd-settings-writable-interface.c (+134/-0)
gtk/window-decorator/gwd-settings-writable-interface.h (+142/-0)
gtk/window-decorator/gwd-settings-xproperty-interface.c (+16/-0)
gtk/window-decorator/gwd-settings-xproperty-interface.h (+51/-0)
gtk/window-decorator/gwd-settings-xproperty-storage.c (+261/-0)
gtk/window-decorator/gwd-settings-xproperty-storage.h (+39/-0)
gtk/window-decorator/gwd-settings.c (+911/-0)
gtk/window-decorator/gwd-settings.h (+41/-0)
gtk/window-decorator/metacity.c (+59/-39)
gtk/window-decorator/org.compiz.gwd.gschema.xml (+47/-0)
gtk/window-decorator/settings.c (+76/-631)
gtk/window-decorator/tests/CMakeLists.txt (+114/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings.cpp (+186/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings.h (+86/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_notified.cpp (+173/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_notified.h (+70/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_storage.cpp (+192/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_storage.h (+79/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_writable.cpp (+277/-0)
gtk/window-decorator/tests/compiz_gwd_mock_settings_writable.h (+107/-0)
gtk/window-decorator/tests/compiz_gwd_tests.h.in (+12/-0)
gtk/window-decorator/tests/org.gnome.desktop.wm.preferences.gschema.xml (+72/-0)
gtk/window-decorator/tests/org.gnome.mutter.gschema.xml (+18/-0)
gtk/window-decorator/tests/test_gwd_settings.cpp (+1635/-0)
gtk/window-decorator/wnck.c (+4/-1)
include/decoration.h (+4/-0)
libdecoration/decoration.c (+11/-0)
To merge this branch: bzr merge lp://qastaging/~compiz-team/compiz/compiz.gtk-window-decorator-gsettings
Reviewer Review Type Date Requested Status
jenkins (community) continuous-integration Approve
Daniel van Vugt Approve
Compiz Maintainers Pending
Review via email: mp+122517@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-09-01.

This proposal has been superseded by a proposal from 2012-09-05.

Commit message

This branch transitions gtk-window-decorator over to use GSettings and adds a testing framework for the options code.
(LP: #1042323)

1) Introduced three new interfaces (GWDSettingsNotifiedInterface, GWDSettingsWritableInterface, GWDSettingsInterface, GWDSettingsStorageInterface
2) GWDSettingsStorageInterface represents the actual method for settings to be read from a configuration backend, eg GSettings or GConf
3) GWDSettingsWritableInterface is injected into GWDSettingsStorageGConf and GWDSettingsStorageGSettings, and its methods are called when changes occur on the backend
4) GWDSettings is the main object for storing all settings in memory and provides an interface for them being written into
5) GWDSettingsNotifiedInterface provides a callback mechanism for GWDSettings to update the main program state whenever a setting changes.

Description of the change

This branch transitions gtk-window-decorator over to use GSettings and adds a testing framework for the options code.

1) Introduced three new interfaces (GWDSettingsNotifiedInterface, GWDSettingsWritableInterface, GWDSettingsInterface, GWDSettingsStorageInterface
2) GWDSettingsStorageInterface represents the actual method for settings to be read from a configuration backend, eg GSettings or GConf
3) GWDSettingsWritableInterface is injected into GWDSettingsStorageGConf and GWDSettingsStorageGSettings, and its methods are called when changes occur on the backend
4) GWDSettings is the main object for storing all settings in memory and provides an interface for them being written into
5) GWDSettingsNotifiedInterface provides a callback mechanism for GWDSettings to update the main program state whenever a setting changes.

Fixes LP #1042323

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Can't build, for some reason, on precise:

[ 19%] Recompiling GSettings schemas locally
/bin/sh: 1: --targetdir=/home/dan/bzr/compiz/tmp.gwd/build/generated/glib-2.0/schemas/: not found
make[2]: *** [generated/glib-2.0/schemas/gschemas.compiled] Error 127
make[1]: *** [gtk/window-decorator/CMakeFiles/compiz_gsettings_compile_local.dir/all] Error 2

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

And again, please make sure upper-case is only used for macros. Everything else should have some lower case.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks I'll look into that.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

null

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Hi,

So it looks like we both fixed the build issue at the same time. As for the constants - I started to change all of the uppercase ones to lowercase but decided only to do it for constants which should have been function-local anyways.

Generally speaking I don't like working with global data unless its constant - macros have been historically upper case for this reason, and I think that the global constants fit into this category as well. The fact that they are uppercase immediately alerts the maintainer that they are working with program-local data.

I think it also aides in the case of confusing global data for local data. For example:

const gchar *org_foo = "org.foo";

...

GSettings *
instantiate_some_schema_foo ()
{
    GSettings *foo_schema = g_settings_new (org_foo);
    return foo_schema;
}

as opposed to:

const gchar *ORG_FOO = "org.foo"

...

GSettings *
instantiate_some_schema_foo ()
{
    GSettings *foo_schema = g_settings_new (ORG_FOO);
    return foo_schema;
}

Its quite obvious that ORG_FOO has some special meaning in the second case just by looking at the function alone, wheras in the first case one might be left scratching their head as to where org_foo came from. In addition, there's also no worry about shadowing conflicts

const gchar *org_foo = "org.foo"

...

GSettings *
instantiate_some_schema_foo ()
{
    GSettings *org_foo = g_settings_new (org_foo);
    return org_foo;
}

If one used the latter, and stuck to the "global constant data is uppercase, everything else is lowercase" rule there'd be no room for naming conflicts.

That's why I haven't changed the constants that are global like ORG_COMPIZ_GWD etc.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I disagree because:

1. Finding where something is defined is no easier or harder if it is upper or lower case.

2. All C/C++ programmers are used to the convention of uppercase for macros only. So you should not break peoples' mental model. Although, very often you still need to know if MY_STRING_CONSTANT is addressable, I admit.

3. const gchar *ORG_FOO is a variable. I think you meant to make it a constant. Which should be either:
     const gchar * const ORG_FOO = "org.foo";
or
     const gchar ORG_FOO[] = "org_foo";

4. Your masking example should actually just be:
    return g_settings_new (org_foo);
which has no masking.

But enough pedantism. I won't block on this issue.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

The following tests FAILED:
   6 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetUseTooltips/0 (Failed)
   7 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetDraggableBorderWidth/0 (Failed)
   8 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetAttachModalDialogs/0 (Failed)
   9 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetBlur/0 (Failed)
  10 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetButtonLayout/0 (Failed)
  11 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetOpacity/0 (Failed)
  12 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetMetacityTheme/0 (SEGFAULT)
  13 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetFont/0 (SEGFAULT)
  14 - GSettingsStorageUpdates/GWDSettingsTestStorageUpdates.TestSetTitlebarActions/0 (SEGFAULT)
Errors while running CTest

Please be sure to Resubmit new revisions, and not make changes while Status is "Needs review"

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Now compiles and passes test cases.

Unfortunately my window decorations are messed up now. It's not applying my theme properly, showing the [-][+][x] on the right with large spaces between each button. I can see a button for the window menu on the left, which I shouldn't. And the buttons/title jitter their position depending on window focus and mouse-over the title bar. Very broken.

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

> Now compiles and passes test cases.
>
> Unfortunately my window decorations are messed up now. It's not applying my
> theme properly, showing the [-][+][x] on the right with large spaces between
> each button. I can see a button for the window menu on the left, which I
> shouldn't. And the buttons/title jitter their position depending on window
> focus and mouse-over the title bar. Very broken.

Right, I hit this issue initially too :)

I forgot to mention in the description that Ubuntu hasn't updated the gnome keys for their settings changes, so you will need to change the value of org.gnome.desktop.wm.preferences button-layout to close:maximize:minimize: (or whatever the syntax is, at the moment I don't remember and don't have access to an ubuntu machine)

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Confirmed workaround for the moving decoration buttons:
    gsettings set org.gnome.desktop.wm.preferences button-layout 'close,minimize,maximize:'

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

gtk-window-decorator now crashes under valgrind. This does not happen using trunk...

==29769== Jump to the invalid address stated on the next line
==29769== at 0x0: ???
==29769== by 0x41DFDC: set_frame_scale (gwd-settings-notified.c:63)
==29769== by 0x410EEA: decor_frame_refresh (frames.c:49)
==29769== by 0x411059: gwd_get_decor_frame (frames.c:102)
==29769== by 0x41CC31: update_default_decorations (decorator.c:1317)
==29769== by 0x419A8F: decorations_changed (wnck.c:198)
==29769== by 0x41DECB: gwd_settings_notified_impl_update_decorations (gwd-settings-notified.c:46)
==29769== by 0x6E8DF96: g_list_foreach (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3200.3)
==29769== by 0x41F1B1: release_notify_funcs (gwd-settings.c:132)
==29769== by 0x40E293: main (gtk-window-decorator.c:382)
==29769== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==29769==
==29769==
==29769== Process terminating with default action of signal 11 (SIGSEGV)
==29769== Bad permissions for mapped region at address 0x0
==29769== at 0x0: ???
==29769== by 0x41DFDC: set_frame_scale (gwd-settings-notified.c:63)
==29769== by 0x410EEA: decor_frame_refresh (frames.c:49)
==29769== by 0x411059: gwd_get_decor_frame (frames.c:102)
==29769== by 0x41CC31: update_default_decorations (decorator.c:1317)
==29769== by 0x419A8F: decorations_changed (wnck.c:198)
==29769== by 0x41DECB: gwd_settings_notified_impl_update_decorations (gwd-settings-notified.c:46)
==29769== by 0x6E8DF96: g_list_foreach (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3200.3)
==29769== by 0x41F1B1: release_notify_funcs (gwd-settings.c:132)
==29769== by 0x40E293: main (gtk-window-decorator.c:382)
==29769==

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

I'm assuming this is using precise again? I'll take another look into it I guess ...

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

That one should be reproducible with valgrind on any distro/compiler.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Okay, was an oversight in the workaround code. Test added and pushed.

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

I can't say it's reviewed, because it's not. Just too big. But I can't break it any more, so that's good enough.

Still, it would be nice if someone else had a look before this was fully approved.

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

Running my eyes over the code again, just the same old style complaints from me:

1. Please format the code to look nice in 80-column windows.

2. Please don't use long identifiers, like:
gwd_settings_storage_gsettings_update_draggable_border_width
The name alone takes up almost the whole line in my editor.

3. Please be careful with your tabs. Mismatching tabs and spaces are causing misalignments in some cases as observed below.

4. Variable declarations in the middle of a block are not valid for C89 at least.

5. Capitalized identifiers should only be used for macros.

6. Some source files have correct copyrights, but are missing the author's name.

7. Reviewing code should also be about understanding the context and implications of code. In this way, 8541 lines is not reviewable.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

On Wed, 5 Sep 2012, Daniel van Vugt wrote:

> Running my eyes over the code again, just the same old style complaints from me:
>
> 1. Please format the code to look nice in 80-column windows.

I think we should move to 120 columns, 80 columns if very restrictive and
doesn't allow for descriptive names. Discussion for another day though.

>
> 3. Please be careful with your tabs. Mismatching tabs and spaces are causing misalignments in some cases as observed below.
>
> 4. Variable declarations in the middle of a block are not valid for C89 at least.
>
> 6. Some source files have correct copyrights, but are missing the author's name.
>

I'll propose a merge to clean these out later today. Thanks.
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.gtk-window-decorator-gsettings/+merge/122517
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~compiz-team/compiz/compiz.gtk-window-decorator-gsettings into lp:compiz.
>

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Needs Review while the distro team evaluates the FFe

Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Needs Fixing (continuous-integration)
3415. By Sam Spilsbury

Merge lp:compiz

3416. By Sam Spilsbury

Fix typo which causing org.gnome.mutter not to be used in tests.

Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches