Bug 56337 - ctrl+left and ctrl+right skip over punctuation "words"
Summary: ctrl+left and ctrl+right skip over punctuation "words"
Status: RESOLVED FIXED
Alias: None
Product: kate
Classification: Applications
Component: general (show other bugs)
Version: 2.1
Platform: OpenSUSE Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2003-03-24 19:42 UTC by hughjonesd
Modified: 2006-06-13 22:01 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
makes wordLeft() and wordRight() behavior symmetric (6.07 KB, patch)
2006-06-03 15:25 UTC, Johannes Sixt
Details

Note You need to log in before you can comment on or make changes to this bug.
Description hughjonesd 2003-03-24 19:42:35 UTC
Version:           2.1 (using KDE 3.1.0)
Installed from:    SuSE
Compiler:          gcc version 2.95.3 20010315 (SuSE)
OS:          Linux (i686) release 2.4.18-4GB

Try editing this lines:
foo => bar 
(with spaces between foo, => and bar)

Move left and right using the Ctrl+left/right arrow shortcuts.

You will notice that moving left, "foo =>" is treated as one word, and moving right "=> bar" is treated as one word.

This has two problems:
1. it's not consistent with the Qt editing widgets which treat all 3 items as separate words
2. it means that Ctrl+left arrow and ctrl+right arrow aren't consistent with each other: they don't always stop at the same places.

Ta!

Dave
Comment 1 Anders Lund 2003-03-29 08:41:09 UTC
I agree, I think we should stop at anything matching the \b regexp assertion. Should we 
make it configurable? 
Comment 2 hughjonesd 2003-03-31 11:46:46 UTC
I don't think this requires a configuration option, in fact configuring how
Ctrl+arrows work would definitely fall into the realms of "geeky
hyperconfigurability" in my book! The most important thing is consistency
between moving left and right, followed by consistency with the rest of KDE, IMHO.
Comment 3 Christoph Cullmann 2003-04-02 00:23:34 UTC
yeah, please change it to the \b thingy without config option, that is what the rest of 
kde/qt is used to do, should be fine for us, too  
Comment 4 Anders Lund 2003-04-11 23:23:33 UTC
works in CVS 
Comment 5 Johannes Sixt 2005-06-21 21:36:22 UTC
This no longer works as advertised in SUSE 9.3 (KDE 3.4.0, Kate 2.4). Consider again:

foo => bar

Starting right before foo, the cursor jumps forward right before =>, then before bar, then to the end of the line. But backwards it jumps before bar and then immediately before foo, and not => as I would expect.

-- Hannes
Comment 6 Johannes Sixt 2006-06-03 15:25:07 UTC
Created attachment 16444 [details]
makes wordLeft() and wordRight() behavior symmetric

This patch fixes that word moves forward and backward (Ctrl-Left and
Ctrl-Right) are not exactly symmetric, i.e. they do not stop at exactly the
same points.

The previous implementation unifies wordLeft() and wordRight() into a single
function wordMove(), but the problem is that the move operations are such
that they must be implemented assymetrically, and cannot really be unified.
The patch does right this: It splits the implementation into two: one for
wordLeft() and wordRight() each.

The behavior with this patch is this:

wordRight():
- If the cursor is on a word character, the word and space following it is
skipped.
- If the cursor is on a non-word character, all following non-word characters
excluding space are skipped, then all space following it is skipped.
- If the cursor is at the end of the line, all space at the beginning of the
next line is skipped. It does not skip forward for more than one line.

wordLeft():
All space before the cursor is skipped. Then:
- If there is a word character before the cursor, it is skipped.
- If there is a non-word character before the cursor, all non-word characters
excluding space are skipped.
- If the cursor it is at the beginning of the line, it is moved to the end of
the previous line.

This behavior is now symmetric. It _is_ different from the word move
operations of some other edit widgets (like, for example, KMail's composer),
but it is more appropriate for a programmer's editor, because it stops at all
punctuation.
Comment 7 Dominik Haumann 2006-06-03 16:04:30 UTC
Imho the suggested patch is fine.
Christoph, Anders, Hamish? :)
Comment 8 Anders Lund 2006-06-03 21:52:12 UTC
FWIW, in my opinion that is very desirable ;)

