Indicator Applet

Merge lp://staging/~jconti/indicator-applet/gnome3 into lp://staging/indicator-applet/0.4

gnome3
Merge into applet

Proposed by Raphaël Hertzog on 2011-11-01

Status:

Superseded

Proposed branch:

lp://staging/~jconti/indicator-applet/gnome3

Merge into:

lp://staging/indicator-applet/0.4

Diff against target:

1835 lines (+732/-741)

8 files modified

.bzrignore (+3/-0)
configure.ac (+13/-8)
data/Makefile.am (+1/-1)
src/Makefile.am (+8/-0)
src/applet-main.c (+694/-718)
src/eggaccelerators.c (+9/-9)
src/eggaccelerators.h (+1/-1)
src/tomboykeybinder.c (+3/-4)

To merge this branch:

bzr merge lp://staging/~jconti/indicator-applet/gnome3

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Sebastian Geiger (community)	code review and test		Approve on 2012-02-03
Ted Gould (community)		2011-11-01	Approve on 2011-11-28
Marco Trevisan (Treviño)			Approve on 2011-11-26
Dmitry Shachnev			Approve on 2011-11-01
Review via email: mp+80877@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2012-02-07.

Description of the change

It looks like this branch is the only code base that works with GNOME3. Someone should take the responsibility to merge it and to make a new official release of it.

Without this release, the package is going to be dropped from Debian testing (unless we decide to package this branch directly, but it would be weird to have to do that...).

I put Ted as reviewer since he's the only upstream person that has worked on a port.

Revision history for this message

Dmitry Shachnev (mitya57) wrote on 2011-11-01:

Builds and works for me™, though not for all (see https://bugs.launchpad.net/indicator-applet/+bug/724369/comments/27).

Note that my branch lp:~mitya57/indicator-session/fix-lp-881832 should be merged as well to get this working properly.

review: Approve

Revision history for this message

Sebastien Bacher (seb128) wrote on 2011-11-01:

Thanks for the work there, the people working on this code are at the ubuntu summit this week so it might take a few extra fays before getting a review but it's on the list of things to review

Revision history for this message

Jason Conti (jconti) wrote on 2011-11-01:

I unfortunately converted the tabs to spaces in src/applet-main.c, which makes the diff kind of ugly, so I did the same with Ted's branch and posted a slightly better diff here: http://paste.ubuntu.com/725607/

It is worth noting that I took a look at Ted's branch after seeing the merge proposal, and it appears to work correctly with just the updated configure.ac, data/Makefile.am and src/tomboykeybinder.c (to silence a warning) from my branch, so that may be another way to go since I made quite a few possibly unwelcomed changes in my branch (such as using the logging functions from lightdm, since I found that the ones in indicator-applet tend to lose debug messages occasionally).

Revision history for this message

Dmitry Shachnev (mitya57) wrote on 2011-11-03:

@Jason: I've updated indicator-session package in my ppa (it's now based on 0.7.3.1), can you please copy it to yours?

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-11-25:

Missing support for secondary-activate event (i.e. middle-click) and I'd suggest to use something like the indicator-priority that we used in unity-panel-service to show some app-indicators (like bluetooth, keyboard and network ones) in the proper place.

For the rest, it seems good.

review: Needs Fixing

lp://staging/~jconti/indicator-applet/gnome3 updated on 2011-11-26

405. By Jason Conti on 2011-11-25: Add indicator secondary-activate signal
406. By Jason Conti on 2011-11-25: * Prepare indicator_order array for name-hints.
* Clean up menuitem data:
- Remove old "indicator" data
- Add new definitions for box and secondary-activate signal data
407. By Jason Conti on 2011-11-26: * Refactor place_in_menu into a separate function using place_in_menu_cb.
* If an entry has name-hint, it is used to position the entry.
408. By Jason Conti on 2011-11-26: Bump version to 0.4.14

Revision history for this message

Jason Conti (jconti) wrote on 2011-11-26:

Added secondary-activate signal and name-hint for positioning support. I'll update the ppa tomorrow for testing.

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-11-26:

Code looks good. Thanks.

I've also planned to make the indicator order configurable via gsettings, so stay tuned on this side.

review: Approve

Revision history for this message

Ted Gould (ted) wrote on 2011-11-28:

Great, thanks for the work on this. A few comments:

* Please don't change the version number in configure.ac, it really confuses things if there are multiple version numbers in "the wild" so it's better just to have one place define it. I'll fix this on merge, no worries there.
* Love that you changed the strings to #defines. I think that's a wonderful thing :-)

Thanks again for your work.

review: Approve

Revision history for this message

Sebastian Geiger (lanoxx) wrote on 2012-02-03:

This was approved by Marco and Ted back in November, but the merge status is still set to 'Needs Review' could some body please update the status and tell if this is going to be merge upstream?

I also reviewed the code and can approve it. And also the ppa works on my oneric installation right now.

review: Approve (code review and test)

Unmerged revisions

408. By Jason Conti on 2011-11-26: Bump version to 0.4.14
407. By Jason Conti on 2011-11-26: * Refactor place_in_menu into a separate function using place_in_menu_cb.
* If an entry has name-hint, it is used to position the entry.
406. By Jason Conti on 2011-11-25: * Prepare indicator_order array for name-hints.
* Clean up menuitem data:
- Remove old "indicator" data
- Add new definitions for box and secondary-activate signal data
405. By Jason Conti on 2011-11-25: Add indicator secondary-activate signal
404. By Jason Conti on 2011-10-25: Bump version to 0.4.13
403. By Jason Conti on 2011-09-18: Missed a dollar sign in configure.ac.
402. By Jason Conti on 2011-08-11: Drop the default css code, it seems to be causing theming issues in the about
dialog when using the Ambiance theme.
401. By Jason Conti on 2011-08-02: Forgot to update the location of the applets.
400. By Jason Conti on 2011-08-02: Updated indicator_order to match the order in unity-panel-service.
399. By Jason Conti on 2011-08-02: Dropped menubar_on_expose and cw_panel_background_changed, I'm not sure they
are necessary anymore (the indicator applet seems to respond correctly to
changes in the panel background).