Bug 133937 - select + word-left/right incorrectly changes selection start
Summary: select + word-left/right incorrectly changes selection start
Status: CLOSED FIXED
Alias: None
Product: kate
Classification: Applications
Component: part (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Matthew Woehlke
URL:
Keywords:
: 146673 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-09-11 23:35 UTC by Matthew Woehlke
Modified: 2007-06-12 16:55 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Woehlke 2006-09-11 23:35:14 UTC
Version:            (using KDE KDE 3.4.3)
Installed from:    Compiled From Sources
Compiler:          gcc version 3.4.3 20050227 (Red Hat 3.4.3-22.fc3) 
OS:                Linux

Under certain conditions, shift (select) plus move-word (ctrl-<arrows>) can change the selection start.

To reproduce ([]'s used to show selection, | to show cursor):
1. Place some text on the clipboard (I'll use "bar")
2. In a blank document, type:
foo foo     */
3. Double-click to select the second 'foo':
foo [foo]|     */
4. Paste (ctrl-v by default):
foo bar|     */
5. Press ctrl+shift+right:
foo bar[     */]|
6. Press ctrl+shift+left:
foo |bar[     */]
7. Press ctrl+shift+right:
foo [bar     ]|*/

The selection at (5) is wrong (should be 'foo bar [     ]*/'; also see below) but from other reports this might be fixed in newer KATE. The selection at (6) is also wrong (should be 'foo |[bar]     */'); this seems to be where the problem starts. Finally, by (7) the selection start has changed from between 'bar' and the following spaces to before 'bar'; this is the bug. (3) seems to be the key step; if I drag a selection or select with the keyboard, it works correctly (except for the additional problem below).


There is an additional issue (not sure if it is bug or design, but if design, it feels like BAD design), where the selection or lack thereof changes the behavior of ctrl-<arrow>, as follows:
1. (Start):    foo bar|  */
2. ctrl+right: foo bar[  */]|
3. ctrl+left:  foo |[bar]  */
4. ctrl+right: foo bar[  ]|*/

(2) and (4) should both produce the same result (which IMO should be (4), but as above maybe this is fixed?).
Comment 1 Matthew Woehlke 2006-09-13 01:28:00 UTC
I can also reproduce this by typing new text instead of pasting (step 4).
Comment 2 Matthew Woehlke 2006-11-08 20:34:49 UTC
Hello? Is anyone able to reproduce this?
Comment 3 Anders Lund 2006-11-08 21:02:22 UTC
On Wednesday 08 November 2006 20:34, Matthew Woehlke wrote:
> Hello? Is anyone able to reproduce this?

Not me.
And I disagree. At least the current version of kate, which does not work 
exactly as you describe, is very consistent, but not working like you 
suggest.

Ctrl + arrow moves the selection end one word in the desired correction, so 
that the next word becomes selected, or deselected if the direction is from 
the selection end towards the selection start.
Comment 4 Dominik Haumann 2006-11-09 09:34:50 UTC
I can kind of reproduce. The selection is extended to the left, although you only go to the right. I'm not sure if that's how it is supposed to work.
Comment 5 Matthew Woehlke 2006-11-09 17:14:27 UTC
Do you mind giving me a complete description (like what I did) of what you are seeing? I'd be very interested in comparing notes.

Anyway, if you have any ideas, I'd be more than happy to test patches.

I should add that in 3.5.5, 5 and 6 are now:
5: foo bar[     ]|*/
6: foo |bar[     ]*/

...and the selection at 6 is clearly wrong; the cursor is completely outside the selection, which AFAIK is not supposed to be possible.

Actually, I think I know what the problem is... when you double-click to select a word, the cursor position becomes decoupled from both the selection start and end. While I can see where this *might* have been an intentional design, it causes behavior that is at best described as "strange". To demonstrate, type a long word surrounded by other text (just so you have somewhere to move the cursor to), and double-click it in the middle. The selection start and end become the word, while the cursor is in the middle of the word. From here, what happens as you shift-arrow off of the word always leaves the word selected. Again, while I can see this as being "desirable", it is not intuitive (I would describe it as "strange, at best") and is inconsistent with how double-click selection works in other circumstances (QLineEdit, for example, or this firefox text box I am typing in), and is something I have never found useful. Having discovered this, I suspect it is the cause of this bug, which crops up often enough to be quite irritating.

What SHOULD be happening here (and does anywhere outside of KATE AFAICT) is that double-clicking sets the cursor - equal to the selection end - to the end of the word, rather than the click location. If you then shift-left from here, you will wind up with only part of the word selected.
Comment 6 Anders Lund 2006-11-09 17:45:00 UTC
Uhm, I see that you are in fact correct, this does happen.

The problem is during the paste. Paste clears the selection, but not the 
selectAnchor property used internally i guess, this should not be too hard to 
fix.
Comment 7 Dominik Haumann 2006-11-09 18:03:41 UTC
> Actually, I think I know what the problem is... when you double-click to 
> select a word, the cursor position becomes decoupled from both the selection 
> start and end. While I can see where this *might* have been an intentional 
> design, it causes behavior that is at best described as "strange". To 
> demonstrate, type a long word surrounded by other text (just so you have 
> somewhere to move the cursor to), and double-click it in the middle.

double click -> now you are in the select-whole-words-only mode

> The selection start and end become the word, while the cursor is in the
> middle of the word. From here, what happens as you shift-arrow off of the
> word always leaves the word selected. Again, while I can see this as 
> being "desirable", it is not intuitive (I would describe it as "strange, at
> best") and is inconsistent with how double-click selection works in other 
> circumstances (QLineEdit, for example, or this firefox text box I am typing 
> in), and is something I have never found useful. Having discovered this, I 
> suspect it is the cause of this bug, which crops up often enough to be quite 
> irritating.

Our standard documents state that double clicking results in the select-words-only mode. see http://developer.kde.org/documentation/standards/kde/style/mouse/selection.html
Maybe using the keyboard afterwards should behave differently (as you are not using the mouse then).

> What SHOULD be happening here (and does anywhere outside of KATE AFAICT) is
> that double-clicking sets the cursor - equal to the selection end - to the 
> end of the word, rather than the click location. If you then shift-left from 
> here, you will wind up with only part of the word selected. 

This was changed to fix bug #106402.
Comment 8 Matthew Woehlke 2006-11-09 22:02:35 UTC
(sorry if this double-posts, after a few hours I'm giving up on <id> AT bugs-kde-org actually posting...)

> double click -> now you are in the select-whole-words-only mode
> [snip]
> Our standard documents state that double clicking results in the
> select-words-only mode. see
> http://developer.kde.org/documentation/standards/kde/style/mouse/selection.html
> Maybe using the keyboard afterwards should behave differently (as you are not
> using the mouse then).

Yes and no. There is no reason 'select-words-only' mode needs to decouple the cursor from the selection-end. What IMO should happen is the cursor follows the selection end rather than the position of the mouse click. Again, this is consistent with other editors, and would result in no further problems once you release the mouse button (which should end words-only selection mode also). It looks like WOSM ends when it should, except that the select start/end are already broken.

> This was changed to fix bug #106402.

I don't see how that precludes making select work as I describe, i.e. the way it works in /every/other/editor/.

Here is what I think should happen for successive clicks:
1. clears selection, sets cursor to position of click
2. selects word under cursor; cursor is at right of word (selection end)
3. selects entire line; cursor still equal to selection end, at end of line
4. Same as 2

...and I'm not sure what should happen at 5 and beyond; behavior right now seems to be "touchy" (i.e. really sensitive to speed and timing), but I think at this point all that matters is not breaking anything.

I tried playing with this but didn't get far (fixing the cursor placement when multi-clicking is easy, but unfortunately doesn't fix any of the weirdness). I still think what needs to happen is that this weird 'preserving the selected word' stuff needs to go away. Double-clicking should set the select start and end, and set the cursor as described. After that, the select start should remain constant and the select end should stay glued to the cursor.
Comment 9 Matthew Woehlke 2007-02-05 21:48:24 UTC
Replying to comment #6: I (finally) figured out what Anders is talking about with the original report. Now that I'm starting to understand how KATE handles selections, I agree in principle, although I'm not sure it is selectAnchor (isn't this *always* set?) as opposed to sel*Cached that is not purged correctly. Especially since I believe that they *do* need to be purged on paste, and I didn't yet spot where this should be (and I suspect, isn't) happening.

I also finally found how to kill the KATE-unique behavior talked about in comment #8, and it doesn't fix the bug, so comment #8 is talking about a different issue.
Comment 10 Anders Lund 2007-02-05 22:09:41 UTC
Ok, lets fight this until we get to a point where we are hopefully all good :)
Comment 11 Anders Lund 2007-02-05 22:25:16 UTC
hm, i didn't mean to do that, let's keep it on the list!
Comment 12 Matthew Woehlke 2007-02-05 22:28:55 UTC
Indeed. We will do that (making this comment for the record).
Comment 13 Matthew Woehlke 2007-02-13 18:26:26 UTC
SVN commit 633268 by mwoehlke:

BUG: 133937

Update selection after paste. Also, don't preserve the word from a double-click selection when selecting with the keyboard.


 M  +7 -0      kateview.cpp  
 M  +1 -1      kateview.h  
 M  +7 -13     kateviewinternal.cpp  


--- branches/KDE/3.5/kdelibs/kate/part/kateview.cpp #633267:633268
@@ -1592,6 +1592,13 @@
   setSelection (cursor.line(), start, cursor.line(), end);
 }
 
+void KateView::paste()
+{
+  m_doc->paste( this );
+  emit selectionChanged();
+  m_viewInternal->repaint();
+}
+
 void KateView::cut()
 {
   if (!hasSelection())
--- branches/KDE/3.5/kdelibs/kate/part/kateview.h #633267:633268
@@ -83,7 +83,7 @@
   //
   public slots:
     // TODO: Factor out of m_viewInternal
-    void paste()         {  m_doc->paste( this ); m_viewInternal->repaint(); }
+    void paste();
     void cut();
     void copy() const;
 
--- branches/KDE/3.5/kdelibs/kate/part/kateviewinternal.cpp #633267:633268
@@ -2097,23 +2097,13 @@
           else // same line, ignore
             doSelect = false;
         break;
-        default: // *allways* keep original selection for mouse
+        default:
         {
           if ( selStartCached.line() < 0 ) // invalid
             break;
 
-          if ( newCursor.line() > selEndCached.line() ||
-               ( newCursor.line() == selEndCached.line() &&
-                 newCursor.col() > selEndCached.col() ) )
-            selectAnchor = selStartCached;
-
-          else if ( newCursor.line() < selStartCached.line() ||
-               ( newCursor.line() == selStartCached.line() &&
-                 newCursor.col() < selStartCached.col() ) )
-            selectAnchor = selEndCached;
-
-          else
-            doSelect = false;
+          selectAnchor = selStartCached;
+          doSelect = true;
         }
 //         break;
       }
@@ -3264,7 +3254,11 @@
 void KateViewInternal::viewSelectionChanged ()
 {
   if (!m_view->hasSelection())
+  {
     selectAnchor.setPos (-1, -1);
+    selEndCached = selectAnchor;
+    selStartCached = cursor;
+  }
 }
 
 //BEGIN IM INPUT STUFF
Comment 14 Matthew Woehlke 2007-04-18 17:59:03 UTC
SVN commit 655507 by mwoehlke:

CCBUG: 133937
Fixing 133937 broke mouse selection which *should* preserve the selected word. So restore the behavior removed by r633268, but make it conditional on mouse selection (new selection mode) so that both will Do The Right Thing.


 M  +21 -3     kateviewinternal.cpp  
 M  +2 -2      kateviewinternal.h  


--- branches/KDE/3.5/kdelibs/kate/part/kateviewinternal.cpp #655506:655507
@@ -2115,14 +2115,24 @@
           else // same line, ignore
             doSelect = false;
         break;
+        case Mouse:
+        {
+          if ( selStartCached.line() < 0 ) // invalid
+            break;
+
+          if ( newCursor > selEndCached )
+            selectAnchor = selStartCached;
+          else if ( newCursor < selStartCached )
+            selectAnchor = selEndCached;
+          else
+            doSelect = false;
+        }
+        break;
         default:
         {
           if ( selectAnchor.line() < 0 ) // invalid
             break;
-
-          doSelect = true;
         }
-//         break;
       }
 
       if ( doSelect )
@@ -2687,6 +2697,10 @@
           e->accept ();
           return;
         }
