Merge lp:~genius12a/unity/unity into lp:unity

Proposed by Anish Dowlut on 2014-06-26
Status: Rejected
Rejected by: Christopher Townsend on 2015-03-12
Proposed branch: lp:~genius12a/unity/unity
Merge into: lp:unity
Diff against target: 30 lines (+5/-1)
1 file modified
launcher/ApplicationLauncherIcon.cpp (+5/-1)
To merge this branch: bzr merge lp:~genius12a/unity/unity
Reviewer Review Type Date Requested Status
Marco Trevisan (Treviño) 2014-06-26 Disapprove on 2014-06-28
Review via email: mp+224692@code.launchpad.net

Description of the change

I have made a commit to fix bug #1170647. The revision number is 3831.

To post a comment you must log in.
Marco Trevisan (Treviño) (3v1n0) wrote :

I've another solution for this, more complex, but it's something I need to do ASAP... We still want these device and trash icons to be used to handle the file manager, but we don't want the nautilus icon to fight with them...

So, for now I'd prefer to avoid this.

review: Disapprove
Anish Dowlut (genius12a) wrote :

On 06/28/2014 04:40 AM, Marco Trevisan (Treviño) wrote:
> Review: Disapprove
>
> I've another solution for this, more complex, but it's something I need to do ASAP... We still want these device and trash icons to be used to handle the file manager, but we don't want the nautilus icon to fight with them...
>
> So, for now I'd prefer to avoid this.
I do agree with you that the devices and trash icons could be better
used in Unity. I've noted that you've already started taking steps to
achieve this in bug #1063830, #1161323 and #75393. I understand that you
are still working on a perfect solution for the problem. However, since
the solution is not complete yet, the changes in behavior that you've
made to Unity does not seem correct at the moment (which is why bug
#1170647
was filed). Of course, the new behavior would make complete
sense once the whole solution has been delivered. But for now, I believe
it would be best if all changes were reverted until the complete
solution is available.

By the way, I have a few ideas that could help solving the whole problem
and may be I can help a bit with the code. Where is the right place to
discuss that?

Marco Trevisan (Treviño) (3v1n0) wrote :

Sure, happy to hear feedback. Feel free to ping me in #ubuntu-desktop or #ubuntu-unity in freenode

Christopher Townsend (townsend) wrote :

I'm rejecting this as Marco has disapproved.

Unmerged revisions

3831. By Anish Dowlut on 2014-06-26

Fixed bug #1170647

Description of fix: Commented a section of code. The section of code was introduced to fix an earlier bug #753938, but it lead to incorrect behaviours in a few situations which are described in bug #1170647. The expected behaviours specified in these two bug reports actually contradicts each other. Hence, in order to fix bug #1170647, the changes for bug #753938 had to be reverted.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/ApplicationLauncherIcon.cpp'
2--- launcher/ApplicationLauncherIcon.cpp 2014-06-07 16:27:16 +0000
3+++ launcher/ApplicationLauncherIcon.cpp 2014-06-26 17:22:38 +0000
4@@ -351,6 +351,9 @@
5 active = false;
6 }
7
8+ /* The following code section was commented to fix bug #1170647,
9+ * potentially/essentially reverting the changes made for fixing bug
10+ * #753938.
11 if (user_visible && IsSticky() && IsFileManager())
12 {
13 // See bug #753938
14@@ -372,6 +375,7 @@
15 }
16 }
17 }
18+ */
19 }
20
21 /* Behaviour:
22@@ -396,7 +400,7 @@
23 {
24 if (scaleWasActive) // #5 above
25 {
26- if (minimize_window_on_click())
27+ if (minimize_window_on_click)
28 {
29 for (auto const& win : GetWindows(WindowFilter::ON_CURRENT_DESKTOP))
30 wm.Minimize(win->window_id());