Support Application Indicators

Bug #497883 reported by Jorge Castro
18
This bug affects 3 people
Affects Status Importance Assigned to Milestone
vino
Won't Fix
Wishlist
vino (Ubuntu)
Fix Released
Wishlist
Canonical Desktop Team

Bug Description

This application should be investigated to be ported to use Application
Indicators for Lucid - https://edge.launchpad.net/indicator-application

 affects ubuntu/vino

 assignee canonical-dx-team
 tag indicator-application

Related branches

Jorge Castro (jorge)
Changed in vino (Ubuntu):
assignee: Canonical Desktop Experience Team (canonical-dx-team) → Travis B. Hartwell (nafai)
Changed in vino (Ubuntu):
importance: Undecided → Wishlist
status: New → Triaged
Revision history for this message
Matthew Paul Thomas (mpt) wrote :

Recommendation: Port the notification area menu directly. Change Preferences text to “Show Remote Desktop menu:” “Always”/“Whenever someone is connected”/“Never”.

Revision history for this message
Travis B. Hartwell (nafai) wrote :

Forgot to mark "In Progress" while working on it, whoops.

The porting of the VinoStatusTubeIcon have been untested, as it is not obvious how to test that.

However the port of the VinoStatusIcon has been fully tested.?field.comment=Forgot to mark "In Progress" while working on it, whoops.

The porting of the VinoStatusTubeIcon have been untested, as it is not obvious how to test that.

However the port of the VinoStatusIcon has been fully tested.

Changed in vino (Ubuntu):
status: Triaged → In Progress
tags: added: patch
Changed in vino (Ubuntu):
assignee: Travis B. Hartwell (nafai) → Canonical Desktop Team (canonical-desktop-team)
Revision history for this message
Martin Pitt (pitti) wrote :

Travis,

can you please forward the patch upstream and get some initial feedback there? This is a very intrusive patch, which we don't want to maintain forever in the desktop team (and it's also already past feature freeze and alpha-3, so it becomes harder to back out later on if it causes trouble).

Changed in vino (Ubuntu):
status: In Progress → Triaged
Revision history for this message
Martin Pitt (pitti) wrote :

Summary from #u-desktop discussion, for the record:

It would be great if the string changes could be done as a separate patch, since that has a much higher chance of being adopted upstream soon. String changes are hard to maintain because they break all translations, and .glade changes are hard to maintain because they tend to change completely when some UI detail is changed.

Revision history for this message
Travis B. Hartwell (nafai) wrote :
Revision history for this message
Travis B. Hartwell (nafai) wrote :

All of the strings contained within this patch are copies of existing strings or are not in the UI, so no additional translations are necessary.

Changed in vino (Ubuntu):
assignee: Canonical Desktop Team (canonical-desktop-team) → Ken VanDine (ken-vandine)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

This seems pretty buggy, using the no string changes patch enabling vino and doing anything that displays the icon seem to work fine, however anything that hides the icon just hangs. Like disabling sharing just hangs, no crash... just hangs. Also there is a minor tweak needed to configure.in

-+ Application indicator support: ${have_app_indicator}
++ Application indicator support: ${enable_appindicator}

Changed in vino (Ubuntu):
assignee: Ken VanDine (ken-vandine) → Travis B. Hartwell (nafai)
Jorge Castro (jorge)
Changed in vino (Ubuntu):
assignee: Travis B. Hartwell (nafai) → Karl Lattimer (karl-qdh)
Revision history for this message
Karl Lattimer (karl-qdh) wrote :

@Travis, this patch no longer applies to 2.28.2 which I am told is the version to ship with maverick.
I will proceed with updating the patch now however I was wondering if you could tell me the version it was developed for?

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Applied patch in the wrong order and generated a lot of errors... Moving on there is a problem with the patch in that;

@@ -107,8 +114,8 @@ vino-marshal.c: vino-marshal.list $(GLIB_GENMARSHAL)
 vino_enum_headers = \
  $(top_srcdir)/server/vino-server.h \
  $(top_srcdir)/server/vino-prompt.h \
