Add an action to insert a non-indented newline
ClosedPublic

Authored by ahmadsamir on Jul 4 2019, 4:21 PM.

Details

Summary

Currently pressing Enter to insert a new line, KTextEditor will,
in most cases, indent the new line with the same indentation as
the previous line, which makes sense when writing code. But there
is a viable use case where a user may want to insert a new line to
separate blocks of code that have different context/functionality
inside a function, in that case that line should be non-indented,
having trailing spaces on an empty line isn't good, IMHO.

The new action can be triggered with Ctrl+Enter. And the current
default behaviour is preserved.

BUG: 314395
FIXED-IN: 5.61.0

Test Plan
  • Open a file with some C++ code, put the cursor at the end of an indented line, and press Enter to insert a new line
  • Note that the new line is indented
  • Repeat the first step, but press Ctrl+Enter; the newly inserted line has no indentation

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Jul 4 2019, 4:21 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJul 4 2019, 4:21 PM
ahmadsamir requested review of this revision.Jul 4 2019, 4:21 PM

Ctrl+Enter/Return may be a too common shortcut, i.e. users probably have it already mapped to do something, so maybe we shouldn't set a default shortcut for that action...

Another way to avoid indented empty line is to clear the trailing spaces/indentation when pressing enter while the cursor is at the end of a line containing only spaces or \t. This can lead to a nightmare though if someone want to keep trailing spaces...

I think this is no bad idea.
About the default shortcuts I am not sure.
For the code: I would prefer a enum instead of a bool, to make it more clear what it does.

dhaumann added inline comments.Jul 5 2019, 7:45 AM
src/document/katedocument.h
827–829

I dislike the double negation: noIndentation = false. Later even !noIndentation. This is bad API design.

Please change to bool indent = true.

src/view/kateview.cpp
2881

Do we really need this call? Also Don't we miss setting the cursor position? Incliding a begin/endEdit?

bruns added a subscriber: bruns.Jul 5 2019, 10:34 AM

Another way to avoid indented empty line is to clear the trailing spaces/indentation when pressing enter while the cursor is at the end of a line containing only spaces or \t. This can lead to a nightmare though if someone want to keep trailing spaces...

This is similar to what Vim does, although not exactly.

Vim only clears the line when you haven't modified the line, so you can force a line containing only indendation whitespace by adding any character (or space) and deleting it. Works really nice.

cullmann added inline comments.Jul 5 2019, 8:47 PM
src/view/kateview.cpp
2881

I assume this is just copied from the normal

void KTextEditor::ViewPrivate::keyReturn()
{

doc()->newLine(this);
m_viewInternal->iconBorder()->updateForCursorLineChange();
m_viewInternal->updateView();

}

;=) In doubt I would just keep this as is and one can think separately if that is needed in both places.

ahmadsamir added inline comments.Jul 5 2019, 9:41 PM
src/document/katedocument.h
827–829

Noted. Although I'll use an enum per cullmann's recommendation (but I'll choose a better name, hopefully).

(Believe it or not, I had it as "const bool indent" first, but then bike-shed it to an over-engineered death).

src/view/kateview.cpp
2881

Correct. My reasoning was, it's exactly like keyReturn(), but with a different newLine() call, (i.e. someone else invented the wheel I just pushed in a different direction, so to speak).

As for updateForCursorLineChange(), digging around in git history I found this[1], so I guess it's needed.

Setting the cursor position and begin/endEdit are handled by DocumentPrivate::newLine().

[1]https://bugs.kde.org/show_bug.cgi?id=340363

cullmann requested changes to this revision.Jul 8 2019, 5:22 PM

I think, once there is no bool but some enum, one can push this as is.
If one wants to remove the updateView()... glibberish can be decided later, it is ok that this is a 1:1 copy of the other function at the moment.

This revision now requires changes to proceed.Jul 8 2019, 5:22 PM
ahmadsamir updated this revision to Diff 61363.Jul 8 2019, 6:08 PM

Use an Enum instead of bool

cullmann requested changes to this revision.Jul 8 2019, 6:25 PM

Please fix the

// second: if "indent" is true, indent the new line, if needed...

comment

and use some non-camel-case action name like no_indent_newline. Then this is ok.

This revision now requires changes to proceed.Jul 8 2019, 6:25 PM
ahmadsamir updated this revision to Diff 61373.Jul 8 2019, 8:20 PM

Fix action name, don't use camel case, use _
Fix comment indentation :)

If this is OK, please commit it too.

Thanks.

cullmann accepted this revision.Jul 8 2019, 8:59 PM
This revision is now accepted and ready to land.Jul 8 2019, 8:59 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the patch!