Bug 313455

Summary: JJ: Autobracket plugin does not replicate all the functionality of the built in function
Product: [Applications] kate Reporter: Danni Coy <danni.coy>
Component: applicationAssignee: KWrite Developers <kwrite-bugs-null>
Status: RESOLVED FIXED    
Severity: normal CC: andre.stein.1985, apaku, arthur, cberger, cullmann, cyberang3l, darrella, dpashov01, emdeck, finex, gita.benadi, javi, jcook, joschi.brauchle, jpetso, lrbh64, mail, massimiliano.torromeo, opensource, pprkut, sahilsehgal1995, saintdev, silver.salonen, Teyras, tommi.nieminen, zanetu
Priority: NOR Keywords: junior-jobs
Version: unspecified   
Target Milestone: ---   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: autobrace plugin rewrite
autobrace plugin rewrite - fixed undo
autobrace plugin rewrite - fixed undo - no ununised parameter warnings
autobrace plugin rewrite - fixed paste
autobrace plugin rewrite - fixed paste in block mode
autobrace plugin rewrite - remove closing option by default
attachment-2623-0.html

Description Danni Coy 2013-01-18 16:10:40 UTC
I understand the built in autobracket feature was removed from kate - I am not happy about this as the plugin lacks important functionality.



Reproducible: Always

Steps to Reproduce:
1. select some text and press ", (, { or [
2. 
3.
Actual Results:  
kate removes the text and replaces it with the pressed character

Expected Results:  
kate should enclose the selection in "", (), {} or [] respectively.
Comment 1 Danni Coy 2013-01-22 17:08:18 UTC
It gets worse...

The plugin only produces a closing bracket on return - which is useless on one liners.
It sometimes removes a closing character when it shouldn't 
and places closing characters when it shouldn't.
Comment 2 Heinz Wiesinger 2013-01-23 12:04:08 UTC
Same issue(s) here using 4.10rc3
Comment 3 FiNeX 2013-02-09 11:58:36 UTC
I confirm this regression. Just one question: why the existing feature has been removed and replaced with an incomplete one?

Thanks for the attention.
Comment 4 Dominik Haumann 2013-02-11 13:36:47 UTC
Anyone interested in sending a patch?
The relevant code that was removed can be found in katedocument.cpp in:
https://projects.kde.org/projects/kde/kde-baseapps/kate/repository/revisions/d51d4820a854985a833f28f1facac9d9287466ab

It needs to be added to the autobrace-plugin. Just follow http://kate-editor.org/get-it/ and edit the files in part/plugins/autobrace.

Would be really cool to get a patch!
Comment 5 Dimitar Pashov 2013-02-11 23:31:18 UTC
I hoped this is going to be resolved after rc3 but could not wait longer. After looking at the new autobrace plugin and decided it was beyond me patch it I managed to write a new plugin implementing the old functionality bar the deletion which I found to be an annoyance too often. I hope people will share my experience but I can put it in as default option. A configuration panel will have to be made in this case.

The tutorial at http://techbase.kde.org/Development/Tutorials/Kate/KTextEditor_Plugins was helpful as well as the autobrace plugin itself.

