Unity and Sound Menu support should be soft dependencies

Reported by Kurt Smolderen on 2012-06-20
24
This bug affects 3 people
Affects Status Importance Assigned to Milestone
BeatBox
High
Kurt Smolderen

Bug Description

Currently, support for Unity and Sound Menu is done at compile time. If the configure script detects the presence of unity and indicate-0.7, support for both is enabled automatically at runtime. However, if one is using Gnome Classic or Gnome shell or another desktop environment, these technologies might not be used at all.

Therefore, I think it is better to check whether unity and/ or sound menu are used at startup...

Kurt Smolderen (kurt.smolderen) wrote :

In order to verify whether sound menu and unity are being used, I try to determine their PID. I'm not sure whether this is the most optimal way however...

Changed in beat-box:
status: New → Fix Committed
importance: Undecided → Low
assignee: nobody → Kurt Smolderen (kurt.smolderen)
milestone: none → 0.6
Kurt Smolderen (kurt.smolderen) wrote :

In revision 627, the checks at runtime have disappeared... Instead, MPRIS and Unity support are activated when the libraries are found at compile time again...

Scott Ringwelski (sgringwe) wrote :

Yes. Unfortunately, although your checks were nice with Ubuntu's unity/sound menu system, they were not so nice to non-unity systems.

MPRIS2 and libunity are NOT specific to unity/soundmenu. Users are allowed to have neither of those but still have MPRIS2 clients/listeners.

