Bug 121108 - Kst assertion failure when line or arrow moved outside of window
Summary: Kst assertion failure when line or arrow moved outside of window
Status: RESOLVED FIXED
Alias: None
Product: kst
Classification: Applications
Component: general (show other bugs)
Version: 1.x
Platform: Compiled Sources Linux
: NOR crash
Target Milestone: ---
Assignee: kst
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-31 21:05 UTC by Andrew Walker
Modified: 2006-02-04 20:31 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
linemoving.patch (4.49 KB, text/x-diff)
2006-02-02 12:11 UTC, George Staikos
Details
Proposed patch (5.60 KB, patch)
2006-02-02 19:09 UTC, Andrew Walker
Details
Tweak to George's patch (5.52 KB, patch)
2006-02-03 21:49 UTC, Netterfield
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Walker 2006-01-31 21:05:04 UTC
Version:           HEAD (using KDE KDE 3.5.0)
Installed from:    Compiled From Sources
OS:                Linux

PROBLEM:
Moving one end of a line or arrow object outside of its window causes a crash.

STEPS TO REPRODUCE:
Start Kst
In the default window create a line contained within the default
Switch to layout mode
Click on the line to select it
Move the cursor over one of the end-points of the line
Drag the end-point so that it moves outside of the window
Release the mouse button

RESULTS:
Crash

EXPECTED RESULTS:
Cannot drag end-point outside of the window
Comment 1 Andrew Walker 2006-01-31 21:05:37 UTC
Stack dump:

Using host libthread_db library "/lib/tls/libthread_db.so.1".
[Thread debugging using libthread_db enabled]
[New Thread -187439392 (LWP 30535)]
[New Thread -192955472 (LWP 30536)]
[Thread debugging using libthread_db enabled]
[New Thread -187439392 (LWP 30535)]
[New Thread -192955472 (LWP 30536)]
[Thread debugging using libthread_db enabled]
[New Thread -187439392 (LWP 30535)]
[New Thread -192955472 (LWP 30536)]
[KCrash handler]
#4  0xf6fe97a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#5  0xf54cc955 in raise () from /lib/tls/libc.so.6
#6  0xf54ce319 in abort () from /lib/tls/libc.so.6
#7  0xf54c5f41 in __assert_fail () from /lib/tls/libc.so.6
#8  0xf6e7617c in KstViewObject::updateFromAspect (this=0x8ffec40)
    at qrect.h:188
#9  0xf6e7bd9d in KstViewObject::resize (this=0x8ffec40, size=@0xfef9f478)
    at kstviewobject.cpp:566
#10 0xf6e67b35 in KstViewLine::updateOrientation (this=0x8ffec40)
    at qsize.h:120
#11 0xf6e67ca6 in KstViewLine::setFrom (this=0x8ffec40, from=@0x6)
    at kstviewline.cpp:156
#12 0xf6e89a96 in KstTopLevelView::releasePressLayoutModeEndPoint (
    this=0x8ffeac0, pos=@0xfef9facc, shift=false) at kstsharedptr.h:136
#13 0xf6e8d068 in KstTopLevelView::releasePressLayoutMode (this=0x8ffeac0, 
    pos=@0xfef9facc, shift=false) at ksttoplevelview.cpp:875
#14 0xf6e8d274 in KstTopLevelView::releasePress (this=0x8ffeac0, 
    pos=@0xfef9facc, shift=false) at ksttoplevelview.cpp:1036
#15 0xf6e71371 in KstViewWidget::mouseReleaseEvent (this=0x8e1f618, 
    e=0xfef9fac0) at kstsharedptr.h:136
#16 0xf5bf95c4 in QWidget::event () from /usr/qt/lib/libqt-mt.so.3
#17 0xf5b59a91 in QApplication::internalNotify ()
   from /usr/qt/lib/libqt-mt.so.3