-anders
Comment 9 Leo Savernik 2006-06-06 15:54:36 UTC
Am Samstag, 3. Juni 2006 15:25 schrieb Johannes Sixt:
> The behavior with this patch is this:


Just a small question for clarification (not having tested the patch).

On the snippet below where | denotes the position of the caret, does 
sequentially pressing of Ctrl+Right result in the following movement?

1. |some.identifier, .some.other
2. some.|identifier, .some.other
3. some.identifier, |.some.other
4. some.identifier, .some.|other
5. some.identifier, .some.other|

which I think is good enough for not moving too slowly and not getting too 
much in your way.

Or does it result in
1. |some.identifier, .some.other
2. some.identifier, |.some.other
3. some.identifier, .some.other|
4. some.identifier, .some.other

or does it result in
1. |some.identifier, .some.other
2. some|.identifier, .some.other
3. some.|identifier, .some.other
4. some.identifier|, .some.other
5. some.identifier,| .some.other
6. some.identifier, |.some.other
7. some.identifier, .|some.other
8. some.identifier, .some|.other
etc.
Comment 10 Johannes Sixt 2006-06-07 20:35:33 UTC
It works like this:

1. |some.identi_fier, some-> other
2. some|.identi_fier, some-> other (*)
3. some.|identi_fier, some-> other
4. some.identi_fier|, some-> other (*)
5. some.identi_fier, |some-> other
6. some.identi_fier, some|-> other (*)
7. some.identi_fier, some-> |other
8. some.identi_fier, some-> other|

Currently, Ctrl-Right does right this, but Ctrl-Left skips these (*). The patch fixes that.
Comment 11 Johannes Sixt 2006-06-10 13:35:29 UTC
I'd like to commit this on the 3.5 branch (and later on the trunk). Any objections?
Comment 12 Dominik Haumann 2006-06-10 14:16:14 UTC
If you tested it well, then I have none ;)
Comment 13 Johannes Sixt 2006-06-10 23:21:15 UTC
SVN commit 550059 by jsixt:

Make wordLeft() and wordRight() behavior symmetric.

Consider this line:

  if (num >= 0) num--; else return;

The points where the cursor stops are (indicated by |):

  |if |(|num |>= |0|) |num|--; |else |return|;|

with both Ctrl-Left and Ctrl-Right.

This behavior _is_ different from the word move operations of some other
edit widgets (like, for example, KMail's composer), but it is more
appropriate for a programmer's editor, because it stops at all punctuation.
BUG: 56337


 M  +70 -23    kateviewinternal.cpp  
 M  +0 -1      kateviewinternal.h  


--- branches/KDE/3.5/kdelibs/kate/part/kateviewinternal.cpp #550058:550059
@@ -1123,43 +1123,90 @@
   }
 }
 
