Ubuntu One Client

Merge lp://staging/~mandel/ubuntuone-client/notification-center-support into lp://staging/ubuntuone-client

  1. notification-center-support
  2. Merge into trunk
Proposed by Manuel de la Peña
Status: Rejected
Rejected by: dobey
Proposed branch: lp://staging/~mandel/ubuntuone-client/notification-center-support
Merge into: lp://staging/ubuntuone-client
Diff against target: 351 lines (+309/-0)
6 files modified
tests/platform/notification/test_darwin.py (+150/-0)
ubuntuone/platform/filesystem_notifications/monitor/darwin/fsevents_client.py (+1/-0)
ubuntuone/platform/notification/__init__.py (+3/-0)
ubuntuone/platform/notification/darwin/__init__.py (+53/-0)
ubuntuone/platform/notification/darwin/growl.py (+53/-0)
ubuntuone/platform/notification/darwin/notification_center.py (+49/-0)
To merge this branch: bzr merge lp://staging/~mandel/ubuntuone-client/notification-center-support
Reviewer Review Type Date Requested Status
Mike McCracken (community) Needs Fixing
dobey (community) Needs Fixing
Review via email: mp+122237@code.staging.launchpad.net

Commit message

- Added support for the notification center and growl (LP: #1044315).

Description of the change

- Added support for the notification center and growl (LP: #1044315).

The following dependencies have been added which are probably no in your system:

http://pypi.python.org/pypi/pync/1.1 => notification center support
https://github.com/kfdm/gntp/ => growl support

You can test it IRL by starting sd and adding a big file to the Ubuntu One folder, you should see a notificaiton.

To post a comment you must log in.
lp://staging/~mandel/ubuntuone-client/notification-center-support updated
1308. By Manuel de la Peña

Compare ints no strings.

1309. By Manuel de la Peña

Do not use growl all the time.

Revision history for this message
dobey (dobey) wrote :

+APPLICATION_NAME = 'Ubuntu One Client'

We shouldn't duplicate this string in multiple places. Also, I think this should probably just be 'Ubuntu One'.

Also, what do these 2 notifications actually look like? Can you provide screen shots of them?

review: Needs Fixing
lp://staging/~mandel/ubuntuone-client/notification-center-support updated
1310. By Manuel de la Peña

Do not duplicate strings.

Revision history for this message
Manuel de la Peña (mandel) wrote :

Images of how it looks:

Growl support: http://imgur.com/QnMu6,jZwvj
Notification center support: http://imgur.com/QnMu6,jZwvj#1

The proper icon is missing because we have to discuss its locations in the fs once the app has been packaged.

Revision history for this message
dobey (dobey) wrote :

This does need to pass the icon argument on to the upstream API calls though, where applicable. And perhaps a comment stating it is ignored for pync because the API doesn't accept it as an argument.

Revision history for this message
Mike McCracken (mikemc) wrote :

A couple of issues:

- gntp won't work if growl isn't installed. It only talks to an existing growl, so we'll get tracebacks on 10.6 and 10.7 for lots of people…

- pync depends on terminal-notifier, which is a separate .app that calls NSNotification Center.
pync downloads that if it can't find it in a hard-coded spot, which is kind of awkward. we can put terminal-notifier there, but we'll have to strip out the apple-copyrighted Terminal.icns, which is in the terminal-notifier app for some reason.

We should probably look at using PyObjC, which would let us either directly call NSNotificationCenter and Growl, or (even better) just use the Growl 2.0 ObjC api via PyObjC, which lets us use one API and support growl on 10.6/7, and either growl or NC on 10.8, depending on user choice.

Also, as of 1.3 or so, Growl doesn't need to install anything to show notifications - that was an annoying issue previously.

here's the description from the Growl 2.0 API readme:

 1. Growl.app is installed, and the user has configured growl to NOT use NC
 ▪ Notifications work as before. Notification tickets are sent to the Growl.app which displays them and performs actions and displays notifications as configured by the user
 2. Growl.app is installed, and the user has configured Growl.app to use NC
 ▪ Growl.framework detects this state from within the app and notifications are sent directly to NC from the app's process. Additionally, notification tickets are sent to growl which performs any associated actions. Growl.app displays nothing.
 3. Growl.app is not installed
 ▪ Growl.framework detects this state from within the app and notifications are sent directly to NC from the app's process. Nothing else is done.

review: Needs Fixing

Unmerged revisions

1310. By Manuel de la Peña

Do not duplicate strings.

1309. By Manuel de la Peña

Do not use growl all the time.

1308. By Manuel de la Peña

Compare ints no strings.

1307. By Manuel de la Peña

Added support for growl in older versions. Added tests.

1306. By Manuel de la Peña

Do call notify.

1305. By Manuel de la Peña

Added support for the os x 10.8 notification center.

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