Bug 437419

I have a different result, as of v21.08.0:
Name=Bug 437419
Executable=true
Arguments:=-r %{Document:Path}
Editor command:=git-cola
Category:=Git

:= means an initially empty value has been filled with WTF.
Other parameters are empty.

Those bogus values return after erasing and saving the tool parameters.
When I set them to "empty", they value persists.
When I erase it and save, the value "empty" returns.

Arguments= (space) works though.  It seems the Editor command must be filled, it does not matter with what unless you want to actually use it as a command.

(In reply to Christopher Yeleighton from comment #1)
> Arguments:=-r %{Document:Path}
> Editor command:=git-cola
> Category:=Git

These things get borrowed from [Tool 0].

Did you find the location in the code for this misbehavior already? That would be great!

Unfortunately, everything in the code looks quite normal.  Additionally, gdb is giving me a hard time:

(gdb) info symbol KateExternalToolsConfigWidget::editTool
KateExternalToolsConfigWidget::editTool(KateExternalTool*) in section .text of /usr/lib64/qt5/plugins/ktexteditor/externaltoolsplugin.so

(gdb) info symbol KateExternalTool::save
There is no field named save

(In reply to Dominik Haumann from comment #4)
> Did you find the location in the code for this misbehavior already? That
> would be great!

The arguments are empty upon return from editTool.
Still empty when added to QStandardItem.
Still empty when being saved at apply.

Breakpoint 9 at 0x7f6a7b468a94: file /usr/src/debug/kate-21.08.0-1.1.x86_64/addons/externaltools/kateexternaltool.cpp, line 129.

Still empty when passed to writeStringEntry by KateExternalTool::save.

However, m_config [Tool 0] .arguments is compromised at this point:

350         m_config->sync();

Continue to investigate.

I think we should replace the entry [Tool 0] with [tool hash] to avoid old values hanging around.

This also means that a tool must have a category specified before saving.  Currently any new tool is processed first because its category is empty, which means it ends up in the first category (in my case category Git), which is unwanted and unexpected.  Again, the way to do it is to add the category as a required selector in the tool editor widget.

I also think that setting a configuration property to an empty string is a bad idea; such a property should not be stored at all.

I think the code is fine as is. But indeed before writing the config file it needs to be cleared before. Maybe that is missing?

In any case, nice catch. I also once looked into this and wasn't able to quickly find the cause for this.

I think I added the regression by accident here:
https://invent.kde.org/utilities/kate/-/commit/372485eb165aa6a58e122dd927e5d4660e9025c1

What is missing is to first remove the section before writing it only partially.

Would be very good, if someone could provide a fix!

Do you mean "to remove the group"?

(In reply to Dominik Haumann from comment #11)
> I think I added the regression by accident here:
> https://invent.kde.org/utilities/kate/-/commit/
> 372485eb165aa6a58e122dd927e5d4660e9025c1

Also, "writeStringEntry" is a hell of a name.  It should be renamed to "writeStringEntryMaybe".

A possibly relevant merge request was started @ https://invent.kde.org/utilities/kate/-/merge_requests/480

Git commit fabd4630efd729d5a9df62a433c8f4949b3f4add by Christoph Cullmann, on behalf of Christopher Yeleighton.
Committed on 02/09/2021 at 11:24.
Pushed by cullmann into branch 'master'.

Delete unfilled entries in tool config group

M  +19   -16   addons/externaltools/kateexternaltool.cpp

https://invent.kde.org/utilities/kate/commit/fabd4630efd729d5a9df62a433c8f4949b3f4add