Bug 314415 - Crash on moving cursor down via j button
Summary: Crash on moving cursor down via j button
Status: RESOLVED FIXED
Alias: None
Product: kate
Classification: Applications
Component: Vi Input Mode (show other bugs)
Version: Git
Platform: Other Linux
: NOR crash
Target Milestone: ---
Assignee: Simon St James
URL:
Keywords:
: 318781 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-02-04 12:32 UTC by Vishesh Handa
Modified: 2013-08-12 13:00 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Backtrace (44.27 KB, text/x-log)
2013-02-04 12:34 UTC, Vishesh Handa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vishesh Handa 2013-02-04 12:32:23 UTC
I have file open in KDevelop (nepomuk/core/services/fileindexer/indexscheduler.cpp). On pressing the 'j' button in order to move down one line it crashes. This seems to happen on any part of the file.

Reproducible: Always

Steps to Reproduce:
1. Open the nepomuk-core project in Kdevelop
2. Go to the indexscheduler file
3. Press 'j' when in normal mode
Actual Results:  
Does not crash

Expected Results:  
It crashed!

Surprisingly, the crash window doesn't seem to open up. I had to run this in gdb manually.

Kate git head: -

commit 42e0ada05d4cb50f765c3093a1a6ee97fd511530
Author: T.C. Hollingsworth <tchollingsworth@gmail.com>
Date:   Mon Feb 4 02:04:01 2013 -0700

    give KatePart plugin docs a new home
Comment 1 Vishesh Handa 2013-02-04 12:34:42 UTC
Created attachment 76902 [details]
Backtrace

Gdb backtrace.
Comment 2 Vishesh Handa 2013-02-04 13:41:17 UTC
Not being able to use 'j' or 'k' was super annoying, so I decided to debug this a little. I apparently added custom mappings

j -> gj
k -> gk

last night. Those seem to be causing the problem. Removing them fixes it.

