Merge lp:~michael.rawson/unity/fix-show-desktop into lp:unity

[Michael Rawson]

Fixes position of "show desktop" icon-LP:964073

See this merge proposal-failed because I'm useless at bzr. https://code.launchpad.net/~michaelrawson76/unity/fix-for-964073/+merge/99182

Corrects the position of the show desktop icon.

[Andrea Azzarone (azzar1)]

LGTM

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

I don't like too much this workaround (considering that this fix doesn't modify the switcher icon position only by chance)... I'd prefer to be more explicit using something like this: http://paste.ubuntu.com/928646/

review: Needs Fixing

[Andrea Azzarone (azzar1)]

If you do what Marco suggests please remove the second desktop icon.

[Michael Rawson (michael.rawson)]

> I don't like too much this workaround (considering that this fix doesn't
> modify the switcher icon position only by chance)... I'd prefer to be more
> explicit using something like this: http://paste.ubuntu.com/928646/

That also looks fine to me, I can do that if you'd prefer. Give us a tick to work out whether anything else needs fixing. :)

Although, as a sidenote, this was just a workaround until somebody did the proposed "mini-app".

Andrea, yup, sure.

[Marco Trevisan (Treviño) (3v1n0)]

Cool, but aren't you removing too much?

I mean, the code in LauncherController.cpp should stay as it was... Fix that, and it's good to go for me ;)

[Michael Rawson (michael.rawson)]

Well, you're the reviewer...I'll fix.

The code in LauncherController doesn't do anything anymore, and nothing happens if you remove it, so I say they're wasted code. But whatever you choose. :)

[Andrea Azzarone (azzar1)]

154 AbstractLauncherIcon::Ptr desktop_launcher_icon_;
155 AbstractLauncherIcon::Ptr desktop_icon_;

We have two icons for the desktok icon. Now you can remove desktop_launcher_icon_ :)

review: Needs Fixing

[Marco Trevisan (Treviño) (3v1n0)]

Cool, one thing you could do instead, to avoid unneeded data duplication is to remove any reference to desktop_launcher_icon_ from LauncherController and to only use desktop_icon_ for everything.

Applying this should be enough: http://paste.ubuntu.com/930287/

Sorry for asking you to change this again, but your effort is appreciated :)

Thanks.

[Michael Rawson (michael.rawson)]

Erm, well you guys disagree on whether to keep desktop_icon_ or desktop_launcher_icon_, so I'll go with desktop_launcher_icon_ .

[Michael Rawson (michael.rawson)]

Actually, you had to make desktop_launcher_icon_ static to make it available from getavataricons, so I used desktop_icon_ instead.

[Andrea Azzarone (azzar1)]

I'm not sure if the option "show desktop icon" in the ccsm works fine. I mean that option should remove the icon from the launcher and not from the alt+tab switcher.

[Marco Trevisan (Treviño) (3v1n0)]

This shouldn't be removed (in fact I didn't in my linked patch), or the CCSM option will touch the Switcher too:
46 - results.push_back(pimpl->desktop_icon_);

[Andrea Azzarone (azzar1)]

The CCSM should not touch the Switcher.

[Marco Trevisan (Treviño) (3v1n0)]

> The CCSM should not touch the Switcher.

Yes, that's why GetAltTabIcons() should always include it in its list; it won't be duplicated.

[Michael Rawson (michael.rawson)]

Damn, you're right. I really should test CCSM as well. Okay, thanks, will fix. :)

[Andrea Azzarone (azzar1)]

Not sure if we have something like SetShowInLauncher.

[Michael Rawson (michael.rawson)]

I don't think we do either. The problem is that if we do have that, the for loop immediately after that adds another ShowDesktop Icon to the switcher. I'm trying adding an if statement checking if it is of TYPE_DESKTOP or not.

[Marco Trevisan (Treviño) (3v1n0)]

> I don't think we do either. The problem is that if we do have that, the for
> loop immediately after that adds another ShowDesktop Icon to the switcher. I'm
> trying adding an if statement checking if it is of TYPE_DESKTOP or not.

Have you tried my code? It doesn't add a duplicated icon. No worry about that.

57 + if(icon->GetName()!="DesktopLauncherIcon") {
58 + results.push_back(icon);
59 + }

I don't think that it's really needed according to my tests, however that's not logically wrong, but please fix indentation and use GetIconType here.

I.e.

if (icon->GetIconType() != AbstractLauncherIcon::IconType::TYPE_DESKTOP)
{
  results.push_back(icon);
}

[Michael Rawson (michael.rawson)]

Odd...it does for me. I get two desktop icons in the switcher.

Okay, sure I can use GetIconType(), I just kinda assumed that was what GetName() was for. @ indentation, obviously a trivial fix that will be done. :)

Yes, I tried your code, it just added the duplicate icon...how do you test? I run make install....

Sorry about the hassle; new to Ubuntu/software generally.

[Marco Trevisan (Treviño) (3v1n0)]

4 + for (auto icon : *(pimpl->model_)) {
55 + if (icon->ShowInSwitcher(current)) {
56 + //otherwise we get two desktop icons in the switcher.
57 + if (icon->GetIconType()!=AbstractLauncherIcon::IconType::TYPE_DESKTOP) {
58 + results.push_back(icon);
59 + }
60 + }
61 + }

I'm sorry, maybe you misunderstood me: the brackets should be o new line (plus the != should be spaced and pay attention not to put trailing spaces). So the above code should have been:

