Merge lp://staging/~gary-lasker/software-center/unity-launcher-integration-fixes into lp://staging/software-center/5.2

Thanks for your work on this branch.

The code looks good, I like the fact that we have a fake PURCHASE_TRANSACTION_ID now
in enums for example.

One minor nitpick would be that I liked that we had a seperate function in 5.0 _register_unity_launcher_transaction_started() for on_transaction_started(). But
given how small it is now its probably not needed.

During manual testing I noticed a delay for up to 20s between the finish of the install and the fly-in animation. This needs to be investigated before this can be merged.
I reproduced this delay on both my quantal box and on precise (to ensure that its not a quantal artifact).

Ideally, the Launcher item should appear as soon as the download begins. By the time installation finishes, you might have queued up several other things to install or remove, or even closed USC altogether.

In the short term, though, I guess for an item to show up in the Launcher five minutes late is still better than not showing up at all.

I tried r3054 of this branch with Qreator and with IntelliJ IDEA Community Edition.

It did not work at all with Qreator.

With IntelliJ, the Launcher item appeared about ten seconds after installation finished, but its tooltip said "Waiting to install".

Hey mpt and mvo! Thanks for the reviews.

Regarding the "waiting to install", that's a one line fix in Unity that Bilal already has a branch in review for (I mentioned it above, see the last paragraph of my MP description).

The dbus signal to Unity is fired by Software Center immediately at the end of install. Any delay from that moment is due to issues in the Unity side. To be specific, I have found that the animation on my machine doesn't happen until I actually "show" my hidden launcher. It's important to note that this is not a regression with this branch, I verified the exact same behavior with current software-center 5.2.2.2 in precise. So it appears to be a regression in Unity, which I will report. Is this the "delay" that you are seeing?

As I said, Software Center fires the dbus signal immediately at the end of the install. If Unity does its work, the animation will happen at that exact moment.

Michael, I didn't use the old code as a model but since I remember very well how it worked I just implemented it directly based on what we have now. While implementing I was thinking about making the registering an explicit method and I agree it's nicer, so I'll make that change. Thank you for the suggestion and for noticing it in the old code!

mpt, I don't see anything like "5 minutes later", unless the install takes 5 minutes (which I am assuming to be the case). But, let's be fair, quick installs add the launcher quickly, with the caveat that I mentioned above about the Launcher unhiding. But that is not related to this fix (as I mentioned above) as it also happens in the released USC. I'll test the packages that you tried to see why Qreater didn't get added.

For the record, I tested this carefully for many cases:

  1. normal installs from the Ubuntu archive
  2. multiple simultaneous installs from the Ubuntu archive
  3. a new purchase
  4. reinstalls of previous purchases

So, I'd like to be clear about what "needs fixing". Based on what I see above, this looks like:

  1. Make the _register_unity_launcher_transaction_started() improvement.
  2. Unity lands and SRUs the "Waiting to install" fix. However, that is outside the scope of this MP and I personally feel this should *not* delay our releasing this current fix as is as it takes care of so many high-target bugs for which we are understandably under a lot of pressure to fix. The Waiting to install message is annoying and ugly, but it's far less annoying and ugly than the bugs being fixed. And this message is cleared after a restart, so it's not permanent.
  3. Verify and figure out why Qreator is not being added. I did not hit this issue with any of the applications that I tested with so this may just be an issue with Qreator
  4. The launcher "unhide" needed before animation. Again, this is not in scope for this SRU as it is also the case even in released USC in Precise right now.

Sound good?

On Wed, Jun 13, 2012 at 01:39:28PM -0000, Gary Lasker wrote:
> Hey mpt and mvo! Thanks for the reviews.
Hi Gary,

thanks for your feedback.

> Regarding the "waiting to install", that's a one line fix in Unity
> that Bilal already has a branch in review for (I mentioned it above,
> see the last paragraph of my MP description).

Yeah, we need to case the unity team to see if/why that merge is
blocked and if there is anything we can do to help getting this
unblocked.

> The dbus signal to Unity is fired by Software Center immediately at
> the end of install. Any delay from that moment is due to issues in
> the Unity side. To be specific, I have found that the animation on
> my machine doesn't happen until I actually "show" my hidden
> launcher. It's important to note that this is not a regression with
> this branch, I verified the exact same behavior with current
> software-center 5.2.2.2 in precise. So it appears to be a regression
> in Unity, which I will report. Is this the "delay" that you are
> seeing?

Yes, that is the delay I'm seeing. Its rather odd and needs
investigation and a bugreport for unity. With that delay the UI looks
rather odd, I had a 20s delay between USC telling me that the app is
installed before it was going to the launcher. Before this was also
there but it was mitigated because the install was usually still in
progress.

> Michael, I didn't use the old code as a model but since I remember
> very well how it worked I just implemented it directly based on what
> we have now. While implementing I was thinking about making the
> registering an explicit method and I agree it's nicer, so I'll make
> that change. Thank you for the suggestion and for noticing it in the
> old code!

Like I said in the review, its really minor and not a blocker.

> So, I'd like to be clear about what "needs fixing". Based on what I see above, this looks like:
>
> 1. Make the _register_unity_launcher_transaction_started()
> improvement.

Low priority, not a blocker (but a low hanging fruit).

> 2. Unity lands and SRUs the "Waiting to install" fix. However,
> that is outside the scope of this MP and I personally feel this
> should *not* delay our releasing this current fix as is as it takes
> care of so many high-target bugs for which we are understandably
> under a lot of pressure to fix. The Waiting to install message is
> annoying and ugly, but it's far less annoying and ugly than the bugs
> being fixed. And this message is cleared after a restart, so it's
> not permanent.