By the way https://bugs.kde.org/show_bug.cgi?id=314325 is duplicate of this but I cannot mark it as such.
Comment 6 Dimitar Pashov 2013-02-11 23:32:59 UTC
Created attachment 77176 [details]
autobrace plugin rewrite
Comment 7 FiNeX 2013-02-12 00:30:34 UTC
*** Bug 314325 has been marked as a duplicate of this bug. ***
Comment 8 Danni Coy 2013-02-12 02:22:19 UTC
Ok downloaded and built... So far seems to work as advertised. I should know in a day or two if this will stand up in a production environment.
Comment 9 Michał D. (Emdek) 2013-02-12 07:53:22 UTC
Nice, any chances that it could be included in future release?
Although there is small but annoying issue, it adds one useless action to undo history (I've checked behavior of Kate in KDE 4.8 and it works correctly).
Comment 10 Dimitar Pashov 2013-02-12 10:44:35 UTC
Yes indeed it adds annoying extra undo step because the signal of the edit gets to the plugin after the text has been changed i.e. the opening character typed and selection deleted. The KTextEditor::Document class does not provide 'undo'. It is only given in KateDocument and I am not sure whether I can use it in a plugin.

I am trying to remember whether the undo issue existed in 4.9 but I can't. In 4.8 I think the autobrace might have been an internal KateDocument feature.
Comment 11 Michał D. (Emdek) 2013-02-12 10:50:37 UTC
(In reply to comment #10)
> I am trying to remember whether the undo issue existed in 4.9 but I can't.

I don't have 4.9 available right now, but I don't remember such issue (although undoing after such action is not something that is done very often).
Comment 12 Vangelis 2013-02-12 12:01:32 UTC
Works for me well. 
Only problem was that the installation copied the files in the wrong paths(?), so I had to copy them myself to the proper ones.

sudo cp /usr/local/lib/kde4/ktexteditor_bracesane.so /usr/lib/kde4/
sudo cp -r /usr/local/share/apps/ktexteditor_bracesane/ /usr/share/apps/
sudo cp -r /usr/local/share/kde4/services/ktexteditor_bracesane.desktop /usr/share/kde4/services/
Comment 13 Michał D. (Emdek) 2013-02-12 12:04:46 UTC
(In reply to comment #12)
> Only problem was that the installation copied the files in the wrong
> paths(?), so I had to copy them myself to the proper ones.

You should use correct prefix, like:
cmake ../ -DCMAKE_INSTALL_PREFIX=`kde4-config --prefix`
Comment 14 Dimitar Pashov 2013-02-12 12:46:05 UTC
I think I have fixed the undo issue. I noticed the history entry is recorded after the textRemoved and textInserted signals and before the textChanged signal.
Comment 15 Dimitar Pashov 2013-02-12 12:47:06 UTC
Created attachment 77216 [details]
autobrace plugin rewrite - fixed undo
Comment 16 Michał D. (Emdek) 2013-02-12 13:01:10 UTC
(In reply to comment #14)
> I think I have fixed the undo issue. 
Confirmed, works fine now.
But compiler now issues two warnings about unused parameter ‘document’.
Comment 17 Dimitar Pashov 2013-02-12 14:35:34 UTC
> But compiler now issues two warnings about unused parameter ‘document’.
These are OK as they are send from the signals. I'll disable the warnings to not raise suspicion.
Comment 18 Dimitar Pashov 2013-02-12 14:36:36 UTC
Created attachment 77220 [details]
autobrace plugin rewrite - fixed undo - no ununised parameter warnings
Comment 19 Michał D. (Emdek) 2013-02-12 14:42:18 UTC
(In reply to comment #17)
> > But compiler now issues two warnings about unused parameter ‘document’.
> These are OK as they are send from the signals. I'll disable the warnings to
> not raise suspicion.
Sure, but it looks better without any warnings. ;-)

AFAIK you could just omit unused variable in method signature and create connection like:

    connect(document, SIGNAL(textChanged(KTextEditor::Document*)),
            this, SLOT(slotTextChanged()));
Comment 20 Michał D. (Emdek) 2013-02-12 15:15:41 UTC
There seems to be some issues with this extensions, like additional brackets while pasting snippets.
For example pasting contents of this one to empty document:
http://paste.kde.org/669770/
Creates additional closing curly bracket at end.
And even weirder one, selecting contents of above file and pasting gives very weird results.

Both happen only while extension is enabled.

Is it possible to detect text pasting and exclude such cases?
Comment 21 Dimitar Pashov 2013-02-12 16:39:11 UTC
Created attachment 77225 [details]
autobrace plugin rewrite - fixed paste

It seems there is no sane way to distinguish, in a plugin, between pasted and typed text. I fixed the pasting issue you experienced and I think the only time a closing character will appear after a paste operation will be if the pasted text consists of a sole opening character. There is no way to prevent this AFAIK.
Thanks for the slot tip. I can see how it may work for singe argument (or rather last argument), but I am suspicious about the case of slotTextRemoved where the second arg is important but the first unused. I put the Q_UNUSED(arg) macro to deal with this.
Comment 22 Michał D. (Emdek) 2013-02-12 17:02:38 UTC
Confirmed, seems to work flawlessly now. :-)

(In reply to comment #21)
> Thanks for the slot tip. I can see how it may work for singe argument (or
> rather last argument), but I am suspicious about the case of slotTextRemoved
> where the second arg is important but the first unused. I put the
> Q_UNUSED(arg) macro to deal with this.

AFAIK Q_UNUSED() is the only solution in such case.
Comment 23 Dimitar Pashov 2013-02-13 22:48:48 UTC
Created attachment 77269 [details]
autobrace plugin rewrite - fixed paste in block mode

I have found and fixed what I hope is the last issue with this plugin: pasting text in block mode with sole opening character in the first line lead to erroneous closing chars being added.
Comment 24 Michał D. (Emdek) 2013-02-15 13:21:26 UTC
There is one minor detail still missing, Kate used to remove closing character when opening one was deleted (tested using KDE 4.8).
For example if we deleted "(" from "function()" then ")" was also deleted.
Comment 25 Dimitar Pashov 2013-02-15 14:46:22 UTC
Hi Michał, I mentioned in my first post that I did not implement this one feature. After many years with it I found it counterproductive. Though If you and others like it I can implement it and put an option for disabling.
Comment 26 Michał D. (Emdek) 2013-02-15 15:14:08 UTC
Oops, I've overlooked that.
And yes, surely it could be annoying for some users so if it would be added then maybe it would a good idea to make it optional (disabled by default).
Comment 27 Massimiliano Torromeo 2013-02-15 16:06:39 UTC
I also vote for having back the automatic deletion of the closing parenthesis/quotes.
I would like to report that the plugin has a problem when the text selection starts at the end of a line.

Thanks for the plugin Dimitar, in my office everyone was missing this functionality.
Comment 28 FiNeX 2013-02-15 16:35:45 UTC
I agree about a setting to enable/disable the automatic deletion of the closing char. Thanks!
Comment 29 Dimitar Pashov 2013-02-17 13:28:38 UTC
Created attachment 77378 [details]
autobrace plugin rewrite - remove closing option by default

Yes indeed when the selection starts at end of line it gets messed up. This attachment fixes this and adds an option to enable/disable removing of the closing character if the opening is deleted as a sole character. If the removal is part of a selection then the closing character is restored.
Comment 30 Michał D. (Emdek) 2013-02-17 14:46:19 UTC
Thanks a lot. :-)
Comment 31 Vangelis 2013-02-18 15:28:53 UTC
Just found one more bug :)

I have the following text and I want to replace all occurrences of &apos; with '

location=grep &apos;&lt;name&gt;&apos; | sed -e &apos;s/&lt;name&gt;\(.*\)&lt;\/name&gt;/\1/&apos;
country=grep &apos;&lt;country&gt;&apos; | sed -e &apos;s/&lt;country&gt;\(.*\)&lt;\/country&gt;/\1/&apos;

When I launch the replace all command I get this:
location=grep '&apos;'&lt;name&gt;'| sed -e 's/&lt;name&gt;\(.*\)&lt;\/name&gt;/\1/'
country=grep '&lt;country&gt;' | sed -e 's/&lt;country&gt;\(.*\)&lt;\/country&gt;/\1/'

instead of this that I should get:
location=grep '&lt;name&gt;'| sed -e 's/&lt;name&gt;\(.*\)&lt;\/name&gt;/\1/'
country=grep '&lt;country&gt;' | sed -e 's/&lt;country&gt;\(.*\)&lt;\/country&gt;/\1/'

The same of course occurs if you want to replace all with ", (, { and anything else the plugin will autobrace.
Comment 32 Dominik Haumann 2013-02-18 17:42:32 UTC
*** Bug 315389 has been marked as a duplicate of this bug. ***
Comment 33 Dimitar Pashov 2013-02-18 22:03:47 UTC
I have looked a bit this evening and I did not see how to get a signal for beginning of search&replace action in a sane way. As it is now I cannot differentiate between the different kinds of replace: type over selection, paste over selection and search&replace. The way the remove insert and change signals are sent is bad enough in that it requires significantly messier code, to somewhat distinguish between the first 2, than I wish to have. 
If anyone has an idea how to fix the search&replace issue I will be glad to hear it!
 Otherwise I hope this plugin is a short term solution until the original feature is brought back. When I wrote the first version I had no patience left and thought this should be simple thing. After figuring out the quirks of ktextdocument it does look like a plugin is the wrong approach for this functionality.
Comment 34 Tommi Nieminen 2013-03-17 20:22:23 UTC
BraceSane works fine, thank you!

But it still makes me wonder. What was the reason to remove the functionality from Kate in the first place? Why was AutoBrace used as a replacement if its author never intended it as such (as he told me in an email)? Has the KDE dev team gone totally insane?!?!
Comment 35 Dominik Haumann 2013-03-17 20:43:29 UTC
Of course we are insane, mad, totally crazy and what not at the same time. What did you expect?

On a more serious note: It was removed because it clashed with the shipped autobrace plugin and thought the autobrace plugin would work just fine. It's that simple. If we add the feature back, the autobrace plugin won't work correctly anymore. I think we should bring the option it back, along with the possibility that the auto-bracket option is handled by indenters as it is sometimes language-specific...
Comment 36 Jakob Petsovits 2013-03-17 22:38:01 UTC
Tommi, I said "originally", not "never". Please don't twist my words and don't resort to name calling, it's not very productive. The Kate team has improved the functionality a lot and is clearly working on getting the best of both worlds out to the people.

On that note, let me also say that I find it heartwarming how all of you guys work together to make my originally sidelined plugin really awesome and versatile. Here's a huge thanks for everyone involved.
Comment 37 Tommi Nieminen 2013-03-18 08:18:57 UTC
Dominik: Sorry for the strong words but they came from the heart: lost functionality seems to be the ongoing trend now in KDE (as it is in G****). The AutoBrace plugin is a poor substitute, if only because braces {} are only completed for LaTeX files. I write most of my scripts and programs in Python and need all of () [] {} "".

Jakob: I misunderstood you then. However, I fail to see the difference between “originally” and “never” if we are talking about the timeframe when you created the plugin. Also I find it strange that no one seems to have bothered to check whether the plugin really did the job of the old feature before deciding to remove the feature.
Comment 38 Woodsman 2013-04-08 18:48:09 UTC
I would like to add the plugin design for single and double quotation marks is different from the original builtin function. The builtin function did not add a closing single or double quotation mark when the opening mark immediately follows a non space character. The original builtin function added the closing quotation mark only when the opening mark followed a space, which is appropriate behavior.

I confirm the behavior with braces {}. I see the reasoning for adding the closing brace with pressing Enter, useful in a programming environment, but confusing to anybody wanting to use braces in a single line.

Thanks. :)
Comment 39 Jeff 2013-05-01 15:44:18 UTC
Thank you for this plugin! This is one fuctionality I was missing in kdevelop, the one that ships kind of sucks.

Cheers!
Comment 40 Cyrille Berger 2013-05-31 08:05:58 UTC
@Dimitar thanks for your plugin, I found a bug, in a function, if I add a "if(statement)" and press return, and then try to add the curly braces, then it only add the closing one.

For instance, I get:
void function()
{
  if(blah)
  }
}