#18 0xf5b59d5f in QApplication::notify () from /usr/qt/lib/libqt-mt.so.3
#19 0xf62de4c8 in KApplication::notify () from /usr/lib/libkdecore.so.4
#20 0xf5af3195 in QETWidget::translateMouseEvent ()
   from /usr/qt/lib/libqt-mt.so.3
#21 0xf5af1402 in QApplication::x11ProcessEvent ()
   from /usr/qt/lib/libqt-mt.so.3
#22 0xf5b04456 in QEventLoop::processEvents () from /usr/qt/lib/libqt-mt.so.3
#23 0xf5b6f3a3 in QEventLoop::enterLoop () from /usr/qt/lib/libqt-mt.so.3
#24 0xf5b6f300 in QEventLoop::exec () from /usr/qt/lib/libqt-mt.so.3
#25 0xf5b58c90 in QApplication::exec () from /usr/qt/lib/libqt-mt.so.3
#26 0x08054c9e in main (argc=1, argv=0xfefa0be4) at main.cpp:814
Comment 2 Andrew Walker 2006-02-01 23:21:31 UTC
Propoased patch which keeps the line/arrow within the parent, thus avoiding the assert:

Index: ksttoplevelview.cpp
===================================================================
--- ksttoplevelview.cpp (revision 504684)
+++ ksttoplevelview.cpp (working copy)
@@ -37,6 +37,7 @@
 #include "kstaccessibility.h"
 #include "kstdoc.h"
 #include "kstgfxmousehandler.h"
+#include "kstmath.h"
 #include "kstplotgroup.h"
 #include "kstsettings.h"
 #include "ksttimers.h"
@@ -780,12 +781,80 @@
 }