By checking for unity and sound menu at start and not enabling MPRIS2 if neither is found, we are cutting off MPRIS2 support for many users (there were bug reports for gnomeshell specifically, and i'm sure others).

That said, there should be a sound menu check before enabling sound menu in MPRIS.vala.

Kurt Smolderen (kurt.smolderen) wrote :

Agree on that. But is leaving out any runtime check not dangerous as well? What if someone is running for example Xubuntu and has removed libunity. Beatbox will probably be installed via Launchpad PPA and hence will have Unity integration enabled... Now when Beatbox starts, it will crash because it can not load libunity...

So as suggested in "MPRIS2 doesn't work" (LP bug 1016369), I think the following is what needs to be done:
- Create MPRIS anyway (dbus is the only dependency, right?)
- Check for soundmenu when creating the MPRIS instance and only activate soundmenu support if soundmenu-indicator is running
- Check for unity process to be running before enabvling Unity integration support.

This way, soundmenu-indicator and unity become no requirement for running Beatbox... If you agree on this, I will update my branch and test the required behavior in Gnome Shell as well (sorry for not doing this on the first commit).

Kurt Smolderen (kurt.smolderen) wrote :

One additional remark: right now, Beatbox will not crash in Xubuntu as libunity is a dependency for Beatbox... So the way it is implemented now makes sense for distributions not shipping libunity or libindicate...

Scott Ringwelski (sgringwe) wrote :

The problem is that we want to use libunity when we can because it also offers playlist support as well as quicklists and progress reports for docks. This is really horrible mixing of features that needs to be fixed. UnityIntegration.vala and MPRIS.vala do the mostly the same things except for:

unityintegration.vala requires libunity, an api for MPRIS
unityintegration.vala has playlist support
unityintegration.vala has quicklist and progress bar support for unity, docks

I think in the end the optimal solution would be:
Use MPRIS.vala ONLY for MPRIS player stuff, which would mean figuring out playlist support in that file.
Use UnityIntegration.vala ONLY for quicklists and progress notifications (neither of which is specific to unity)

We could also put it all into UnityIntegration and force libunity package. This would be easier, not sure if best though.

Would you be willing to take that up? In the end, I think that's the solution we're going to have to use.

Scott Ringwelski (sgringwe) wrote :

Victor, what are your opinions?

Kurt Smolderen (kurt.smolderen) wrote :

I can have a look at the redesign of this functionality, yes...

Victor Martinez (victored) wrote :

Well, since we already depend on a couple of libraries that are not available outside the Ubuntu/Debian ecosystem, I don't see the problem with using libunity in order to reduce the amount of code required for MPRIS integration. I would rather mark this bug as "Won't Fix", since checking for the presence of client software is not our responsibility and only adds code clutter. Having the MPRIS and UnityIntegration modules doing exactly the same and exporting the same info over D-Bus twice is not acceptable either.

Victor Martinez (victored) wrote :

... So, my vote is for ditching the MPRIS class in favor of UnityIntegration. These are non-core features that I'm sure we want to keep as simple as possible, and libunity gives us exactly that. Besides, as Scott said, many of them are not exclusive to Unity, as many desktop APIs are shared across multiple desktop environments.

Scott Ringwelski (sgringwe) wrote :

Victor raises a good point. We could make libunity a requirement rather than optional, which would remove the necessity for this check. But, is libunity installed/installable in all distros?

I've just searched for libunity in the main repositories of the following distributions:
- OpenSuse: positive
- Arch: Negative
- Fedora: Negative

This offcourse does not mean libunity won't be installable at all, but I think having MPRIS as backup is still a good idea. So either only instantiate the MPRIS client in case libunity can't be loaded or remove the MPRIS functionality from UnityIntegration and always load MPRIS client. This would turn 'libunity' into a preferred dependecy and dbus into a required one.

Any thoughts? I'm still volunteering to take this up, but would like to know which change has the best chance to get accepted for merging :-)

Victor Martinez (victored) wrote :

Kurt, here are my replies:

> This of course does not mean libunity won't be installable at all, but I think having MPRIS as backup is still a good idea.

I agree with you on this. It would be a dumb action to drop MPRIS support in distros like Arch or Fedora just because we decided to go with libunity.

> So either only instantiate the MPRIS client in case libunity can't be loaded or remove the MPRIS functionality from UnityIntegration and always load MPRIS client.

If we already linked the player against libunity, and the it's not found, bad things will happen. There's not much we can do to avoid it at runtime. We could do this safely if UnityIntegration were a plugin (which is easy to do by using libpeas). I believe there will be some work on basic plugin functionality soon, so this is actually possible and should be considered. We could then disable the MPRIS plugin if the UnityIntegration plugin is enabled.
The nice thing with plugins is that distros can package the plugins that make sense and are installable in their context. For instance, the MPRIS plugin wouldn't be built in Ubuntu.

What do you think?

Scott Ringwelski (sgringwe) wrote :

I like the plugin solution myself. We could have 2 plugins - 'mpris' and 'mpris-libunity'. But, we still need to take the launcher entry part of unityintegration out and into its own separate class. AFAIK the launcher entry part works for most docks - not just unity's (at least I know it works for docky).

So, it would be

UnityLauncher.vala - the 'entry' part of UnityIntegration for quicklists, badge, and progress. Make this a plugin?
mpris - MPRIS.vala
mpris-libunity - libunity for MPRIS (UnityIntegration.vala - launcher entry stuff)

Opinions?

Devid - do you see any problems with this approach on the packaging side?

Changed in beat-box:
status: Fix Committed → Triaged

Yes, I do. This bug should be set as "Won't fix" due to Debian packaging limites. Please note that in order to build code you actually have to install (for example) libunity-dev as build dependency. If you have libunity-dev when building the package then unity dependency is automatically added. You can create a Debian package for *each* plugin, but I'm sorry, I'm not going to maintain them, this requires too efforts and a we will have a lot of problems in Debian/Ubuntu. Each time a new Debian binary package is added in a Debian source package, if you upload it to Debian/Ubuntu then it will have to be manually approved, and we cannot do this on each plugin change.

Scott Ringwelski (sgringwe) wrote :

Ok, so that is not an option.

I think the best options is to ditch libunity for media player support. In the end, all libunity is is an extra dependency to carry around that we really don't need and would limit our user base quite a bit.

For MPRIS, we will stick with using the dbus api directly such as in MPRIS.vala. All we have to do is add playlist suppport to MPRIS.vala.

However, it is nice to have quicklists, progress, etc. for unity, so we will keep UnityIntegration.vala but take out all of the player stuff and leave only the launcher entry stuff.

libunity-dev will remain optional.

We will still use #HAVE_X checks at start, but we need to make sure we are not disabling all of MPRIS if we are missing sound menu dependencies - sound menu is just one of many clients that listens to MPRIS.

As usual let me know of any objections.

You are not fixing anything. In this bug user complains about libunity
dependency, so you have to choose between keeping Unity or dropping it
at all. UnityIntegration.vala will require libunity-dev during build
which will add a dependency on Unity.

On Mon, Jul 2, 2012 at 4:15 PM, Scott Ringwelski
<email address hidden> wrote:
> Ok, so that is not an option.
>
> I think the best options is to ditch libunity for media player support.
> In the end, all libunity is is an extra dependency to carry around that
> we really don't need and would limit our user base quite a bit.
>
> For MPRIS, we will stick with using the dbus api directly such as in
> MPRIS.vala. All we have to do is add playlist suppport to MPRIS.vala.
>
> However, it is nice to have quicklists, progress, etc. for unity, so we
> will keep UnityIntegration.vala but take out all of the player stuff and
> leave only the launcher entry stuff.
>
> libunity-dev will remain optional.
>
> We will still use #HAVE_X checks at start, but we need to make sure we
> are not disabling all of MPRIS if we are missing sound menu dependencies
> - sound menu is just one of many clients that listens to MPRIS.
>
> As usual let me know of any objections.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1015564
>
> Title:
> Unity and Sound Menu support should be checked at runtime
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/beat-box/+bug/1015564/+subscriptions

At this point, our discussion is not relevant to the original bug report.

We are going to keep libunity, but only require it for unity launcher api stuff. We are going to keep it as an optional dependency. This will not satisfy the checking for libunity at runtime, but that is ok, there isn't much we can do there.

What my plan above fixes is it allows distros without libunity package to still use MPRIS by not depending on libunity for MPRIS and instead using the dbus api directly.

So, in the end - yes libunity is still an optional dependency. But, not having that dependency only means that there is no launcher entry api (quicklists, progress).

But, MPRIS will not depend on libunity which means all other distros without libunity still get MPRIS.

Ok, this makes sense, I honestly remember MPRIS was implemented on top
on DBus but I was wrong. Sorry for the mistake :)