It seems to be caused by the automatic indentation being removed. I have fixed the problem in the plugin, by commenting in the function "BraceSanePluginDocument::slotTextInserted", the second if case: else if (op_inserted && inserts == 1) { ... } (which seems to be related to the paste bug, but even with that code commented, I could not reproduce the paste bug).
Comment 41 Mathieu Jobin 2014-04-17 11:42:45 UTC
how about something like this??

diff --git a/addons/ktexteditor/autobrace/autobrace.cpp b/addons/ktexteditor/autobrac
index ac079b6..ed7b303 100644
--- a/addons/ktexteditor/autobrace/autobrace.cpp
+++ b/addons/ktexteditor/autobrace/autobrace.cpp
@@ -267,7 +262,11 @@ void AutoBracePluginDocument::slotTextInserted(KTextEditor::Docu
         // Only insert auto closing brackets if current text range
         // is followed by one of the allowed next tokens.
         if (allowedNextToken.contains(nextToken(document,range))) {
-            insertAutoBracket(document, range, brackets[text]);
+            if (view->selection()) {
+                insertAutoBracket(document, range, brackets[text] + view->selectionT
+            } else {
+                insertAutoBracket(document, range, brackets[text]);
+            }
         }
 
     }
Comment 42 Mathieu Jobin 2014-04-17 11:44:45 UTC
my terminal was not wide enough when I copy/pasted

s/view->selectionT$/view->selectionText());/
Comment 43 Rik 2015-01-16 19:04:15 UTC
Also, pressing { used to yield {} (which was good) whereas now it requires a new line before producing the } (which is bad).

