Merge lp:~townsend/compiz/fix-snap-window-resize-0.9.10 into lp:compiz/0.9.10

Proposed by Christopher Townsend
Status: Merged
Approved by: Marco Trevisan (Treviño)
Approved revision: 3795
Merged at revision: 3796
Proposed branch: lp:~townsend/compiz/fix-snap-window-resize-0.9.10
Merge into: lp:compiz/0.9.10
Diff against target: 81 lines (+0/-71)
1 file modified
plugins/snap/src/snap.cpp (+0/-71)
To merge this branch: bzr merge lp:~townsend/compiz/fix-snap-window-resize-0.9.10
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+189157@code.launchpad.net

Commit message

Remove seemingly useless code in the SnapWindow::resizeNotify() method where a snapped window that is being resized on the opposite side of the snap would cause orders of magnitude more calls to window->resize() and kill performance.

Description of the change

= Issue =
Set Default Resizing mode to Normal. Snap a window to an edge such as the Launcher or the Panel or screen edge. Resize the opposite side of the snapped edge. Terrible performance ensues.

= Fix =
A seemingly useless code block in the SnapWindow::resizeNotify() method would make orders of magnitude more calls to window->resize() in this case. Each call destroys/creates a new X Window. For example a normal resize may call resize 5 times, but with this code in place, it may call 5000 times or more.

= Note =
I could not figure out what the real intent of the code block was for when resizing a window. It just didn't seem like it was supposed to do anything. As a matter of fact, when using the Default Resizing mode as Rectangle, this code was never touched, so it's not as if resizing a snapped window needed to do something special. At any rate, some pretty good testing of various resizing should be done to ensure there is no regressions by removing this code.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

LGTM

But would like someone else to test it out :)

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

This seems to work well here, I was jut worried by the possibility that this could lead to non working snapping when resizing a window (if enabled), but I don't see that working even in normal version... So I think this is fine

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/snap/src/snap.cpp'
--- plugins/snap/src/snap.cpp 2013-05-09 13:43:07 +0000
+++ plugins/snap/src/snap.cpp 2013-10-03 18:19:48 +0000
@@ -587,77 +587,6 @@
587 if (!ss->snapping)587 if (!ss->snapping)
588 return;588 return;
589589
590 // apply edge resistance
591 if (ss->optionGetSnapTypeMask () & SnapTypeEdgeResistanceMask)
592 {
593 // If there's horizontal snapping, add dx to current buffered
594 // dx and resist (move by -dx) or release the window and move
595 // by buffered dx - dx, same for dh
596 if (!snapGeometry.isEmpty () && snapDirection & HorizontalSnap)
597 {
598 m_dx += dx;
599 if (m_dx < ss->optionGetResistanceDistance ()
600 && m_dx > -ss->optionGetResistanceDistance ())
601 {
602 resize (-dx, 0, 0, 0);
603 }
604 else
605 {
606 resize (m_dx - dx, 0, 0, 0);
607 m_dx = 0;
608 if (!m_dwidth)
609 snapDirection &= VerticalSnap;
610 }
611 m_dwidth += dwidth;
612 if (m_dwidth < ss->optionGetResistanceDistance ()
613 && m_dwidth > -ss->optionGetResistanceDistance ())
614 {
615 resize (0, 0, -dwidth, 0);
616 }
617 else
618 {
619 resize (0, 0, m_dwidth - dwidth, 0);
620 m_dwidth = 0;
621 if (!m_dx)
622 snapDirection &= VerticalSnap;
623 }
624 }
625
626 // Same for vertical snapping and dy/dh
627 if (snapGeometry.isEmpty () && snapDirection & VerticalSnap)
628 {
629 m_dy += dy;
630 if (m_dy < ss->optionGetResistanceDistance ()
631 && m_dy > -ss->optionGetResistanceDistance ())
632 {
633 resize (0, -dy, 0, 0);
634 }
635 else
636 {
637 resize (0, m_dy - dy, 0, 0);
638 m_dy = 0;
639 if (!m_dheight)
640 snapDirection &= HorizontalSnap;
641 }
642 m_dheight += dheight;
643 if (m_dheight < ss->optionGetResistanceDistance ()
644 && m_dheight > -ss->optionGetResistanceDistance ())
645 {
646 resize (0, 0, 0, -dheight);
647 }
648 else
649 {
650 resize (0, 0, 0, m_dheight - dheight);
651 m_dheight = 0;
652 if (!m_dy)
653 snapDirection &= HorizontalSnap;
654 }
655 }
656 // If we are no longer snapping in any direction, reset snapped
657 if (!snapGeometry.isEmpty () && !snapDirection)
658 snapGeometry = CompWindow::Geometry ();
659 }
660
661 // If we don't already snap vertically and horizontally,590 // If we don't already snap vertically and horizontally,
662 // check edges status591 // check edges status
663 if (snapDirection != (VerticalSnap | HorizontalSnap))592 if (snapDirection != (VerticalSnap | HorizontalSnap))

Subscribers

People subscribed via source and target branches