Supporting nested brackets for Kate autobrackets
AbandonedPublic

Authored by cullmann on Apr 17 2018, 6:55 PM.

Details

Reviewers
dhaumann
brauch
sraizada
Group Reviewers
KTextEditor
Kate
Summary

With autobrackets enabled, Kate adds a closing bracket when opening a bracket. E.g. typing "if(" will result in "if(|)" appearing on screen, where | is the cursor position. Then, if the closing ) is manually typed in, it gets 'eaten' by the autogenerated one. So starting from "if(|)", typing in ")" will give "if()".

However, this fails when any types of brackets are nested. Typing in "if({})" results in the innermost "}" getting eaten, but the closing ")" is not eaten, so the resulting text is "if({}))". This is for any combination of same/different brackets or quotes, as long as they are nested.

The patch uses a QStack<QChar> (instead of just a QChar) to keep track of this and correctly eat closing brackets. That behaviour does work correctly (see attached video - only ({["' and their closing pairs are typed, the cursor is not manually moved). However, the stack should be reset when the cursor is moved away from the area where brackets are being inserted. I have not been able to figure out how to get that to work properly.

On line 1357, upon entering an open bracket/quote, m_currentAutobraceRange seems to get set to (cursorposition-1, insertedTextPosition). Or at least that's what I think, my C++ knowledge doesn't really go that far, and I'm not at all familiar with the codebase or program structure. This needs to take into account nesting, instead of just setting the range to cursor position +- 1 unit for the bracket.

Sven posted on the kwrite-devel mailing list https://mail.kde.org/pipermail/kwrite-devel/2018-April/000346.html that there are methods called to move the cursor left or right, as well as a cursorPositionChanged event. Hooking into this would allow for a basic solution, where moving the cursor at all would clear the autobracket history and result in future autobrackets not getting eaten. However, as long as Backspace and Enter events don't cause the stack to get cleared, that should work for cases when one is just typing a statement that contains multiple levels of parentheses in one shot without any errors. I'm a bit busy right now, but will try to see if I can implement this or a more comprehensive solution within the next few days. I would appreciate any tips on where to look in the code, again this is my first experience with C++ in a long, long time and I'm just putting code in random places until it works :)

Note: in order to get nested autobraces to get eaten with this patch, line 3095 has to be commented out, otherwise the stack is cleared upon entering any type of closing bracket. Making m_currentAutobracketRange fit the actual range instead of one bracket will fix that.

Diff Detail

Repository
R39 KTextEditor
Lint
Lint Skipped
Unit
Unit Tests Skipped
sraizada created this revision.Apr 17 2018, 6:55 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptApr 17 2018, 6:55 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
sraizada requested review of this revision.Apr 17 2018, 6:55 PM
sraizada updated this revision to Diff 34333.EditedMay 16 2018, 10:55 PM

Updated patch, closing brackets now get eaten properly in all cases I tested.

The only issue I found is that matching bracket highlighting can be off when entering 'incorrect' sequences of braces. Such as in this example, where the ) in the middle causes the second-to-last parenthesis to be matched with the first one.

The bracket-eating behaviour with such incorrect sequences is to simple ignore the incorrect ) in the middle - all four of the }})) at the end will get eaten. Out of the three other text editors I tested (IntelliJ, Sublime Text, and 'micro'), Sublime and micro also behave this way.

Restricted Application edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: Frameworks. · View Herald TranscriptMay 16 2018, 10:55 PM

The issue with matching unbalanced braces (picture in previous comment) also happens in unmodified KWrite, so it doesn't seem to be an issue with this revision.
This fails test 49 - kateview_test; but checking out ktexteditor from git and then running make/make install/make test also results in that test failing.
Thus, I believe this revision is ready for review.

I like the idea.
For the patch:

I think one should check with isEmpty() if the thing is empty, instead of the size() > 0 things, thats more clear.
And is there no m_currentAutobraceClosingChar clear missing somewhere? e.g. don't go this two stack out of sync?

cullmann requested changes to this revision.Jul 14 2018, 6:36 PM
This revision now requires changes to proceed.Jul 14 2018, 6:36 PM
cullmann added a subscriber: brauch.

Hi, I like the improvements, just want some feedback on my questions above.
Still alive?

His original post with some more text/hints/questions
https://mail.kde.org/pipermail/kwrite-devel/2018-April/000345.html
He got the advice from @brauch to post here.

For me a little bit hard to follow, not only there but also here.
I could apply this patch almost without trouble. Why some stuff was rejected I have no idea. Just added manually and it seems to work.

So far I understood ask he how to add in some cases the proper cursor positions to the stack, and not only an "approximation". But the noted code lines are not so easy to find for me.

The main issue with this patch is (as he had explained (I think)) that it does not work properly when you move/edit around inside some new created "brace range". To fix this he ask for help.

My feeling is that this patch may cause in the actual condition more hassle than it solves due to the noted missing fixes.

Currently I have not the mood to think more about this, but you smart guys may have no problem to fix the issues :-)

I didn't test it even more but think on it from time to time. If my slightly negative comment was OK or too fast judged. And yes it may.

On the other hand I ask me if the approach to track each key stroke and change the behavior on that record is right or a wrong way.
How about to judge each brace key stroke on the next char? If there is the same brace check if that brace is balanced or not. If it is, just move one position and your are done. This way should it be work always supporting and not only after a recent sequence start.
(?)

cullmann commandeered this revision.Sat, Apr 13, 5:09 PM
cullmann edited reviewers, added: sraizada; removed: cullmann.

We close this in favor of the other patches.

cullmann abandoned this revision.Sat, Apr 13, 5:09 PM