Add an action to insert a non-indented newline
ClosedPublic
Actions

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

Details

Reviewers

cullmann
dhaumann

Group Reviewers

KTextEditor

Commits

R39:01526bfcb7e8: Add an action to insert a non-indented newline

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

Branch

regular-newline (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 13614
Build 13632: arc lint + arc unit

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

Harbormaster completed remote builds in B13614: Diff 61156.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	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?

In D22276#491055, @mickaelbo wrote:

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	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

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

Use an Enum instead of bool

Harbormaster completed remote builds in B13713: Diff 61363.Jul 8 2019, 6:08 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

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

Harbormaster completed remote builds in B13721: Diff 61373.Jul 8 2019, 8:20 PM

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

Closed by commit R39:01526bfcb7e8: Add an action to insert a non-indented newline (authored by ahmadsamir, committed by cullmann). · Explain WhyJul 8 2019, 8:59 PM

This revision was automatically updated to reflect the committed changes.

Thanks for the patch!

Revision Contents
Changeset List

			Path	Packages
M			src/document/katedocument.h (2 lines)
M			src/document/katedocument.cpp (8 lines)
M			src/view/kateview.h (8 lines)
M			src/view/kateview.cpp (17 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	61156	44f0498		Jul 4 2019, 4:21 PM	★	★
Diff 2	61363	44f0498	Use an Enum instead of bool	Jul 8 2019, 6:08 PM	★	★
Diff 3	61373	44f0498	Fix action name, don't use camel case, use _	Jul 8 2019, 8:20 PM	★	★
Diff 4	61374	c032147	R39:01526bfcb7e811dbc787f2fdf2924f30d7f8324c	Jul 8 2019, 8:59 PM	★	★