Change algorithm for autobrace.
AbandonedPublic

Authored by cullmann on Jan 21 2017, 4:30 PM.

Details

Reviewers
mwolff
brauch
cactus
Group Reviewers
KTextEditor
Summary

The autobrace ignore closing brace when it already is here.

Diff Detail

Repository
R39 KTextEditor
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
cactus updated this revision to Diff 10412.Jan 21 2017, 4:30 PM
cactus retitled this revision from to Change algorithm for autobrace..
cactus updated this object.
cactus edited the test plan for this revision. (Show Details)
cactus added a reviewer: KTextEditor.
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 21 2017, 4:30 PM
Restricted Application added subscribers: Frameworks, kwrite-devel. · View Herald Transcript

Given it eliminates some members, this looks nice, thought I don't use that feature enough to judge if the behavior change is wanted.

brauch added a subscriber: brauch.Jan 22 2017, 4:22 PM

I'm a bit skeptical, this means I can't add closing braces when they are missing ...

In D4234#79253, @brauch wrote:

I'm a bit skeptical, this means I can't add closing braces when they are missing ...

Yes, I agree with you. If you are in this situation: (((|)) with | for the cursor, you must type 3 ')' to close the brace. But this rarely happens.
I find the current behavior much more embarrassing.

I'm already used to this behavior because I use emacs and intellij idea and they work like that.

brauch added a comment.EditedJan 23 2017, 11:36 PM

Let's discuss an alternative suggestion maybe: how about it just keeps track of the parenthesis balancing and removes them if doing so would make it unbalanced? It could stop counting at the next folding region, and exclude spellchecked parts.

That would make it work as you suggested in the case I think you care about, but still make it possible to add missing parentheses...

Sorry, I am not sure to understand what do you want. You would like the previous behavior with a multi-depth stack of range?

I don't understand what disturb you in my solution. You can close a missing bracket. If you have (| and you press ')', you obtain ()|.
But if you have many parenthesis with missing closed one, this solution considers the missing is the much external.
So with (((|)), and pressing many time on the closed parenthesis give you
((()|)
((())|
((()))|

But I think this situation is very occasional because every brace is automatically closed.

What's the current state of this?

Sorry, I wanted to write a reply but failed. My idea was simply to have the algorithm always aim to make parentheses balanced when closing one.

As I don't use this myself, my opinion won't really matter.
Sven, is the new proposed behavior really worse than the old?

anthonyfieroni added inline comments.
src/document/katedocument.cpp
2920–2921

Why seach a checks ?

In D4234#85804, @brauch wrote:

Sorry, I wanted to write a reply but failed. My idea was simply to have the algorithm always aim to make parentheses balanced when closing one.

You mean when we have 3 open when 1 is close to be inserted 2 more to balance counting ? It's not a good idea, about me.

You mean when we have 3 open when 1 is close to be inserted 2 more to balance counting ? It's not a good idea, about me.

No, but if you type a closing one right before a closing one, "eat" it iff there's no closing one missing right now.

I don't think the suggested behaviour is worse than what we have now, but it's also not better IMO, so I'm not in favour of changing it ...

In my opinion, the current solution is "ok", i.e., if I type: void foo(bar); with auto-brackets enabled, the current code is smart enough eat the ')' since it maintains the range. While defining new functions or calling methods, this works perfectly.

The proposed patch does the same but very aggressive, so it is not "clearly better", since it will have many false positives.

I vote to reject this patch, since it's much better to decide this now than waiting months without any action.

@Jérémy: That said, I still appreciate very much the fact that you try to improve this. Please don't feel demotivated by this and provide many more patches :-)

Hello,

In the case of void foo(bar); the both solution has the same result. But when you have 2 levels of bracket the result are different.
In the example of :
if (isEndMatching())
or
printf("Bonjour")

With auto-brackets enabled and with the old behavior, if you type every char you obtain :
if (isEndMatching()))
and
printf("Bonjour"))

To have 2 levels of bracket is very common.
For example, in the diff below you have :
if (view->config()->autoBrackets() && chars.size() == 1) {
closingBracket = matchingEndBracket(chars[0], true);
if ( m_currentAutobraceClosingChar == chars[0] && m_currentAutobraceRange ) {
if (!closingBracket.isNull() && !skipAutobrace ) {
if (!(config()->backspaceIndents())) {
const auto nextChar = view->document()->text({cursorPos, cursorPos + Cursor{0, 1}}).trimmed();
uint col = qMax(c.column(), 0);
removeText(KTextEditor::Range(line - 1, textLine->length(), line, 0));

I think more than 1/2 of this below code use more than 1 level of bracket and the current code isn't enough smart to eat them.

@dhaumann: I don't feel demotivated by this and I would try to help more. I don't have time now but it is not because you reject my patch than I stop to want helping you.
The behavior of auto-bracket is a personal choice. I think the current code is frustrating to me but I understand some other people dislike my patch.

Hm ok, I never hit that problem, I guess we're just different there. I have muscle memory which types f() or f(x) or "hi", but not for more than one level of closing parentheses.

I'm really sceptical of the "always eat parentheses" suggestion, it seems too extreme to me. My suggested solution fixes your issue as well, by the way, so maybe we can find some middle grounds?

Ok, I see. Then I would like you to look at how Qt Creator behaves here. And agree, then the proposed patch is maybe better than thought. If you want, you can even look into the implementation of Qt Creator...

mwolff requested changes to this revision.Mar 19 2017, 1:25 PM
mwolff added a subscriber: mwolff.

I'm personally all for improving the status quo, but I think the biggest problem here is that we have no unit test coverage (or do we?). The unit tests would also clearly show the advantage of this new beheavior compared to the old one. So: Could you add unit tests?

This revision now requires changes to proceed.Mar 19 2017, 1:25 PM

Hm, I think the biggest problem is that we don't know how we want it to work ... so we can't write tests either ;)

Writing tests for this might not be easy, so even having passing tests for the current behavior would be very useful, as a first step.

Restricted Application added a project: Kate. · View Herald TranscriptAug 14 2018, 2:38 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald Transcript
cullmann commandeered this revision.May 8 2019, 5:52 PM
cullmann added a reviewer: cactus.

We altered stuff in D19608.
Could you try if that fits your needs?
If not, perhaps we need a new review for what can be further improved, thanks.

cullmann abandoned this revision.May 8 2019, 5:52 PM