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

Proposed by Michael Rawson
Status: Merged
Approved by: Marco Trevisan (Treviño)
Approved revision: 2287
Merged at revision: 2326
Proposed branch: lp:~michael.rawson/unity/fix-show-desktop
Merge into: lp:unity
Diff against target: 95 lines (+20/-10)
4 files modified
plugins/unityshell/src/DesktopLauncherIcon.cpp (+1/-2)
plugins/unityshell/src/LauncherController.cpp (+12/-7)
plugins/unityshell/src/SwitcherController.cpp (+6/-0)
plugins/unityshell/src/unityshell.cpp (+1/-1)
To merge this branch: bzr merge lp:~michael.rawson/unity/fix-show-desktop
Reviewer Review Type Date Requested Status
Marco Trevisan (Treviño) Approve
Andrea Azzarone (community) Needs Fixing
Review via email: mp+101961@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Andrea Azzarone (azzar1) wrote :

LGTM

review: Approve
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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
Revision history for this message
Andrea Azzarone (azzar1) wrote :

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

Revision history for this message
Michael Rawson (michael.rawson) wrote :

> 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.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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 ;)

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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. :)

Revision history for this message
Andrea Azzarone (azzar1) wrote :

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
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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.

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Revision history for this message
Andrea Azzarone (azzar1) wrote :

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.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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_);

Revision history for this message
Andrea Azzarone (azzar1) wrote :

The CCSM should not touch the Switcher.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

> The CCSM should not touch the Switcher.

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

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Revision history for this message
Andrea Azzarone (azzar1) wrote :

Not sure if we have something like SetShowInLauncher.

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

> 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);
}

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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);
    }
  }
}

;)

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Cool, thanks.

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

No commit message specified.

Revision history for this message
Unity Merger (unity-merger) wrote :

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.

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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

Revision history for this message
Unity Merger (unity-merger) wrote :

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.

Revision history for this message
Michael Rawson (michael.rawson) wrote :

Build 695 doesn't exist yet...

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

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

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Revision history for this message
Michael Rawson (michael.rawson) wrote :

Awesome!

Thanks Andrea, Marco.

Revision history for this message
Didier Roche (didrocks) wrote :

seems there were some SSL issue in the QA datacenter

Revision history for this message
Michael Rawson (michael.rawson) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
35 icon_name = "desktop";35 icon_name = "desktop";
36 SetQuirk(QUIRK_VISIBLE, true);36 SetQuirk(QUIRK_VISIBLE, true);
37 SetQuirk(QUIRK_RUNNING, false);37 SetQuirk(QUIRK_RUNNING, false);
38 SetIconType(TYPE_BEGIN);38 SetIconType(TYPE_DESKTOP);
39 SetShowInSwitcher(false);
40}39}
4140
42DesktopLauncherIcon::~DesktopLauncherIcon()41DesktopLauncherIcon::~DesktopLauncherIcon()
4342
=== 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 @@
151 DeviceLauncherSection* device_section_;151 DeviceLauncherSection* device_section_;
152 LauncherEntryRemoteModel remote_model_;152 LauncherEntryRemoteModel remote_model_;
153 AbstractLauncherIcon::Ptr expo_icon_;153 AbstractLauncherIcon::Ptr expo_icon_;
154 AbstractLauncherIcon::Ptr desktop_launcher_icon_;
155 AbstractLauncherIcon::Ptr desktop_icon_;154 AbstractLauncherIcon::Ptr desktop_icon_;
156 int num_workspaces_;155 int num_workspaces_;
157 bool show_desktop_icon_;156 bool show_desktop_icon_;
@@ -700,13 +699,12 @@
700699
701void Controller::Impl::InsertDesktopIcon()700void Controller::Impl::InsertDesktopIcon()
702{701{
703 desktop_launcher_icon_ = AbstractLauncherIcon::Ptr(new DesktopLauncherIcon());702 RegisterIcon(desktop_icon_);
704 RegisterIcon(desktop_launcher_icon_);
705}703}
706704
707void Controller::Impl::RemoveDesktopIcon()705void Controller::Impl::RemoveDesktopIcon()
708{706{
709 model_->RemoveIcon(desktop_launcher_icon_);707 model_->RemoveIcon(desktop_icon_);
710}708}
711709
712void Controller::Impl::RegisterIcon(AbstractLauncherIcon::Ptr icon)710void Controller::Impl::RegisterIcon(AbstractLauncherIcon::Ptr icon)
@@ -900,13 +898,20 @@
900std::vector<AbstractLauncherIcon::Ptr> Controller::GetAltTabIcons(bool current) const898std::vector<AbstractLauncherIcon::Ptr> Controller::GetAltTabIcons(bool current) const
901{899{
902 std::vector<AbstractLauncherIcon::Ptr> results;900 std::vector<AbstractLauncherIcon::Ptr> results;
903901
904 results.push_back(pimpl->desktop_icon_);902 results.push_back(pimpl->desktop_icon_);
905903
906 for (auto icon : *(pimpl->model_))904 for (auto icon : *(pimpl->model_))
905 {
907 if (icon->ShowInSwitcher(current))906 if (icon->ShowInSwitcher(current))
908 results.push_back(icon);907 {
909908 //otherwise we get two desktop icons in the switcher.
909 if (icon->GetIconType() != AbstractLauncherIcon::IconType::TYPE_DESKTOP)
910 {
911 results.push_back(icon);
912 }
913 }
914 }
910 return results;915 return results;
911}916}
912917
913918
=== 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 @@
439{439{
440 if (first->GetIconType() == second->GetIconType())440 if (first->GetIconType() == second->GetIconType())
441 return first->SwitcherPriority() > second->SwitcherPriority();441 return first->SwitcherPriority() > second->SwitcherPriority();
442
443 if (first->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP)
444 return true;
445
446 if (second->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP)
447 return false;
442 return first->GetIconType() < second->GetIconType();448 return first->GetIconType() < second->GetIconType();
443}449}
444450
445451
=== 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 @@
17321732
1733 auto results = launcher_controller_->GetAltTabIcons(show_mode == switcher::ShowMode::CURRENT_VIEWPORT);1733 auto results = launcher_controller_->GetAltTabIcons(show_mode == switcher::ShowMode::CURRENT_VIEWPORT);
17341734
1735 if (!(results.size() == 1 && results[0]->GetIconType() == AbstractLauncherIcon::IconType::TYPE_BEGIN))1735 if (!(results.size() == 1 && results[0]->GetIconType() == AbstractLauncherIcon::IconType::TYPE_DESKTOP))
1736 switcher_controller_->Show(show_mode, switcher::SortMode::FOCUS_ORDER, false, results);1736 switcher_controller_->Show(show_mode, switcher::SortMode::FOCUS_ORDER, false, results);
17371737
1738 return true;1738 return true;