Merge lp:~mvo/unity/sc-launcher-integration-fixes into lp:unity
| Status: | Merged |
|---|---|
| Approved by: | Michael Terry on 2012-12-12 |
| Approved revision: | 2927 |
| Merged at revision: | 2974 |
| Proposed branch: | lp:~mvo/unity/sc-launcher-integration-fixes |
| Merge into: | lp:unity |
| Diff against target: |
491 lines (+234/-27) 11 files modified
launcher/ApplicationLauncherIcon.cpp (+8/-3) launcher/ApplicationLauncherIcon.h (+7/-4) launcher/LauncherIcon.cpp (+5/-3) launcher/SoftwareCenterLauncherIcon.cpp (+82/-1) launcher/SoftwareCenterLauncherIcon.h (+2/-2) po/unity.pot (+1/-1) tests/CMakeLists.txt (+5/-1) tests/data/applications/kde4/afile.desktop (+37/-0) tests/test_main.cpp (+8/-0) tests/test_software_center_launcher_icon.cpp (+78/-12) unity-shared/DebugDBusInterface.cpp (+1/-0) |
| To merge this branch: | bzr merge lp:~mvo/unity/sc-launcher-integration-fixes |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Marco Trevisan (Treviño) | 2012-11-19 | Approve on 2012-12-11 | |
| Michael Vogt | Resubmit on 2012-12-05 | ||
| PS Jenkins bot | continuous-integration | Pending | |
|
Review via email:
|
|||
Commit Message
Software center launcher integration fixes. This will setup the correct desktop file when software-center applications "fly" into the launcher.
Description of the Change
This branch adds support for "native" software center unity launcher integration.
This means that when a application is installed via software-center (and soon via the dash) the icon of
that application will "fly" into the launcher and the launcher will track progress. Once the application
is installed the "temp" desktop file is replaced with the "real" desktop file of the installed app.
The reason this swapping needs to be done is to ensure that unity tracks the real application and not
the copy from app-install-
I would appreciated a initial review and feedback on this as I had to change bit in "ApplicationLau
from private to protected to make it possible to update the data from SoftwareCenterL
| Michael Vogt (mvo) wrote : | # |
- 2912. By Michael Vogt on 2012-11-20
-
update remote uri as well to ensure that the data that is saved to the panel is correct
- 2913. By Michael Vogt on 2012-11-20
-
do not use sc_pkgname from the aptdaemon metadata and instead reply on the temp desktop filename that the software-center uses to substitute for the real one
| Michael Vogt (mvo) wrote : | # |
Trevinho was kind enough to give me some feedback on irc that I want to address here :)
<Trevinho> mvo: another thing.. instead of adding FRIEND_
I looked at this now and gtest_prod.h is very lean and basicly is just the lines:
#define FRIEND_
So it should be fine to include it even in the production code. Plus there is a build-dependency on libgtest-dev already so that should be ok too (as long as I'm not overlooking anything else).
- 2914. By Michael Vogt on 2012-11-21
-
add unittest for SoftwareCenterL
auncherIcon. OnFinished( ) - 2915. By Michael Vogt on 2012-11-21
-
revert some unneeded changes to make diff clearer
| Michael Vogt (mvo) wrote : | # |
I added another test now and look forward to feedback. From my POV its merge-able now, but of course I'm happy to fix any issues.
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> I looked at this now and gtest_prod.h is very lean and basicly is just the
> lines:
> #define FRIEND_
> test_case_
> So it should be fine to include it even in the production code. Plus there is
> a build-dependency on libgtest-dev already so that should be ok too (as long
> as I'm not overlooking anything else).
Yeah, but considering how is defined... I really prefer if you get rid of these macros and you just add at the end of the class a "friend class TestSoftwareCen
The main thing I'd like to be changed, is the apt daemon to send a desktop-id instead, so that you can simply use DesktopUtilitie
- 2916. By Michael Vogt on 2012-11-22
-
remove FRIEND_TEST() usage and replace with MockSoftwareCen
terLauncherIcon () in the test
| Michael Vogt (mvo) wrote : | # |
Hi Marco,
thanks for your review!
On Thu, Nov 22, 2012 at 02:14:30PM -0000, Marco Trevisan (Treviño) wrote:
> Review: Needs Fixing
>
> > I looked at this now and gtest_prod.h is very lean and basicly is just the
> > lines:
> > #define FRIEND_
> > test_case_
> > So it should be fine to include it even in the production code. Plus there is
> > a build-dependency on libgtest-dev already so that should be ok too (as long
> > as I'm not overlooking anything else).
>
> Yeah, but considering how is defined... I really prefer if you get rid of these macros and you just add at the end of the class a "friend class TestSoftwareCen
Thanks for this explaination. I fixed this now in r2916 to use a Mock
instead of the FRIEND_TEST().
> The main thing I'd like to be changed, is the apt daemon to send a desktop-id instead, so that you can simply use DesktopUtilitie
I'm not sure I understand this (but I'm pretty new to unity, so that
is ok I hope :). The daemon will send a temp desktop file at first
because the real desktop file is not installed yet. That might be
/tmp/software-
desktop-id would be sending
application:
AIUI? Given that its a temp name and not in the xdg path it would
still send the full path? Or are you suggesting that it should create
the temp desktop file in a xdg .local dir?
Once the app is installed, the GetActualDeskto
take the rfind(":") part ("meep.desktop") and make that
/usr/share/
application:
/usr/share/
away on remove of the application.
Could you please help me understanding how I should use
GetDesktopPathB
Thanks!,
Michael
| Michael Vogt (mvo) wrote : | # |
From irc (just so that its recorded here):
<Trevinho> mvo: gave a look.. Basically I mean that once you've installed the app, then you can make apt to send to unity the desktop id of the new file... So this means "/tmp/software-
<mvo> Trevinho: ok, so that would mean that there needs to be a new dbus call for unity, right? to replace a existing launcher icon (the temp one that is just valid during the install) with the new one?
<Trevinho> mvo: imho that would be better, so that you can keep all the low level stuff there
<Trevinho> whie unity will be just informed with the actual result
<Trevinho> mvo: or just pass that during the "On-Finished" signal...
<mvo> Trevinho: ok, I will think about it
<Trevinho> mvo: but if you don't want to do that, it's fine to use the current way, but use the utility function for doing the real parsing
<mvo> Trevinho: ok, thanks. I would prefer to leave it as is for now, but I will use the utility function
I started working on this now but so far did not succeed to update the tests as for some reason XDG_DATA_HOME pointing to the local builddir is not quite working, I will keep poking at it.
- 2917. By Michael Vogt on 2012-11-23
-
merged from trunk and resolved conflict
| Michael Vogt (mvo) wrote : | # |
I figured out why setting XDG_DATA_HOME is not working reliable in my tests. Gio/Glib will setup its internal search path just once (see gio/gdesktopapp
But DesktopUtilitie
| Michael Vogt (mvo) wrote : | # |
I see two ways around this:
- init the XDG_DATA_DIRS env very early globally (and hope that it does not break other stuff)
- reimplement g_desktop_
(simply drop the tests, but that sounds like not exactly a good idea ;)
Feedback welcome.
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> I see two ways around this:
> - init the XDG_DATA_DIRS env very early globally (and hope that it does not
> break other stuff)
Yeah, probably this is the best solution... I think we already moved the test .desktop files to a proper position, so it should be fine to just init this value in test_main*
- 2918. By Michael Vogt on 2012-11-26
-
use DesktopUtilitie
s::GetDesktopPa thById( ) and update tests accordingly
| Michael Vogt (mvo) wrote : | # |
Thanks again Marco!
I updated the code now to use DesktopUtilitie
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
35 + if (_desktop_
136 + {
Wouldn't better to check if it contains "/share/
Overall looks good now, but I'm not sure this is still working with the new AppManager in trunk.
- 2919. By Michael Vogt on 2012-12-04
-
launcher/
SoftwareCenterL auncherIcon. cpp: do not look at /usr prefix, thanks to Marco Trevisan
| Michael Vogt (mvo) wrote : | # |
Thanks Marco!
I removed the fix for the prefix now and do the check as you suggested.
Now I merged in trunk and resolved the conflicts and indeed, from the look of the code I'm pretty
sure this won't work anymore as the way the application is swapped from the temp desktop file to the
new final desktop file destination has changed completely.
Given that I wonder if maybe the approach reconsidered. I like your original idea of having two dbus calls,
one for setting the "in-progress" app with progress and all and once that is finished, swapping that for the
real one. I will look into that and see how feasible this is, it seems like its a cleaner approach anyway.
Cheers,
Michael
- 2920. By Michael Vogt on 2012-12-05
-
merged from trunk and updated to the new structure
- 2921. By Michael Vogt on 2012-12-05
-
make it work again by allowing LauncherIcon:
:Stick( ) to be called with save even for already sticky icons' and disable tests until they are ported - 2922. By Michael Vogt on 2012-12-05
-
first test ported!
- 2923. By Michael Vogt on 2012-12-05
-
only Animate needs porting
- 2924. By Michael Vogt on 2012-12-05
-
merged trunk and resolved conflicts
| Michael Vogt (mvo) wrote : | # |
This is now ready again with the new ApplicationManager framework. The tests are ported too, only the "TEST_F(
approach of replacing it with XndCollectionWindow does not work. I will keep exploring, but would appreciate
a look at the rest of the MP again.
| Andrea Azzarone (azzar1) wrote : | # |
> This is now ready again with the new ApplicationManager framework. The tests
> are ported too, only the "TEST_F(
> case is missing as DNDCollectionWindow is gone and my naive
> approach of replacing it with XndCollectionWindow does not work. I will keep
> exploring, but would appreciate
> a look at the rest of the MP again.
nux::
nux::
just do:
nux::
- 2925. By Michael Vogt on 2012-12-06
-
enable Animate test again, thanks Andrea!
| Michael Vogt (mvo) wrote : | # |
Thanks Andrea! I updated the code and re-enabled the last test.
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
Ok, now it's fine (sorry for the delays)... :)
Before of general approving (do it yourself when you're ready), remember to set a commit message and it would be also ince if you get rid of the tabs at:
272 + test_static_
Thanks.
- 2926. By Michael Vogt on 2012-12-11
-
tests/CMakeList
s.txt: fix tab, thanks to Trevino)
| Michael Vogt (mvo) wrote : | # |
Thanks Marco! Excellent news. I fixed the tab in line 272 and set a commit message. I can't approve it myself, but it should be good now. Once this is merged I will upload the corresponding software-center branch.
- 2927. By Michael Vogt on 2012-12-11
-
merged trunk


Setting to Needs-review now to get feedback and get it merged if its good. There will be another center- agent.
branch on top of this one that adds support for installs from software-