Merge lp:~sil2100/compiz-core/scale_all-0.9.7 into lp:compiz-core

Proposed by Łukasz Zemczak on 2012-06-21
Status: Merged
Approved by: Daniel van Vugt on 2012-07-02
Approved revision: 3105
Merged at revision: 3110
Proposed branch: lp:~sil2100/compiz-core/scale_all-0.9.7
Merge into: lp:compiz-core
Diff against target: 64 lines (+43/-0)
2 files modified
plugins/scale/src/privates.h (+2/-0)
plugins/scale/src/scale.cpp (+41/-0)
To merge this branch: bzr merge lp:~sil2100/compiz-core/scale_all-0.9.7
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve on 2012-07-02
Sam Spilsbury 2012-06-21 Approve on 2012-06-22
Review via email:

Commit message

Add support for initiating window picker in other than nomal mode. For now added only the additional 'All windows' picker, essentially fixing LP: #933776.
The code from the all mode comes from a patch made by Doug McMahon. Thanks!

Description of the change

Submitting the fix first to 0.9.7 because I had the branch prepared and I want to go to sleep ASAP. But still, I would like some comment on this way of fixing the bug.

The scale plugin has many actions defined: initiate for current workspace, for all windows, for groups and for current output device. But the scale plugin only supports one handler for all actions (the type of the action is not considered at all). For now I added a distinction between ScaleTypeAll and ScaleTypeNormal. I took the code proposed by Doug in the bug report as the code for ScaleTypeAll.

Would be nice to have this SRUed.

I browsed through the commits, but I never really saw any type-distinct action code. But I might have missed that, since it's REALLY busy here in London!lp:~sil2100/compiz-core/scale_all-0.9.7

To post a comment you must log in.
Łukasz Zemczak (sil2100) wrote :

Will add the trunk version in the morning, if this one is alright.

Sam Spilsbury (smspillaz) wrote :

Looks fine to me. Are you willing to write tests for it?

review: Approve
Daniel van Vugt (vanvugt) wrote :

I'm going to say Needs fixing, just because there is no fix upstream yet :)

Also, I strongly recommend investigating the other scale regressions and starting by simply reverting the code that caused the regressions from oneiric to precise. We will fix more bugs, faster and more reliably. Then we can re-implement whatever was reverted more safely.

If we accept bespoke fixes like this, then fixing the other scale regressions in precise becomes harder.

review: Needs Fixing
Sam Spilsbury (smspillaz) wrote :

> I'm going to say Needs fixing, just because there is no fix upstream yet :)
> Also, I strongly recommend investigating the other scale regressions and
> starting by simply reverting the code that caused the regressions

This will inherently "cause" regressions by the fact that you just reverted important bugfixes. It does not take into account the waste of developer time as they are chasing up old code that they probably don't even remember writing.

If you want to fix things safely, analyse how the code works, analyse what its meant to do, write unit tests to ensure that it matches the required behaviour and implement your fixes. I don't revert things simply because I don't understand them - I learn the code by making it better and writing tests to ensure that it does what I think it does.

Lucasz - can you propose this fix to lp:compiz and then resubmit it here? Thanks for taking the time to look into this.

Daniel van Vugt (vanvugt) wrote :

Approved for upstream today. So 0.9.7 is OK too.

This seems to solve 2 of the 3 scale regressions. And the other one, we found last week, was actually a unity bug.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/scale/src/privates.h'
2--- plugins/scale/src/privates.h 2012-02-01 17:49:07 +0000
3+++ plugins/scale/src/privates.h 2012-06-21 23:21:19 +0000
4@@ -63,6 +63,8 @@
5 void findBestSlots ();
6 bool fillInWindows ();
7 bool layoutThumbs ();
8+ bool layoutThumbsAll ();
9+ bool layoutThumbsSingle ();
11 SlotArea::vector getSlotAreas ();
14=== modified file 'plugins/scale/src/scale.cpp'
15--- plugins/scale/src/scale.cpp 2012-04-01 07:34:59 +0000
16+++ plugins/scale/src/scale.cpp 2012-06-21 23:21:19 +0000
17@@ -684,6 +684,47 @@
18 bool
19 PrivateScaleScreen::layoutThumbs ()
20 {
21+ switch (type) {
22+ case ScaleTypeAll:
23+ return layoutThumbsAll ();
24+ case ScaleTypeNormal:
25+ default:
26+ return layoutThumbsSingle ();
27+ }
31+PrivateScaleScreen::layoutThumbsAll ()
33+ windows.clear ();
35+ /* add windows scale list, top most window first */
36+ foreach (CompWindow *w, screen->windows ())
37+ {
40+ if (sw->priv->slot)
41+ sw->priv->adjust = true;
43+ sw->priv->slot = NULL;
45+ if (!sw->priv->isScaleWin ())
46+ continue;
48+ windows.push_back (sw);
49+ }
51+ if (windows.empty ())
52+ return false;
54+ slots.resize (windows.size ());
56+ return ScaleScreen::get (screen)->layoutSlotsAndAssignWindows ();
60+PrivateScaleScreen::layoutThumbsSingle ()
62 bool ret = false;
63 std::map <ScaleWindow *, ScaleSlot> slotWindows;
64 CompWindowList allWindows;


People subscribed via source and target branches