On Mon, Jul 2, 2012 at 4:47 PM, Scott Ringwelski
<email address hidden> wrote:
> At this point, our discussion is not relevant to the original bug
> report.
>
> We are going to keep libunity, but only require it for unity launcher
> api stuff. We are going to keep it as an optional dependency. This will
> not satisfy the checking for libunity at runtime, but that is ok, there
> isn't much we can do there.
>
> What my plan above fixes is it allows distros without libunity package
> to still use MPRIS by not depending on libunity for MPRIS and instead
> using the dbus api directly.
>
> So, in the end - yes libunity is still an optional dependency. But, not
> having that dependency only means that there is no launcher entry api
> (quicklists, progress).
>
> But, MPRIS will not depend on libunity which means all other distros
> without libunity still get MPRIS.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1015564
>
> Title:
> Unity and Sound Menu support should be checked at runtime
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/beat-box/+bug/1015564/+subscriptions

Np :). It's a bit confusing for sure. Victor, if you agree with the proposed solution, then Kurt can start with the changes.

Victor Martinez (victored) wrote :

I like all the solutions exposed here.

Kurt, do you still have the time to make the changes?

summary: - Unity and Sound Menu support should be checked at runtime
+ Unity and Sound Menu support should be soft dependencies

You can leave this bug assigned to me... I'll take a look at it.

I've been working on full playlist support for MPRIS only installations... However, I think we need to reconsider the proposed solution. The problem is - as far as I can tell at least - MPRIS does not offer any funtionality to signal a playlist has been removed or a new playlist has been added. As a consequence, the playlists available in Beatbox and the playlists available in the MPRIS indicator/ applet/ client are not in sync... So unless anyone has a suggestion on how to avoid this shortcoming in the MPRIS specification, I think pulling playlist support out of unityintegration and put into mpris is no longer an option...

So What I would like to propose:
- MPRIS support for 'non-Ubuntu' systems (i.e. no libunity available): Bassic support for controlling the player's state (play, pause, stop, forward, previous,...), display cover arts etc. The only hard dependency involved is dbus-glib.
- libunity support if available (at compilation time, and hence a soft dependency). Support for playlists, progress indication etc...

We might even consider to load either MPRIS, or UnityIntegration (depending on whether libunity is available) as this would avoid having two mpris clients serving the same player...

Any thoughts/ suggestions?

Scott Ringwelski (sgringwe) wrote :

Kurt, could you join my on freenode irc at #beatbox-dev?

Scott Ringwelski (sgringwe) wrote :

I'd say your solution is acceptable. It's basically what I initially had, anyways. I guess the key for this fix is making sure the dependencies and checks are correct, and that MPRIS works for all systems.

Changed in beat-box:
importance: Low → High
status: Triaged → In Progress
Changed in beat-box:
status: In Progress → Fix Committed
Mantas Mikulėnas (grawity) wrote :

Kurt in comment #23:
> The problem is - as far as I can tell at least - MPRIS does not offer any funtionality to signal a playlist has been removed or a new playlist has been added.

I think players can signal changes to the PlaylistCount property to achieve this.

Scott Ringwelski (sgringwe) wrote :

Mantas, you are correct. That set me in the right direction, thank you.

Milos (milos-popovic) wrote :

Just to inform you that with rev. 685 MPRIS2 forks fine. :)
Arch x64 + Gnome shell.

Scott Ringwelski (sgringwe) wrote :

Thanks, but I actually turned off libunity in 685 so it ALWAYS uses the normal mpris. But, it's good that it's working nonetheless :)

Changed in beat-box:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers