Merge lp://qastaging/~charlesk/libindicator/add-visibility into lp://qastaging/libindicator/0.5

Proposed by Ted Gould
Status: Merged
Approved by: Ted Gould
Approved revision: 440
Merged at revision: 442
Proposed branch: lp://qastaging/~charlesk/libindicator/add-visibility
Merge into: lp://qastaging/libindicator/0.5
Diff against target: 846 lines (+594/-35)
7 files modified
libindicator/indicator-desktop-shortcuts.c (+1/-1)
libindicator/indicator-object.c (+328/-34)
libindicator/indicator-object.h (+13/-0)
tests/Makefile.am (+21/-0)
tests/dummy-indicator-signaler.c (+2/-0)
tests/dummy-indicator-visible.c (+139/-0)
tests/test-loader.c (+90/-0)
To merge this branch: bzr merge lp://qastaging/~charlesk/libindicator/add-visibility
Reviewer Review Type Date Requested Status
Ted Gould (community) Approve
Review via email: mp+88478@code.qastaging.launchpad.net

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

To post a comment you must log in.
Revision history for this message
Charles Kerr (charlesk) wrote :

Since the libindicator release is being pushed until next week, I'd like to defer this merge until early next week -- I see a couple of things in r437 that I'd like to improve before the merge.

Revision history for this message
Ted Gould (ted) wrote :

Some fixes needed:

* I think the param spec for default visibility should be a boolean instead of a string.
* entry_get_private should be static
* default /*ENTRY_INIT*/ should actually be case ENTRY_INIT with the default case being a g_warn_if_reached() for better error reporting.
* It seems like indicator_object_set_visible() should set the value in the private structs of the indicator entries so that if get_entries is called after calling it, the correct list would be returned.
* The signal connect for on_settings_changed() should use a detail string since it's only looking for a single value, so "changed::visible"
* schema_set() needs to (sadly) check to make sure that the schema exists otherwise GSettings will dump the application. You can use g_settings_list_schemas to discover that.

I'm a little concerned about the "limbo code" in that I think it should be the responsibility of the subclassing indicator to handle the reference counting. If we handle it in the superclass, we're always going to end up in the situation where the subclasses don't really know how to completely unref an object. It seems like we should fix that issue there.

Also, with the gsettings code I think this is a good use case for the GSettings paths. So we could make the actual schema in the libindicator package, but without a path. Then we allow the subclassing indicators define the path that the schema exists at. This would give us more guarantee of the entries in the schema and would make it easier for upgrades to that.

review: Needs Fixing
Revision history for this message
Ted Gould (ted) wrote :

Oh, it also needs a test. Or Allan comes to your house and hugs you until you write one :-)

439. By Charles Kerr

another iteration of the indicator-object visibility support patch, incorporating ideas from discussion with ted

 - some functions were public when they should have been private
 - the hide/show handler is now a virtual function & is documented in indicator-object.h
 - added unit tests
 - the GSettings monitor has been removed

440. By Charles Kerr

"bzr merge lp:indicator" + conflict resolution in tests suite

Revision history for this message
Ted Gould (ted) :
review: Approve

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