Fix indenter issues for cstyle and cppstyle (bug #370715)
ClosedPublic

Authored by bevendorff on Oct 18 2016, 8:36 PM.

Details

Summary

The cstyle and cppstyle indenter had certain issues with auto brackets (wrong indentation and semicolons inserted instead of closing braces). This patch fixes those issues and adds unit tests for the new behavior.
See https://bugs.kde.org/show_bug.cgi?id=370715 for more information.

Add unit tests for indentation with auto brackets (failing yet, because Kate modeline for autobrack is ignored)

Fix semicolons being inserted after opening braces when auto brackets are enabled (bug #370715)

Add unit tests for cppstyle with auto brackets (since cppstyle ignores autobrackets, behavior should be the same as without auto brackets)

Remove character consumption to enable auto brackets and revise semicolon patch accordingly, TODO: update unit tests

Expose setAutoBrackets() method to test scripts

Adjust unit tests to properly test auto brackets (not failing anymore)

Diff Detail

Repository
R39 KTextEditor
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
bevendorff updated this revision to Diff 7520.Oct 18 2016, 8:36 PM
bevendorff retitled this revision from to Fix unindentation when writing opening braces with auto brackets enabled (bug #370715).
bevendorff updated this object.
bevendorff edited the test plan for this revision. (Show Details)
Restricted Application added a subscriber: kwrite-devel. · View Herald TranscriptOct 18 2016, 8:36 PM
bevendorff retitled this revision from Fix unindentation when writing opening braces with auto brackets enabled (bug #370715) to Fix indenter issues for cstyle and cppstyle (bug #370715).Oct 18 2016, 8:37 PM
bevendorff updated this object.
bevendorff added a reviewer: brauch.
bevendorff added a project: KTextEditor.
bevendorff updated this revision to Diff 7521.Oct 18 2016, 8:44 PM
  • Remove debug line

Looks good to me, except the few nitpicks below -- assuming that the tests pass, I didn't check :)
Somebody from the KTextEditor people should have a quick look over this as well, I think (Dominik?).

Thanks for the patch!

autotests/input/indent/cppstyle/autobrackets1/expected
10

Does this modeline do anything? If yes, why the .setAutoBrackets? If no, remove it ;)

src/script/data/indentation/cppstyle.js
2369–2370

if you don't want this, remove it and the comment

src/script/data/indentation/cstyle.js
757

please don't use this alignment style, just one space before and after the =

bevendorff updated this revision to Diff 7526.Oct 18 2016, 11:52 PM
bevendorff edited edge metadata.
  • Remove unnecessary modelines
  • Remove old code paths
  • Coding style
bevendorff marked 3 inline comments as done.Oct 18 2016, 11:55 PM

I find aligned assignment operators easier to read, but since you seem to prefer not to align them, I changed it (together with the other things you annotated).
And yes, the indentation unit tests pass. ;-)

dhaumann accepted this revision.Oct 19 2016, 6:43 AM
dhaumann added a reviewer: dhaumann.
dhaumann added a subscriber: dhaumann.

Just ran the unit tests and tested manually with cstyle indenter, and the result looks good, so I trust this patch.

Minor changes:

  • please remove trailing spaces of modified lines (you can configure Kate to do so on save)

Do you have commit permissions, or should we push this to ktexteditor.git ?

This revision is now accepted and ready to land.Oct 19 2016, 6:43 AM

Where do you have trailing spaces?
No, I don't have commit access.

dhaumann closed this revision.Oct 20 2016, 6:07 PM

Has been submitted, see bug report.