+QPoint KstTopLevelView::handlePoint(bool shift, const QPoint& ptFixed, const QPoint& pos, double aspect) {
+  QPoint ptMoved = pos;
+
+  if (shift) {
+    double absAspect = fabs(aspect);
+    if (absAspect < 500.0 && (double(abs((pos.y() - ptFixed.y())/(pos.x() - ptFixed.x()))) < aspect ||
+        absAspect < 0.1)) {
+      ptMoved = QPoint(pos.x(), ptFixed.y() + int(aspect*(pos.x() - ptFixed.x())));
+    } else {
+      ptMoved = QPoint(ptFixed.x() + int((pos.y() - ptFixed.y())/aspect), pos.y());
+    }
+  }
+
+  //
+  // ensure that the point we just moved is contained by its parent...
+  //
+  QRect parentRect = _pressTarget->_parent->_geom;
+  if (parentRect.contains(ptFixed) && !parentRect.contains(ptMoved)) {
+    double gradient;
+    bool found = false;
+    int x;
+    int y;
+
+    if (!found && ptMoved.x() < parentRect.left()) {
+      gradient = double(ptMoved.y() - ptFixed.y())/double(ptMoved.x() - ptFixed.x());
+      x = parentRect.left();
+      y = ptFixed.y() + d2i(double(x - ptFixed.x()) * gradient);
+      if (y >= parentRect.top() && y <= parentRect.bottom()) {
+        found = true;
+      }
+    }
+    if (!found && ptMoved.x() > parentRect.right()) {
+      gradient = double(ptMoved.y() - ptFixed.y())/double(ptMoved.x() - ptFixed.x());
+      x = parentRect.right();
+      y = ptFixed.y() + d2i(double(x - ptFixed.x()) * gradient);
+      if (y >= parentRect.top() && y <= parentRect.bottom()) {
+        found = true;
+      }
+    }
+    if (!found && ptMoved.y() < parentRect.top()) {
+      gradient = double(ptMoved.x() - ptFixed.x())/double(ptMoved.y() - ptFixed.y());
+      y = parentRect.top();
+      x = ptFixed.x() + d2i(double(y - ptFixed.y()) * gradient);
+      if (x >= parentRect.left() && x <= parentRect.right()) {
+        found = true;
+      }
+    }
+    if (!found && ptMoved.y() > parentRect.bottom()) {
+      gradient = double(ptMoved.x() - ptFixed.x())/double(ptMoved.y() - ptFixed.y());
+      y = parentRect.bottom();
+      x = ptFixed.x() + d2i(double(y - ptFixed.y()) * gradient);
+      if (x >= parentRect.left() && x <= parentRect.right()) {
+        found = true;
+      }
+    }
+
+    if (found) {
+      ptMoved.setX(x);
+      ptMoved.setY(y);
+    }
+  }
+
+  return ptMoved;
+}
+
+
 void KstTopLevelView::pressMoveLayoutModeEndPoint(const QPoint& pos, bool shift) {
   // FIXME: remove this!!  Should not know about any specific type
   // for now we only know how to deal with lines
   if (KstViewLinePtr line = kst_cast<KstViewLine>(_pressTarget)) {
     const QRect old(_prevBand);
+    QPoint ptFixed;
     double aspect;
+
     if (line->to().x() != line->from().x()) {
       aspect = double(line->to().y() - line->from().y()) / double(line->to().x() - line->from().x());
     } else {
@@ -795,40 +864,22 @@
         aspect = 1.0E300;
       }
     }
-    QPoint fromPoint, toPoint;
+
     if (_pressDirection & UP) {
-      // UP means we are on the start endpoint
-      toPoint = line->to();
-      if (shift) {
-        double absAspect = fabs(aspect);
-        if (absAspect < 500 && (double(abs((pos.y() - toPoint.y())/(pos.x() - toPoint.x()))) < aspect || absAspect < 0.1)) {
-          fromPoint = QPoint(pos.x(), toPoint.y() + int(aspect*(pos.x() - toPoint.x())));
-        } else {
-          fromPoint = QPoint(toPoint.x() + int((pos.y() - toPoint.y())/aspect), pos.y());
-        }
-      } else {
-        fromPoint = pos;
-      }
+      // UP means we are on the "from" point
+      ptFixed = line->to();
+      _prevBand.setTopLeft(handlePoint(shift, ptFixed, pos, aspect));
+      _prevBand.setBottomRight(ptFixed);
     } else if (_pressDirection & DOWN) {
-      // DOWN means we are on the end endpoint
-      fromPoint = line->from();
-      if (shift) {
-        double absAspect = fabs(aspect);
-        if (absAspect < 500 && (double(abs((pos.y() - toPoint.y())/(pos.x() - toPoint.x()))) < aspect || absAspect < 0.1)) {
-          toPoint = QPoint(pos.x(), fromPoint.y() + int(aspect*(pos.x() - fromPoint.x())));
-        } else {
-          toPoint = QPoint(fromPoint.x() + int((pos.y() - fromPoint.y())/aspect), pos.y());
-        }
-      } else {
-        toPoint = pos;
-      }
+      // DOWN means we are on the "to" point
+      ptFixed = line->from();
+      _prevBand.setTopLeft(ptFixed);
+      _prevBand.setBottomRight(handlePoint(shift, ptFixed, pos, aspect));
     } else {
-      abort();
+      assert(false);
+      return;
     }

-    _prevBand.setTopLeft(fromPoint);
-    _prevBand.setBottomRight(toPoint);
-
     if (old != _prevBand) {
       KstPainter p;
       p.begin(_w);
Index: ksttoplevelview.h
===================================================================
--- ksttoplevelview.h   (revision 504664)
+++ ksttoplevelview.h   (working copy)
@@ -98,6 +98,7 @@
     void pressMove(const QPoint& pos, bool shift = false);
     void pressMoveLayoutMode(const QPoint& pos, bool shift = false);
     // helpers for pressMoveLayoutMode
+    QPoint handlePoint(bool shift, const QPoint& ptFixed, const QPoint& pos, double aspect);
     void pressMoveLayoutModeMove(const QPoint& pos, bool shift = false);
     void pressMoveLayoutModeResize(const QPoint& pos, bool shift = false);
     void pressMoveLayoutModeSelect(const QPoint& pos, bool shift = false);
Comment 3 George Staikos 2006-02-02 12:11:20 UTC
> +    QPoint handlePoint(bool shift, const QPoint& ptFixed, const QPoint&
> pos, double aspect); void pressMoveLayoutModeMove(const QPoint& pos, bool


  The meaning of this function is entirely unclear.

>  void KstTopLevelView::pressMoveLayoutModeEndPoint(const QPoint& pos, bool


  And I think all of the fix should go in here.

  As far as I can tell, all we need to do is chop the line into the geometry 
of the view.  This function can already do all the important calculations 
internally if it just makes use of the QPoint argument.  Unfortunately Qt3's 
QRect doesn't ::intersect() well when it isn't normalized, so this patch is 
more complicated than it should be.

  One other bug I noticed is that the viewline object doesn't track moves 
properly, so it sometimes gets desynchronized.  This patch also fixes that 
problem.


Created an attachment (id=14494)
linemoving.patch
Comment 4 Andrew Walker 2006-02-02 19:09:02 UTC
Created attachment 14499 [details]
Proposed patch
Comment 5 Andrew Walker 2006-02-02 19:22:16 UTC
> One other bug I noticed is that the viewline object doesn't track moves
> properly, so it sometimes gets desynchronized.  This patch also fixes that
> problem. 

George, could you provide more details on this. How do I reproduce it?
Comment 6 George Staikos 2006-02-02 22:02:51 UTC
On Thursday 02 February 2006 13:22, Andrew Walker wrote:
> George, could you provide more details on this. How do I reproduce it?


  Move a line and check the from/to values from KstScript, comparing to the 
bounding box.  They don't stay synchronized.
Comment 7 Andrew Walker 2006-02-02 23:59:04 UTC
A  minor point but it looks as if George's patch simply "rescues" a line that extends beyond its parent by considering the x and y values independently. This has the effect of not drawing a line that extends partway between the fixed point and the mouse position, but instead draws a line between the fixed point and some point on the edge of the parent. The effect is less intuitive but simpler to calculate.
Comment 8 Andrew Walker 2006-02-03 00:52:07 UTC
> Move a line and check the from/to values from KstScript, comparing to the 
> bounding box.  They don't stay synchronized. 

This sounds like the same thing you describe in 121068. Is that the case?
Comment 9 Netterfield 2006-02-03 01:30:10 UTC
I think that Andrew's patch behavior is the most consistent.
-Like all other view objects, it doesn't reparent.
-Like all other view objects, it doesn't let the line leave the parent.
-The UI feedback does not imply that it can leave the parent.
-The line lands exactly where the feedback told you it would be.

It is OK to commit in my mind, but we can wait 'till tomorrow for George to comment (or he can tweak and commit it).
Comment 10 Andrew Walker 2006-02-03 19:40:53 UTC
Restrict lines and arrows to their parent - consistent with other view objects.
Comment 11 Netterfield 2006-02-03 21:49:39 UTC
Created attachment 14533 [details]
Tweak to George's patch
Comment 12 Netterfield 2006-02-03 21:51:51 UTC
We still have dueling patches.

This is a tweak to George's patch which:
-Keeps the band line consistent with where the line will actually end up when the pointer is outside the tlv
-re-parents after the move.

Upon further consideration, I really like allowing the line to move outside the current parent - which is what this does.
Comment 13 Andrew Walker 2006-02-03 22:30:42 UTC
If we're going to allow the line outside of the parent on a resize then we should also do the same for all the other view objects.
Comment 14 George Staikos 2006-02-04 14:37:56 UTC
On Friday 03 February 2006 16:34, Barth Netterfield wrote:
> I agree, but not for 1.2.0.  I tried to deal with this for a bit, but it
> will touch too much code at this late time.  The reparenting was the
> problem. But the line stuff looks pretty solid now.


  I disagree.  See upcoming email.
Comment 15 Netterfield 2006-02-04 20:31:05 UTC
This bug has been fixed, but I BUG:ed the wrong bug number on commit.