- $(top_srcdir)/server/vino-status-icon.h \
- $(top_srcdir)/server/vino-status-tube-icon.h \
+ $(top_srcdir)/server/vino-icon-visibility.h \
+ $(top_srcdir)/server/vino-tube-icon-visibility.h \
  $(NULL)

 vino-enums.c: @REBUILD@ $(vino_enum_headers)

excludes the vino-status-icon header file from being scanned by glib-mkenums

@Travis, could you explain your reasoning behind this, and how you failed to be stung with;

vino-status-icon.c: In function ‘vino_status_icon_class_init’:
vino-status-icon.c:547: error: ‘VINO_TYPE_ICON_VISIBILITY’ undeclared (first use in this function)
vino-status-icon.c:547: error: (Each undeclared identifier is reported only once
vino-status-icon.c:547: error: for each function it appears in.)

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

After making the modification noted in comment #7 the patch applied and appears to work with some minor changes.

Buildreqs is missing libappindicator-dev

Confirmed that disabling sharing does in fact fail to hide the icon, although the remote desktop is correctly disabled, seems all that's needed here is an additional call to hide the icon from the indicator panel when sharing is disabled.

Will fix the patch and upload a replacement.

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Further to my previous comment, it appears that vino is unable to re-enable desktop sharing after disabling, this might be related to failing to unhide the icon.

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Retesting the bug reveals it's gone...

Behaviour was;

Enable desktop sharing, icon appears in the app-indicators
Disable desktop sharing, icon remains in app-indicators
Attempt to enable desktop sharing fails, vino-preferences hangs

Behaviour is (in BASIC because we all like to get nostalgic);

10 Enable desktop sharing, icon appears in the app indicators
20 Disable desktop sharing, icon disappears from app-indicators
30 GOTO 10

... To be clear I haven't changed the binaries at all

I will test further tomorrow, after rebooting the VM to test if this is a one time problem

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Adding some debugging messages to vino-prefs.c reveals what is happening is;

 * A SIGTERM signal is received when disabling
 * vino_prefs_restore_background() is called successfully
 * vino_mdns_shutdown() is called and locks

This appears to be unrelated to the app-indicator patch, and would probably also be the case on systems which use the status icon.

Sometimes the bug doesn't manifest itself at all, sometimes it's hard to shake it.

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Confirmed that the bug present is un-related to the app-indicators patch by turning it off at build time and testing.

The bug appears to be in vino_mdns_stop, where avahi_entry_group_free is called, also there appear to be problems with vino_prefs_shutdown (discovered by commenting out the avahi_entry_group_free call, not investigated further).

This bug should be filed upstream if it isn't already.

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

The patch attached fixes the bug as reported.
Upstream vino_mdns_stop bug remains.

Revision history for this message
Karl Lattimer (karl-qdh) wrote :

Upstream appears to already have the bug filed

https://bugzilla.gnome.org/show_bug.cgi?id=599104

Jorge Castro (jorge)
Changed in vino (Ubuntu):
assignee: Karl Lattimer (karl-qdh) → Canonical Desktop Team (canonical-desktop-team)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

initial review looks good, building it in pbuilder now and will do more testing in the morning. Thanks!

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package vino - 2.28.2-1ubuntu2

---------------
vino (2.28.2-1ubuntu2) maverick; urgency=low

  * debian/patches/12_app-indicators-only.patch (LP: #497883)
    - Use libappindicator
  * debian/control.in
    - Added dh-autoreconf build-dep
  * debian/rules
    - enable app-indicator
    - include autoreconf.mk
 -- Ken VanDine <email address hidden> Thu, 05 Aug 2010 22:49:02 -0400

Changed in vino (Ubuntu):
status: Triaged → Fix Released
Changed in vino:
importance: Unknown → Wishlist
status: Unknown → New
Changed in vino:
status: New → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.