-void KateViewInternal::moveWord( Bias bias, bool sel )
+void KateViewInternal::wordLeft ( bool sel )
 {
-  // This matches the word-moving in QTextEdit, QLineEdit etc.
-
   WrappingCursor c( this, cursor );
-  if( !c.atEdge( bias ) ) {
-    KateHighlighting* h = m_doc->highlight();
 
-    bool moved = false;
-    while( !c.atEdge( bias ) && !h->isInWord( m_doc->textLine( c.line() )[ c.col() - (bias == left ? 1 : 0) ] ) )
+  // First we skip backwards all space.
+  // Then we look up into which category the current position falls:
+  // 1. a "word" character
+  // 2. a "non-word" character (except space)
+  // 3. the beginning of the line
+  // and skip all preceding characters that fall into this class.
+  // The code assumes that space is never part of the word character class.
+
+  KateHighlighting* h = m_doc->highlight();
+  if( !c.atEdge( left ) ) {
+
+    while( !c.atEdge( left ) && m_doc->textLine( c.line() )[ c.col() - 1 ].isSpace() )
+      --c;
+  }
+  if( c.atEdge( left ) )
+  {
+    --c;
+  }
+  else if( h->isInWord( m_doc->textLine( c.line() )[ c.col() - 1 ] ) )
+  {
+    while( !c.atEdge( left ) && h->isInWord( m_doc->textLine( c.line() )[ c.col() - 1 ] ) )
+      --c;
+  }
+  else
+  {
+    while( !c.atEdge( left )
+           && !h->isInWord( m_doc->textLine( c.line() )[ c.col() - 1 ] )
+           // in order to stay symmetric to wordLeft()
+           // we must not skip space preceding a non-word sequence
+           && !m_doc->textLine( c.line() )[ c.col() - 1 ].isSpace() )
     {
-      c += bias;
-      moved = true;
+      --c;
     }
+  }
 
-    if ( bias != right || !moved )
+  updateSelection( c, sel );
+  updateCursor( c );
+}
+
+void KateViewInternal::wordRight( bool sel )
+{
+  WrappingCursor c( this, cursor );
+
+  // We look up into which category the current position falls:
+  // 1. a "word" character
+  // 2. a "non-word" character (except space)
+  // 3. the end of the line
+  // and skip all following characters that fall into this class.
+  // If the skipped characters are followed by space, we skip that too.
+  // The code assumes that space is never part of the word character class.
+
+  KateHighlighting* h = m_doc->highlight();
+  if( c.atEdge( right ) )
+  {
+    ++c;
+  }
+  else if( h->isInWord( m_doc->textLine( c.line() )[ c.col() ] ) )
+  {
+    while( !c.atEdge( right ) && h->isInWord( m_doc->textLine( c.line() )[ c.col() ] ) )
+      ++c;
+  }
+  else
+  {
+    while( !c.atEdge( right )
+           && !h->isInWord( m_doc->textLine( c.line() )[ c.col() ] )
+           // we must not skip space, because if that space is followed
+           // by more non-word characters, we would skip them, too
+           && !m_doc->textLine( c.line() )[ c.col() ].isSpace() )
     {
-      while( !c.atEdge( bias ) &&  h->isInWord( m_doc->textLine( c.line() )[ c.col() - (bias == left ? 1 : 0) ] ) )
-        c += bias;
-      if ( bias == right )
-      {
-        while ( !c.atEdge( bias ) && m_doc->textLine( c.line() )[ c.col() ].isSpace() )
-          c+= bias;
-      }
+      ++c;
     }
-
-  } else {
-    c += bias;
   }
 
+  while( !c.atEdge( right ) && m_doc->textLine( c.line() )[ c.col() ].isSpace() )
+    ++c;
+
   updateSelection( c, sel );
   updateCursor( c );
 }
 
-void KateViewInternal::wordLeft ( bool sel ) { moveWord( left,  sel ); }
-void KateViewInternal::wordRight( bool sel ) { moveWord( right, sel ); }
-
 void KateViewInternal::moveEdge( Bias bias, bool sel )
 {
   BoundedCursor c( this, cursor );
--- branches/KDE/3.5/kdelibs/kate/part/kateviewinternal.h #550058:550059
@@ -190,7 +190,6 @@
 
   private:
     void moveChar( Bias bias, bool sel );
-    void moveWord( Bias bias, bool sel );
     void moveEdge( Bias bias, bool sel );
     KateTextCursor maxStartPos(bool changed = false);
     void scrollPos(KateTextCursor& c, bool force = false, bool calledExternally = false);
Comment 14 Johannes Sixt 2006-06-13 22:01:18 UTC
SVN commit 551148 by jsixt:

Forward-port r550059: Make wordLeft() and wordRight() behavior symmetric.
CCBUG: 56337


 M  +70 -23    kateviewinternal.cpp  
 M  +0 -1      kateviewinternal.h  


--- trunk/KDE/kdelibs/kate/part/kateviewinternal.cpp #551147:551148
@@ -1013,43 +1013,90 @@
   }
 }
 
-void KateViewInternal::moveWord( KateViewInternal::Bias bias, bool sel )
+void KateViewInternal::wordLeft ( bool sel )
 {
-  // This matches the word-moving in QTextEdit, QLineEdit etc.
-
   WrappingCursor c( this, m_cursor );
-  if( !c.atEdge( bias ) ) {
-    KateHighlighting* h = m_doc->highlight();
 
-    bool moved = false;
-    while( !c.atEdge( bias ) && !h->isInWord( m_doc->line( c.line() )[ c.column() - (bias == left ? 1 : 0) ] ) )
+  // First we skip backwards all space.
+  // Then we look up into which category the current position falls:
+  // 1. a "word" character
+  // 2. a "non-word" character (except space)
+  // 3. the beginning of the line
+  // and skip all preceding characters that fall into this class.
+  // The code assumes that space is never part of the word character class.
+
+  KateHighlighting* h = m_doc->highlight();
+  if( !c.atEdge( left ) ) {
+
+    while( !c.atEdge( left ) && m_doc->line( c.line() )[ c.column() - 1 ].isSpace() )
+      --c;
+  }
+  if( c.atEdge( left ) )
+  {
+    --c;
+  }
+  else if( h->isInWord( m_doc->line( c.line() )[ c.column() - 1 ] ) )
+  {
+    while( !c.atEdge( left ) && h->isInWord( m_doc->line( c.line() )[ c.column() - 1 ] ) )
+      --c;
+  }
+  else
+  {
+    while( !c.atEdge( left )
+           && !h->isInWord( m_doc->line( c.line() )[ c.column() - 1 ] )
+           // in order to stay symmetric to wordLeft()
+           // we must not skip space preceding a non-word sequence
+           && !m_doc->line( c.line() )[ c.column() - 1 ].isSpace() )
     {
-      c += bias;
-      moved = true;
+      --c;
     }
+  }
 
-    if ( bias != right || !moved )
+  updateSelection( c, sel );
+  updateCursor( c );
+}
+
+void KateViewInternal::wordRight( bool sel )
+{
+  WrappingCursor c( this, m_cursor );
+
+  // We look up into which category the current position falls:
+  // 1. a "word" character
+  // 2. a "non-word" character (except space)
+  // 3. the end of the line
+  // and skip all following characters that fall into this class.
+  // If the skipped characters are followed by space, we skip that too.
+  // The code assumes that space is never part of the word character class.
+
+  KateHighlighting* h = m_doc->highlight();
+  if( c.atEdge( right ) )
+  {
+    ++c;
+  }
+  else if( h->isInWord( m_doc->line( c.line() )[ c.column() ] ) )
+  {
+    while( !c.atEdge( right ) && h->isInWord( m_doc->line( c.line() )[ c.column() ] ) )
+      ++c;
+  }
+  else
+  {
+    while( !c.atEdge( right )
+           && !h->isInWord( m_doc->line( c.line() )[ c.column() ] )
+           // we must not skip space, because if that space is followed
+           // by more non-word characters, we would skip them, too
+           && !m_doc->line( c.line() )[ c.column() ].isSpace() )
     {
-      while( !c.atEdge( bias ) &&  h->isInWord( m_doc->line( c.line() )[ c.column() - (bias == left ? 1 : 0) ] ) )
-        c += bias;
-      if ( bias == right )
-      {
-        while ( !c.atEdge( bias ) && m_doc->line( c.line() )[ c.column() ].isSpace() )
-          c+= bias;
-      }
+      ++c;
     }
-
-  } else {
-    c += bias;
   }
 
+  while( !c.atEdge( right ) && m_doc->line( c.line() )[ c.column() ].isSpace() )
+    ++c;
+
   updateSelection( c, sel );
   updateCursor( c );
 }
 
-void KateViewInternal::wordLeft ( bool sel ) { moveWord( left,  sel ); }
-void KateViewInternal::wordRight( bool sel ) { moveWord( right, sel ); }
-
 void KateViewInternal::moveEdge( KateViewInternal::Bias bias, bool sel )
 {
   BoundedCursor c( this, m_cursor );
--- trunk/KDE/kdelibs/kate/part/kateviewinternal.h #551147:551148
@@ -215,7 +215,6 @@
 
   private:
     void moveChar( Bias bias, bool sel );
-    void moveWord( Bias bias, bool sel );
     void moveEdge( Bias bias, bool sel );
     KTextEditor::Cursor maxStartPos(bool changed = false);
     void scrollPos(KTextEditor::Cursor& c, bool force = false, bool calledExternally = false);