Bug 128399 - Resizing horizontal lines can cause a crash when maintain aspect modifier is held down.
Summary: Resizing horizontal lines can cause a crash when maintain aspect modifier is ...
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-06-01 02:19 UTC by Duncan Hanson
Modified: 2006-06-07 09:56 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Proposed patch (9.32 KB, patch)
2006-06-02 00:42 UTC, Andrew Walker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan Hanson 2006-06-01 02:19:37 UTC
Version:            (using KDE KDE 3.5.0)
Installed from:    Compiled From Sources
OS:                Linux

STEPS TO REPRODUCE:

1) draw a horizontal line from left to right (using the maintain aspect modifier helps).
2) drag the right endpoint of the line across the left edge of the view window, holding the aspect modifier key down.

Kst crashes.
Comment 1 Andrew Walker 2006-06-02 00:42:37 UTC
Created attachment 16423 [details]
Proposed patch

Fxies this bug and also cleans up several other problems related to lines
maintaining their aspecr ratio when resizing with the SHIFT key pressed.
Comment 2 Andrew Walker 2006-06-02 21:33:44 UTC
Do we want this for the upcoming release or should I add it to the branch?
Comment 3 George Staikos 2006-06-03 17:25:28 UTC
  Anything that builds on the "aspect ratio preservation" code needs to stay 
in that branch... it doesn't make sense elsewhere.  I wonder why lines and 
arrows have this at all though.  It goes entirely against the purpose of 
lines and arrows.  They're supposed to stretch and move along with plots and 
labels, and they're supposed to do this relative to the contents rect of the 
endpoints parents.  I think this is making everything very complicated for no 
tangible benefit.

  Right now, the only object I can think of that should be preserving it's 
ratios is picture, with a "possibly" for boxes and ellipses.  (Does anyone 
even use ellipse?)

  In practice, the only boxes I've ever seen used have been to contain plots 
and labels, which means preserving aspect ratio would result in the wrong 
thing.
Comment 4 Andrew Walker 2006-06-03 17:43:33 UTC
The patch was written to fix the bug. As a result it also correctly preserves aspect ratio if the user resizes holding the shift key down. Do we want to ship with a known crash?
Comment 5 George Staikos 2006-06-03 17:49:43 UTC
  None of that code is in trunk as far as I can see.
Comment 6 Andrew Walker 2006-06-03 17:51:22 UTC
I'm not sure what that means. The crash is in trunk.
Comment 7 George Staikos 2006-06-03 17:53:27 UTC
  Doesn't crash for me.
Comment 8 George Staikos 2006-06-03 17:59:22 UTC
  Ah now I see it.  I'm tempted to back out more of this code as opposed to 
pile in more complications.  I really don't like this.  (And it's simply a 
divide by zero so the fix should be very simple.)
Comment 9 Netterfield 2006-06-03 18:34:22 UTC
This is part of the mouse UI only.  Once the modifier is released, the object 
can re-scale as before.

- can anyone think of when one might want this?  Personally, I have neither 
known it existed, nor wanted it.

That being said, we shouldn't ship with a know crash.

On Saturday 03 June 2006 08:30, George Staikos wrote:
[bugs.kde.org quoted mail]
Comment 10 George Staikos 2006-06-03 18:42:51 UTC
Seems like this is all that's needed.  I'd rather not see more changes than 
necessary.  The existence of this code and crash alone are proof of the value 
of doing minimal changes.

@@ -861,7 +861,7 @@
       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)) {
+        if (absAspect < 500 && (double(abs((pos.y() - fromPoint.y())/(pos.x() 
- fromPoint.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());
Comment 11 George Staikos 2006-06-03 18:51:11 UTC
On Saturday 03 June 2006 12:34, netterfield@astro.utoronto.ca wrote:
> - can anyone think of when one might want this?  Personally, I have neither
> known it existed, nor wanted it.


  I really would like to see all this mouse handling re-simplified.  It seems 
to have grown without vision for the past year and it's horribly 
overcomplicated now.  It's not surprising that random bugs are popping up in 
there.  Simplifying means looking at the big picture again, and also removing 
questionable features.
Comment 12 George Staikos 2006-06-07 09:56:45 UTC
SVN commit 549034 by staikos:

fix apparent C&P bug
BUG: 128399


 M  +1 -1      ksttoplevelview.cpp  


--- trunk/extragear/graphics/kst/src/libkstapp/ksttoplevelview.cpp #549033:549034
@@ -861,7 +861,7 @@
       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)) {
+        if (absAspect < 500 && (double(abs((pos.y() - fromPoint.y())/(pos.x() - fromPoint.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());