Support application indicators

Bug #497853 reported by Jorge Castro
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Brasero
Expired
Wishlist
brasero (Ubuntu)
Fix Released
Wishlist
Martin Pitt
Lucid
Fix Released
Wishlist
Martin Pitt

Bug Description

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

Jorge Castro (jorge)
affects: ubuntu → brasero (Ubuntu)
Changed in brasero (Ubuntu):
assignee: nobody → Canonical Desktop Experience Team (canonical-dx-team)
Changed in brasero (Ubuntu Lucid):
importance: Undecided → Wishlist
status: New → Triaged
Jorge Castro (jorge)
Changed in brasero (Ubuntu Lucid):
assignee: Canonical Desktop Experience Team (canonical-dx-team) → Travis B. Hartwell (nafai)
Changed in brasero (Ubuntu Lucid):
status: Triaged → In Progress
Revision history for this message
Travis B. Hartwell (nafai) wrote :

Patch was made against upstream git tag BRASERO_2_29_6, matching the version in Lucid, and I have verified patch both cleanly applies against the original tarball acquired via apt-get source -d brasero (using patch -p0) and against git HEAD (via git rebase).

Feature complete with the following caveats. particularly to make it upstream acceptable:

- Probably need to add a flag to configure like
  --enable/disable-app-indicators. My autoconf foo is weak, so I left
  that undone.

- When compiling with app-indicator support,
  libbrasero-burn/brasero-tray is still compiled and linked, which is
  unnecessary. I tried added #ifdefs in Makefile.am around the lines
  which included brasero-tray.c and brasero-tray.h, but that didn't
  work.

- I have code duplication between the existing
  libbrasero-burn/brasero-tray.c and the new
  libbrasero-burn/brasero-app-indicator.c, due to a cut and paste
  job to get started. The areas I'm concerned about:

  - in libbrasero-burn/brasero-app-indicator.c, the function
    brasero_app_indicator_set_progress_menu_text (starting line 227)
    is largely a copy of libbrasero-burn/brasero-tray.c, the function
    brasero_tray_icon_set_tooltip (starting line 244), except for the
    portion where the menu item label is set vs. the original tooltip
    being set.
  - similarly, libbrasero-burn/brasero-app-indicator.c, function
    brasero_app_indicator_set_progress (starting line 281) is largely
    a copy of libbrasero-burn/brasero-tray.c, function
    brasero_tray_icon_set_progress (starting line 293), except for the
    calls to set the tooltip and set the icon.

  I wasn't sure how to address these the best. It didn't make sense
  to inherit BraseroAppIndicator from BraseroTrayIcon because
  BraseroTrayIcon itself is a sub-class of GtkStatusIcon. Should I
  just move the common calculation / string creation methods out to a
  separate .h/.c pair and call those from each? That leaves the
  question of where to free the string.

Any other suggestions would be great so I can improve my patches in
the future.

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

Will attach patch to upstream bug after Ken has had a chance to review it.

Changed in brasero (Ubuntu Lucid):
assignee: Travis B. Hartwell (nafai) → Ken VanDine (ken-vandine)
status: In Progress → Fix Committed
tags: added: patch
Revision history for this message
Ken VanDine (ken-vandine) wrote :

I made some changes to what you had in configure.in
 * Follow naming conventions already used by upstream
   - APPINDICATOR_REQUIRED instead of APPINDICATOR_REQS
   - enable_appindicator instead of have_app_indicator
 * Make it an enable arg like you suggested, default to yes

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

Not sure how to best remove the code duplication, so pending any other suggestions from Ken, likely ready for upstream review.

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

Change to proper state, per seb128.

Changed in brasero (Ubuntu Lucid):
assignee: Ken VanDine (ken-vandine) → Travis B. Hartwell (nafai)
status: Fix Committed → In Progress
Revision history for this message
Travis B. Hartwell (nafai) wrote :

This patch fixes a bug found by Ken by changing the "cancel" menu item to "close" to emulate the behavior of the old GtkStatusIcon. It also removes the icons from the menu items to make the menu formatting look correct.

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

Ready for re-review. Note that this change is as invasive as the change to vino, bug #497883, and perhaps should first be vetted upstream as Martin suggested for vino.

Changed in brasero (Ubuntu Lucid):
assignee: Travis B. Hartwell (nafai) → Canonical Desktop Team (canonical-desktop-team)
Martin Pitt (pitti)
Changed in brasero (Ubuntu Lucid):
assignee: Canonical Desktop Team (canonical-desktop-team) → Martin Pitt (pitti)
Revision history for this message
Martin Pitt (pitti) wrote :

I reviewed the translated strings in the patch. Many are already present, but "Close" is new. Is it possible to use e. g. a stock button for that, or drop it entirely? The help string is also just "Close", it's not really obvious what it does -- close the menu, close the application, cancel the burn?

Revision history for this message
Martin Pitt (pitti) wrote :

Please forward this patch to the upstream bug as well.

Revision history for this message
Martin Pitt (pitti) wrote :

Also, I don't even see the "Close" in the menu. Instead, I only see an untranslated "Cancel" (but that string is upstream as well, German translation is just missing).

Otherwise this is working fine. I'll wait with the upload until the string question is settled and the patch gets forwarded upstream.

Thanks!

Martin Pitt (pitti)
Changed in brasero (Ubuntu Lucid):
assignee: Martin Pitt (pitti) → Travis B. Hartwell (nafai)
status: In Progress → Incomplete
Revision history for this message
Travis B. Hartwell (nafai) wrote :

To address Martin's concerns, I discussed with mpt what the
appropriate user interaction here should be. I did the following:

- I reverted the cancel menu item back to using GTK_STOCK_CANCEL so it
  will be translated and then did some additional work to remove the
  icon from being displayed.

- Since the application indicator does not give any more information
  than the dialog does, I removed the change of the menu item from
  cancel -> close. Instead, when the burn is finished, the
  application indicator is hidden and the dialog is shown if hidden.
  When the dialog is destroyed, the application indicator is also
  destroyed.

Hopefully this has both made the interaction simpler and also removed
the string translation issue.

Thanks for the feedback Martin.

Changed in brasero (Ubuntu Lucid):
assignee: Travis B. Hartwell (nafai) → Martin Pitt (pitti)
status: Incomplete → In Progress
Revision history for this message
Martin Pitt (pitti) wrote :

Thanks Travis! Looks great now, I'll upload this now. Please still forward it upstream.

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

This bug was fixed in the package brasero - 2.29.91-0ubuntu2

---------------
brasero (2.29.91-0ubuntu2) lucid; urgency=low

  * Add 012_appindicator.patch: Support application indicator, patch by Travis
    B. Hartwell. (LP: #497853)
  * debian/control.in: Add libappindicator build dependency.
  * Regenerate 99_autoconf.patch to pick up above patch.
 -- Martin Pitt <email address hidden> Thu, 04 Mar 2010 11:30:57 +0100

Changed in brasero (Ubuntu Lucid):
status: In Progress → Fix Released
Changed in brasero:
importance: Unknown → Wishlist
status: Unknown → New
Changed in brasero:
status: New → Expired
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.