[Differential] [Commented On] D4234: Change algorithm for autobrace.
dhaumann added a comment. 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... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
cactus added a comment.
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.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4234
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: cactus, #ktexteditor
Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
dhaumann added a comment. 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 :-) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
brauch added a comment. In https://phabricator.kde.org/D4234#86372, @anthonyfieroni wrote: > 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 ... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
anthonyfieroni added a comment. In https://phabricator.kde.org/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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: anthonyfieroni, dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
cullmann added a comment. As I don't use this myself, my opinion won't really matter. Sven, is the new proposed behavior really worse than the old? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
brauch added a comment. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
dhaumann added a comment. What's the current state of this? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: dhaumann, brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
cactus added a comment. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
brauch added a comment. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
cactus added a comment. In https://phabricator.kde.org/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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
brauch added a comment. I'm a bit skeptical, this means I can't add closing braces when they are missing ... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: brauch, cullmann, kwrite-devel, #frameworks
[Differential] [Commented On] D4234: Change algorithm for autobrace.
cullmann added a comment. Given it eliminates some members, this looks nice, thought I don't use that feature enough to judge if the behavior change is wanted. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4234 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cactus, #ktexteditor Cc: cullmann, kwrite-devel, #frameworks