for (auto icon : *(pimpl->model_))
{
  if (icon->ShowInSwitcher(current))
  {
    //otherwise we get two desktop icons in the switcher.
    if (icon->GetIconType() != AbstractLauncherIcon::IconType::TYPE_DESKTOP)
    {
      results.push_back(icon);
    }
  }
}

;)

[Michael Rawson (michael.rawson)]

Yes, I did misunderstand you, a tab got inserted by mistake on the bottom line, I assumed that was what you were referring to. ;) Found the style guide now. :)

Sorry about delays, school is annoying.

[Marco Trevisan (Treviño) (3v1n0)]

No problem, push these fixes and we can finally merge the change :)

[Marco Trevisan (Treviño) (3v1n0)]

Cool, thanks.

review: Approve

[Unity Merger (unity-merger)]

No commit message specified.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/694/console reported an error when processing this lp:~michaelrawson76/unity/fix-show-desktop branch.
Not merging it.

[Michael Rawson (michael.rawson)]

Sorry guys, I have no idea what that message means. And the link is a 404. What do I do?

[Marco Trevisan (Treviño) (3v1n0)]

The branch compiles here, maybe that was a jenkin's issue... Let's try again.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/695/console reported an error when processing this lp:~michaelrawson76/unity/fix-show-desktop branch.
Not merging it.

[Michael Rawson (michael.rawson)]

Build 695 doesn't exist yet...

[Marco Trevisan (Treviño) (3v1n0)]

We probably have some issue with the builder... I'll re-approve it, once fixed.

[Michael Rawson (michael.rawson)]

Not a problem. Happy to help as much as possible. Give us a shout if you need anything. :)

[Michael Rawson (michael.rawson)]

Awesome!

Thanks Andrea, Marco.

[Didier Roche-Tolomelli (didrocks)]

seems there were some SSL issue in the QA datacenter

[Michael Rawson (michael.rawson)]

Well, you got it fixed, that's all that matters. :)

=== modified file 'plugins/unityshell/src/DesktopLauncherIcon.cpp'
--- plugins/unityshell/src/DesktopLauncherIcon.cpp	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/DesktopLauncherIcon.cpp	2012-04-18 18:06:14 +0000
@@ -35,8 +35,7 @@
   icon_name = "desktop";
   SetQuirk(QUIRK_VISIBLE, true);
   SetQuirk(QUIRK_RUNNING, false);
-  SetIconType(TYPE_BEGIN);
-  SetShowInSwitcher(false);
+  SetIconType(TYPE_DESKTOP);
 }
 
 DesktopLauncherIcon::~DesktopLauncherIcon()

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-04-11 07:02:26 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-04-18 18:06:14 +0000
@@ -151,7 +151,6 @@
   DeviceLauncherSection* device_section_;
   LauncherEntryRemoteModel remote_model_;
   AbstractLauncherIcon::Ptr expo_icon_;
-  AbstractLauncherIcon::Ptr desktop_launcher_icon_;
   AbstractLauncherIcon::Ptr desktop_icon_;
   int                    num_workspaces_;
   bool                   show_desktop_icon_;
@@ -700,13 +699,12 @@
 
 void Controller::Impl::InsertDesktopIcon()
 {
-  desktop_launcher_icon_ = AbstractLauncherIcon::Ptr(new DesktopLauncherIcon());
-  RegisterIcon(desktop_launcher_icon_);
+  RegisterIcon(desktop_icon_);
 }
 
 void Controller::Impl::RemoveDesktopIcon()
 {
-  model_->RemoveIcon(desktop_launcher_icon_);
+  model_->RemoveIcon(desktop_icon_);
 }
 
 void Controller::Impl::RegisterIcon(AbstractLauncherIcon::Ptr icon)
@@ -900,13 +898,20 @@
 std::vector<AbstractLauncherIcon::Ptr> Controller::GetAltTabIcons(bool current) const
 {
   std::vector<AbstractLauncherIcon::Ptr> results;
-
+  
   results.push_back(pimpl->desktop_icon_);
 
   for (auto icon : *(pimpl->model_))
+  {
     if (icon->ShowInSwitcher(current))
-      results.push_back(icon);
-
+    {
+      //otherwise we get two desktop icons in the switcher.
+      if (icon->GetIconType() != AbstractLauncherIcon::IconType::TYPE_DESKTOP)
+      {
+        results.push_back(icon);
+      }
+    }
+  } 
   return results;
 }
 

=== modified file 'plugins/unityshell/src/SwitcherController.cpp'
--- plugins/unityshell/src/SwitcherController.cpp	2012-04-16 01:03:10 +0000
+++ plugins/unityshell/src/SwitcherController.cpp	2012-04-18 18:06:14 +0000
@@ -439,6 +439,12 @@
 {
   if (first->GetIconType() == second->GetIconType())
     return first->SwitcherPriority() > second->SwitcherPriority();
+    
+  if (first->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP)
+	return true;
+	
+  if (second->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP)
+	return false;
   return first->GetIconType() < second->GetIconType();
 }
 

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-04-15 10:44:39 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-04-18 18:06:14 +0000
@@ -1732,7 +1732,7 @@
 
   auto results = launcher_controller_->GetAltTabIcons(show_mode == switcher::ShowMode::CURRENT_VIEWPORT);
 
-  if (!(results.size() == 1 && results[0]->GetIconType() == AbstractLauncherIcon::IconType::TYPE_BEGIN))
+  if (!(results.size() == 1 && results[0]->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP))
     switcher_controller_->Show(show_mode, switcher::SortMode::FOCUS_ORDER, false, results);
 
   return true;