Also (which was good), with [x] and the cursor after the  x, two back spaces would remove all three chars, now (which is bad) the ] is left behind.

Also, would like to add my voice to the frustration expressed about losing the ability to highlight and surround with "", '', {}, [] and ()
Comment 44 sahil 2015-01-29 17:35:06 UTC
Can anyone tell that what is finally to be done in this bug so that it can be closed?
Comment 45 Jan Buchar 2015-01-29 17:38:03 UTC
It seems that this persists in the Qt5 version of Kate (which has no autobracket plugin)
Comment 46 sahil 2015-01-29 18:24:11 UTC
Everything works fine for me. Can anyone tell what left in this bug?
Comment 47 Massimiliano Torromeo 2015-01-29 19:32:46 UTC
(In reply to sahil from comment #46)
> Everything works fine for me. Can anyone tell what left in this bug?

Everything this bug describes is still not working as far as I can tell.
Comment 48 Rik 2015-01-29 20:47:46 UTC
Massimiliano Torromeo is correct, I think.

I still see all the same behaviour (with Tools.Mode.Normal, if that`s relevant) that is described in Comment 43.
Comment 49 Christoph Cullmann 2015-10-08 11:25:41 UTC
KTextEditor master has now at least a partial implementation back, for the commits look in bug 350317
Comment 50 Sven Brauch 2015-10-11 12:27:11 UTC
I added some features as well in master/KF5, I'll close this; please re-open if you're still missing something.
Comment 51 Rik 2015-10-12 17:52:23 UTC
Created attachment 94970 [details]
attachment-2623-0.html

Thanks.  Guess I'll have to wait for it to go through the release process
before I can tell (I use Kubuntu).

Not actually sure how I'll know when your changes have made it too my
machine!

Cheers
Rik




On 11 October 2015 at 13:27, Sven Brauch <mail@svenbrauch.de> wrote:

> https://bugs.kde.org/show_bug.cgi?id=313455
>
> Sven Brauch <mail@svenbrauch.de> changed:
>
>            What    |Removed                     |Added
>
> ----------------------------------------------------------------------------
>          Resolution|---                         |FIXED
>              Status|CONFIRMED                   |RESOLVED
>                  CC|                            |mail@svenbrauch.de
>
> --- Comment #50 from Sven Brauch <mail@svenbrauch.de> ---
> I added some features as well in master/KF5, I'll close this; please
> re-open if
> you're still missing something.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
>
Comment 52 Mathieu Jobin 2015-11-08 14:10:02 UTC
I am using the latest available kate, 15.08.2

any idea in which release the new changes will be available?

thanks
Comment 53 Sven Brauch 2015-11-08 14:17:34 UTC
The next one.