It makes the experience look unpolished, so we need to get in touch
with the unity guys and get information about if there is anything
that is blocking that merge and what we can do to help (and also what
SRU this can go into).

> 3. Verify and figure out why Qreator is not being added. I did not
> hit this issue with any of the applications that I tested with so
> this may just be an issue with Qreator

Yes, that needs investigation, I think its just the fact that it comes
from extras and that our guessing code is not smart enough in this
case. A good fix would be to look at /var/lib/dpkg/info/$pkgname.list
for any desktop file in there if $pkgname.desktop ...

Interestingly, Qreator's desktop file is named "extras-qreator.desktop", and so yes, our guessing code is not good enough, but it seems that all (two) of the current packages in "Independent" have their desktop file name prepended with "extras-" in the form:

  extras-<pkgname>.desktop

So it seems a convention (sample size of 2) and I'll just add this to the guessing code as this ony applies to purchased and ARB apps only and this has been reliable for purchased apps for cycles now, so adding the extras check should take care of those cases similarly. I've added this now and it seems to work fine, and will write a new bug for this case.

Indeed, /var/lib/dpkg/info/qreator.list has the correct name for the desktop file, so if we decide we do need to parse these files we can do that in a separate branch, as you suggested.

I added bug 1012877 for the case of apps in the extras.ubuntu.com section, this is how also fixed in this branch.

I took at look at Bilal's Unity side fix at https://code.launchpad.net/~bilalakhtar/unity/waiting-to-install-tooltip-925014/+merge/108858, and it looks to me to be approved now and ready for merging.

I noticed that his code is expecting an empty value for the transaction ID in the launcher dbus call in order to avoid displaying the "Waiting to install" tooltip, so I made that supporting change in this branch. Note that since we are firing the dbus call at the very end of the transaction now, this value will have no use at the launcher side anyway.

I've included a TODO in the code to document this for when we need to change this back.

I've submitted a new bug for the Unity team describing the delayed "fly to launcher" behavior when the Unity launcher is set to auto-hide. It's bug 1012896.

I've fixed the delay bug in this branch:

https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes/+merge/110214

That branch also contains the fix to the Waiting to install tooltip and the "launcher stays stuck in revealed mode" bug.

On Wed, Jun 13, 2012 at 09:20:26PM -0000, Gary Lasker wrote:
> Interestingly, Qreator's desktop file is named "extras-qreator.desktop", and so yes, our guessing code is not good enough, but it seems that all (two) of the current packages in "Independent" have their desktop file name prepended with "extras-" in the form:
>
> extras-<pkgname>.desktop
>
> So it seems a convention (sample size of 2) and I'll just add this to the guessing code as this ony applies to purchased and ARB apps only and this has been reliable for purchased apps for cycles now, so adding the extras check should take care of those cases similarly. I've added this now and it seems to work fine, and will write a new bug for this case.

Yeah, that is the convention for extras.ubuntu.com, thanks for fixing it.

> Indeed, /var/lib/dpkg/info/qreator.list has the correct name for the desktop file, so if we decide we do need to parse these files we can do that in a separate branch, as you suggested.

Could you please file a bug about this so that we can keep track of
it? This would fix it for all cases when we can't guess the desktop
file from the pkgname (unless there are multiple desktop files in the
package in which case we will have to guess or add them all ;).

Cheers,
 Michael

Bilal, I just want to say thank you very much for your super-fast response in making the needed fixes on the Unity side. :) As always, I very much appreciate your dedication and your good work!!

My best to you,
Gary

I have a note in to didrocks to try to help get Bilal's branch reviewed and to get an idea about the timeline for when we can expect the fix to appear in a Unity Precise SRU.

Thanks Gary, that is great to hear.

I will unset the "Needs fixing" as the branch is fine and ready. Given that we need to wait for the current SRU I think the approach to get the timeline for a unity SRU and sync with that (if its not in the too distant future) is the best approach.

Just for completness, here are the branch from Bilal:
https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes

I removed bug 946789 and bug 993224 from the changelog set and from the description as, although these are fixed as a side effect of the fix in the branch, the proper and final fix for these will be to have the Unity launcher service monitor all of the installation events from aptdaemon and to react to them accordingly. Since this work on the Unity side is being tracked in bug 1011681, I have marked the above two bugs as duplicates of that one.

While we are waiting for the unity side, it would be great if you could address the points in:
https://code.launchpad.net/~gary-lasker/software-center/tech-items-to-launcher-fix-lp1006483/+merge/111135/comments/238731 to update the test to the new even sending behavior.

Hi Michael, please check the separate MP for the enhanced set of unit tests here:

  https://code.launchpad.net/~gary-lasker/software-center/launcher-integration-unit-tests/+merge/115236

Thanks!

On Mon, Jul 16, 2012 at 10:03:18PM -0000, Gary Lasker wrote:
> Hi Michael, please check the separate MP for the enhanced set of unit tests here:
>
> https://code.launchpad.net/~gary-lasker/software-center/launcher-integration-unit-tests/+merge/115236

Thanks for your work on this. I commented in the above
merge-proposal. Its good that the tests are covering the start/end now
and that more of the desktop file cases are checked now. Unfortunately
there seems to be a issue with the patch not quite patching the right
spot (see the MP) so that _fake_send_application_to_launcher_and_check
is not called. That will need addressing before this can land.

Thanks,
 Michael