+        else if (m_selectionMode == Default)
+        {
+          m_selectionMode = Mouse;
+        }
 
         if ( e->state() & Qt::ShiftButton )
         {
@@ -2802,6 +2816,10 @@
 
         selStartCached = m_view->selectStart;
         selEndCached = m_view->selectEnd;
+        // FIXME this isn't right in the case of a shift+DC
+        // for shift+DC, we might want to use selStartCached!!
+        // This also breaks shift+TC because it nukes the cursor
+        // order for when the TC handler does its thing. Oops.
         updateCursor( selEndCached );
       }
 
--- branches/KDE/3.5/kdelibs/kate/part/kateviewinternal.h #655506:655507
@@ -273,7 +273,7 @@
     bool m_selChangedByUser;
     KateTextCursor selectAnchor;
 
-    enum SelectionMode { Default=0, Word, Line }; ///< for drag selection. @since 2.3
+    enum SelectionMode { Default=0, Word, Line, Mouse }; ///< for drag selection. @since 2.3
     uint m_selectionMode;
     // when drag selecting after double/triple click, keep the initial selected
     // word/line independant of direction.
@@ -288,7 +288,7 @@
 
     // maximal lenght of textlines visible from given startLine
     int maxLen(uint startLine);
-    
+
     // are we allowed to scroll columns?
     bool columnScrollingPossible ();
 
Comment 15 Matthew Woehlke 2007-06-12 16:55:52 UTC
*** Bug 146673 has been marked as a duplicate of this bug. ***