I'm guessing that it keeps expanding j -> gj -> ggj -> gggj ... Cause the backtrace seems fairly large and repeating.
Comment 3 Dominik Haumann 2013-02-20 10:52:25 UTC
Yes, there obviously is a recursion.
Comment 4 Fabian 2013-02-22 22:32:57 UTC
I suggest to change this bug in a feature request for nonrecursive mappings then. Also adding some check to prevent a crash due to endless recursion might make sense.
Comment 5 Dominik Haumann 2013-03-23 15:57:09 UTC
@Simon: if you have any ideas how to easily fix that, go ahead :-)
Comment 6 Simon St James 2013-03-23 16:05:22 UTC
What would be the preferred solution, here? Disallowing a recursive mapping to be defined in the first place, or not expanding further mappings when executing a mapping?
Comment 7 Vishesh Handa 2013-03-23 16:38:31 UTC
(In reply to comment #6)
> What would be the preferred solution, here? Disallowing a recursive mapping
> to be defined in the first place, or not expanding further mappings when
> executing a mapping?

Since vim allows recursive mapping, I think it would be best if we also allow recursive mapping.
Comment 8 Dominik Haumann 2013-03-23 18:31:17 UTC
Does vim somehow guard against infinite recursion?
Comment 9 Fabian 2013-03-23 18:59:32 UTC
(In reply to comment #8)
> Does vim somehow guard against infinite recursion?

No, vim doesn't guard against it. Nevertheless, I don't think that this is an argument against doing so, at least as long nobody provides a usecase against doing so.
Comment 10 Vishesh Handa 2013-03-23 19:24:46 UTC
(In reply to comment #9)
> 
> (In reply to comment #8)
> > Does vim somehow guard against infinite recursion?
> 
> No, vim doesn't guard against it. Nevertheless, I don't think that this is
> an argument against doing so, at least as long nobody provides a usecase
> against doing so.

I'm not sure what you mean by this. The mappings in the bug report -
j -> gj 
k -> gk

were imported from my vimrc file and they work perfectly in vim. The use case is to be able to move up / down long lines which have become wrapped. Almost all vim users have this in their .vimrc file. They should be supported in our vim mode.
Comment 11 Fabian 2013-03-23 19:54:13 UTC
(In reply to comment #10)

> I'm not sure what you mean by this. The mappings in the bug report -
> j -> gj 
> k -> gk
> 
> were imported from my vimrc file and they work perfectly in vim. The use
> case is to be able to move up / down long lines which have become wrapped.
> Almost all vim users have this in their .vimrc file. They should be
> supported in our vim mode.
Ah, no that wasn't what I meant exactly. That is supported by vim I meant rather what happens when you use: 1)
:map x y
:map y x
and 2)
:map g wg
However, vim is smarter then vi (whose behaviour I remembered regarding this) and throws E223 in case of 1). 2) is still not handled.
Comment 12 Simon St James 2013-03-24 09:28:47 UTC
I'd like some more clarification on what people would consider to be an acceptable solution, here.  For example, if I had, say,

x -> gj
a -> x

what would you expect the result of typing "a" to be? Move down a visible line? Delete the current character under the cursor? If the former, why would j -> gj be treated differently - because an infinite recursion can be detected?
Comment 13 Fabian 2013-03-25 17:43:26 UTC
(In reply to comment #12) 
> x -> gj
> a -> x
> 
> what would you expect the result of typing "a" to be? Move down a visible
> line? Delete the current character under the cursor? If the former, why
> would j -> gj be treated differently - because an infinite recursion can be
> detected?

I think both make sense, but vim's standard behaviour for map would be the first one. The second one would be the behaviour of noremap in vim. I can only specualate why j -> gj is handled the way it is, e.g. using j -> jj will move down to the bottom in vim (instead of two lines down).
Comment 14 Simon St James 2013-03-26 20:38:15 UTC
So perhaps adding a "Recursive" checkbox column to the table (and the ability to distinguish between recursive and non-recursive mappings in the vimrc importer) and respecting this when executing a mapping is the way forward, here?
Comment 15 Fabian 2013-03-27 13:30:10 UTC
Generally that would help. I still don't know  how vim handles j -> gj, though.
Comment 16 Simon St James 2013-04-11 11:24:22 UTC
Let's take the discussion to here:

https://git.reviewboard.kde.org/r/109962/
Comment 17 Simon St James 2013-04-23 22:24:27 UTC
*** Bug 318781 has been marked as a duplicate of this bug. ***
Comment 18 Simon St James 2013-06-03 12:20:59 UTC
Git commit 738f125aa6dc26fcb8f5c77e90ecd7d9660b11a0 by Simon St James.
Committed on 04/04/2013 at 18:52.
Pushed by sstjames into branch 'master'.

Distinguish between recursive and non-recursive mappings: user can choose whether a mapping is recursive or not via the new "Recursive?" column added to the mappings table.  Mappings are recursive by default.  Mappings from configs before the introduction of this distinction are treated as recursive.

The main motivator for this change is so that the mapping of gj -> j - very commonly used by Vim users - can be used without a stack overflow :)

REVIEW:109962

M  +30   -6    part/dialogs/katedialogs.cpp
M  +6    -1    part/dialogs/viinputmodeconfigwidget.ui
M  +4    -3    part/utils/katecmds.cpp
M  +26   -2    part/vimode/kateviglobal.cpp
M  +4    -1    part/vimode/kateviglobal.h
M  +2    -1    part/vimode/kateviinsertmode.h
M  +3    -1    part/vimode/katevimodebase.h
M  +18   -7    part/vimode/katevinormalmode.cpp
M  +4    -2    part/vimode/katevinormalmode.h
M  +2    -1    part/vimode/katevireplacemode.h
M  +72   -16   tests/vimode_test.cpp

http://commits.kde.org/kate/738f125aa6dc26fcb8f5c77e90ecd7d9660b11a0
Comment 19 Dominik Haumann 2013-08-12 12:50:17 UTC
Simon: From the commit above, can this report be closed?
Comment 20 Simon St James 2013-08-12 13:00:38 UTC
Dominik: I think so - if anyone disagrees, please re-open, explaining reasons :)