Bug 115175 - [PATCH] Implement more consistent trailing spaces removal
Summary: [PATCH] Implement more consistent trailing spaces removal
Status: RESOLVED FIXED
Alias: None
Product: kate
Classification: Applications
Component: part (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR wishlist
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
: 117588 124389 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-10-27 06:25 UTC by ndeb
Modified: 2012-05-18 10:55 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Remove trailing blanks from document on saving (2.27 KB, patch)
2011-12-25 16:15 UTC, Davorin Učakar
Details
Version that does not move cursor (1.61 KB, patch)
2011-12-28 17:52 UTC, Davorin Učakar
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ndeb 2005-10-27 06:25:08 UTC
Version:           4.4 (using KDE 3.4.2, Mandrake Linux Cooker i586 - Cooker)
Compiler:          Target: i586-mandriva-linux-gnu
OS:                Linux (i686) release 2.6.12-12mdk-i686-up-4GB

As says in the subject. When the file is saved, the trailing spaces are supposed to vanish but they do not. This bug has been observed in both kate and kwrite.
Comment 1 ndeb 2005-10-27 07:56:15 UTC
In addition, the "audio backend" option always gets reset to "arts" even if it is manually changed to "alsa".
Comment 2 Anders Lund 2005-10-27 18:43:29 UTC
On Thursday 27 October 2005 07:56, ndeb@ece.cmu.edu wrote:
> ------- Additional Comments From ndeb ece cmu edu 
Comment 3 Anders Lund 2005-10-27 18:44:47 UTC
On Thursday 27 October 2005 06:25, ndeb@ece.cmu.edu wrote:
> trailing spaces are supposed to vanish but they do no


Yes, I noted that yesterday in kde 3.5 branch. It looks like it's a bit older 
then. Cullmann, hamish, comments?
Comment 4 ndeb 2005-10-28 06:32:38 UTC
To comment 2, I added those lines by mistake (should have gone to another bug report).

To comment 3, this means that this is not a mandriva distro-specific bug. I know that stock kde-3.4.3 compiled on suse does not have this problem.
Comment 5 Christoph Cullmann 2005-10-29 13:37:36 UTC
Atm have no idea where to look ;)
Btw., do we talk about the automagic removal of spaces at save or do we talk about the removal at editing time?
Comment 6 Anders Lund 2005-10-29 13:54:54 UTC
On Saturday 29 October 2005 13:37, Christoph Cullmann wrote:
> Btw., do we talk about the automagic removal of spaces at save or do we
> talk about the removal at editing time?


Both.

-anders
Comment 7 ndeb 2005-10-30 05:57:49 UTC
To comment 5, I was referring to the removal of trailing spaces at save (which does not work presently). I did not know there was an option of removal of trailing spaces at editing time itself!
Comment 8 Christoph Cullmann 2005-10-30 17:52:29 UTC
Yes, there are two options:

a) under editing: this option does what you want and removes trailing spaces while editing, they will just disappear while editing

b) under saving: this does remove the trailing spaces, too, but only on save/load, which means: yes, they will stay around in your editor after save, but won't end up on disk
Comment 9 ndeb 2005-11-02 03:29:30 UTC
I am not sure comment b) makes sense. Saving is always to the hard disk. Did you mean to say that the trailing spaces will stay in the editor _before_ save but will be removed when the file is saved.
Comment 10 ndeb 2005-11-02 03:32:18 UTC
As for option a), I could not find any such option in kate configuration. There is only one option for removing trailing spaces in "Editing" in the kwrite/kate config window.
Comment 11 Anders Lund 2005-11-02 07:34:24 UTC
You havent stted your version. But since a long time, the option in the 'Editing' panel of kates configuration dialog means remove trailing spaces while editing. It removes trailing spaces on the old line when a new line is entered. This option does not remove trailing spaces leading the cursor.

The options in the Save/Load panel removes trailing spaces from the lines in the data before they are written to disk. They are *not* removed from the view in this case. You can press reload if you don't believe us. This option is there for you if you want to be sure that your disk files does not have trailing spaces.
Comment 12 Anders Lund 2005-11-02 07:57:45 UTC
Christoph, Just an afterthought: The save filter seems to be *very* hard to grip. So I think we should come up with better documentation, for example in the whatsthis help.

A side note: The way the dynamical removing works is not good, it only removes spaces when return is pressed. Why was that changed? And it was changed without adjusting the documentation in the whatsthis help, which claims that trailing space is removed when the insertion cursor leaves a line.
Comment 13 Verdi March 2006-06-01 13:57:56 UTC
Looks like this bug resurfaces with KDE 3.5.3 (suse, 10.1).
My Kate's settings:
- Editing/"Remove trailing spaces" is checked
- Open/Save/"Remove trailing spaces" is also checked

Comment 14 Jakob Petsovits 2006-06-13 19:43:22 UTC
Personally, I don't understand why the editor keeps the trailing spaces on saving when they are gone in the saved file.

I think when saving a file you should try to reflect its contents in the editor view. If the spaces in the view are removed on saving as well, then I can see it with my own eyes that this has been done. Maybe I'm just over-sensitive, but it's annoying for me when I save the file and all those trailing spaces are still there.

Please explain the rationale behind the change in behaviour. I'd be glad if it was reverted to the old style (having the spaces removed both on disk and in the editor).
Comment 15 Jakob Petsovits 2006-06-13 19:54:11 UTC
Quoting Anders Lund at the dupe bug 117588, http://bugs.kde.org/show_bug.cgi?id=117588#c7 :

> The function you use is a filter that removes trailing whitespace from the
> text that is written to disk, but since this seems to frustrate many people,
> maybe we should fix this.

Together with the fact that this dupe is still open and has status NEW, I take this as an invitation to reopen this bug and close the other one instead.
Comment 16 Jakob Petsovits 2006-06-13 19:54:39 UTC
*** Bug 117588 has been marked as a duplicate of this bug. ***
Comment 17 Matthew Woehlke 2006-06-13 21:05:32 UTC
I'm not entirely certain I understand what this bug is, but it sounds like one that would concern me, so can someone please clarify?

If I understand correctly, the problem is that clicking 'save' does not remove trailing spaces from the view (although the file is not saved with trailing spaces) if 'Open-Save->Remove trailing spaces' is checked.

If the above is correct, I can confirm that it works correctly in KWrite 4.4 (KDE 3.4.3) as built from sources, which would make this a regression, correct?
(This seems to be more clearly stated in Bug 117588, but I would like it here so people don't have to go digging for that extra information.)
Comment 18 Vadym Krevs 2006-06-13 21:42:41 UTC
Yep, it is a regression since KDE 3.4.x.
Comment 19 Jan Jergus 2006-07-20 22:07:59 UTC
It is still not working as expected in 3.4.3. After pressing Ctrl-S, the file is saved correctly (with trailing spaces removed) but the trailing spaces are still visible in the editor.
Comment 20 Jan Jergus 2006-07-20 22:10:26 UTC
*** This bug has been confirmed by popular vote. ***
Comment 21 Jan Jergus 2006-07-20 22:13:05 UTC
I apologize for a mistake in my previous comment #19, the correct version is 3.5.3, not 3.4.3.
Comment 22 Matthew Woehlke 2006-10-17 21:26:11 UTC
Still broken in 3.5.5, with plenty of people complaining about it. Also, can someone with write access check bug #124389 (looks like a dup)?
Comment 23 ndeb 2006-11-02 08:17:55 UTC
Bug 124389 does refer to this bug and seems to be a dup of this. And it also makes a very valid point: 

"The spaces are removed, but _only in the saved document_. You have to reload to see your changes. 
 
 Of course, this is not good, since reloading clobbers your undo/redo history."
Comment 24 Dominik Haumann 2006-11-02 10:57:27 UTC
*** Bug 124389 has been marked as a duplicate of this bug. ***
Comment 25 Shriramana Sharma 2007-03-17 14:46:10 UTC
Ye gads, I never noticed this. Adding my votes. (It persists in 3.5.6.)
Comment 26 Shriramana Sharma 2007-04-14 17:14:24 UTC
OK instead of opening another bug I will report this here itself:

The "remove trailing spaces" in editing works only when I press enter on a line. If I type a space at the end of a line and move to the previous or next line using the cursors, this cleanup is not done.

Should I have reported it separately?
Comment 27 Matthew Woehlke 2007-04-16 16:43:44 UTC
> The "remove trailing spaces" in editing works only when I press enter on a line.
> If I type a space at the end of a line and move to the previous or next line
> using the cursors, this cleanup is not done.
>
> Should I have reported it separately?

Hmm... this was actually talked about in comment 12. We do seem to have two bug reports mashed into one already. Will check what the devs want to do.
Comment 28 Davorin Učakar 2007-05-16 01:07:54 UTC
Here's my patch:

diff -Naur kate/part.old/katebuffer.cpp kate/part/katebuffer.cpp
--- kate/part.old/katebuffer.cpp	2006-07-22 10:16:36.000000000 +0200
+++ kate/part/katebuffer.cpp	2007-05-14 00:55:08.000000000 +0200
@@ -592,16 +592,9 @@

     // strip spaces
     if (removeTrailingSpaces)
-    {
-      int lastChar = textline->lastChar();
+      textline->truncate(textline->lastChar() + 1);

-      if (lastChar > -1)
-      {
-        stream << QConstString (textline->text(), lastChar+1).string();
-      }
-    }
-    else // simple, dump the line
-      stream << textline->string();
+    stream << textline->string();

     if ((i+1) < m_lines)
       stream << eol;
diff -Naur kate/part.old/katedocument.cpp kate/part/katedocument.cpp
--- kate/part.old/katedocument.cpp      2007-01-15 12:33:48.000000000 +0100
+++ kate/part/katedocument.cpp  2007-05-15 22:05:24.000000000 +0200
@@ -2556,6 +2556,14 @@
         m_buffer->setHighlight(hl);
     }

+    // re-position cursor if trailing blanks have been removed
+    uint cursorLine = m_activeView->cursorLine();
+    uint lineLength = m_buffer->plainLine(cursorLine)->length();
+
+    if(m_activeView->cursorColumn() >= lineLength) {
+      m_activeView->setCursorPosition(cursorLine, lineLength);
+    }
+
     // read our vars
     readVariables();
   }
Comment 29 Davorin Učakar 2007-05-16 10:48:15 UTC
There's one more problem with KateView::cursorColumn:

If the cursor is beyond last non-blank character in the line and the trailing blanks are removed from buffer, (KateDocument object)->activeView->cursorColumn() returns corrected position (end of the line). However, cursor is still visible at the old position, beyond end of the line.
(My patch takes that in account.)

BTW, I think the "if" clause in KateView::cursorColumn takes no sense. It seems the condition is always false, otherwise it would fix that problem.
Comment 30 Christoph Cullmann 2007-05-17 14:11:01 UTC
1. this behaviour should not be changed for 3.5.x, this was intented...
2. if we change the behaviour, then for KDE 4, provide some patch for it and I will review and add it, if it works.
Comment 31 Matthew Woehlke 2007-05-17 17:29:54 UTC
> 1. this behaviour should not be changed for 3.5.x, this was intented...

Do you mean not (visibly) removing trailing spaces, or the cursor problem in comment #29?
Comment 32 Christoph Cullmann 2007-06-03 13:48:25 UTC
The first, therefor this bug is now closed. The current behaviour is the intended one, perhaps this should all be changed for KDE 4, but than in a completly different way:

There should be two options:
1. remove trailing spaces on load
2. remove trailing spaces on editing

Then the document should be always consistent....
But this is a wish, no bug...
Comment 33 Matthew Woehlke 2007-06-06 18:04:58 UTC
Well, it's technically a regression if you look at it as "something that used to work, and now doesn't" :-).

I still think the old behavior was better; the editor state immediately after a load or save should IMO match what is on disk. This used to work, now it doesn't, and 'remove on load' would also not satisfy this expectation.

If I may ask, what was the reason for changing this?
Comment 34 Davorin Učakar 2011-12-24 17:13:42 UTC
Here's patch for 4.7.95:

diff -aur a/part/buffer/katetextbuffer.cpp b/part/buffer/katetextbuffer.cpp
--- a/part/buffer/katetextbuffer.cpp    2011-09-26 11:44:23.000000000 +0200
+++ b/part/buffer/katetextbuffer.cpp    2011-12-24 18:04:08.731525585 +0100
@@ -709,13 +709,11 @@
     if (m_removeTrailingSpaces)
     {
       int lastChar = textline->lastChar();
-      if (lastChar > -1)
-      {
-        stream << textline->text().left (lastChar+1);
-      }
+      if( lastChar >= 0 )
+        textline->textReadWrite() = textline->text().left(textline->lastChar() + 1);
     }
-    else // simple, dump the line
-      stream << textline->text();
+
+    stream << textline->text();

     // append correct end of line string
     if ((i+1) < m_lines)
Comment 35 Davorin Učakar 2011-12-25 16:15:33 UTC
Created attachment 67098 [details]
Remove trailing blanks from document on saving

Sorry for my previous patch, it didn't work as expected.

I fixed it, this one one also handles undo properly.

BTW: could you check if the same problem described in KateDocument::removeTrailingSpace() could occur here and cause a crash?
Comment 36 Dominik Haumann 2011-12-28 13:37:38 UTC
Davorin: Cool patch. I think we do not get a crash. I was not able to reproduce. And the undo-manager is never active on save.

I've tested it and it's ok to commit.

Christoph: Is this ok with you?
Comment 37 Dominik Haumann 2011-12-28 13:45:21 UTC
@Davorin: Another thing: It would be nice, if you could refactor your patch, so that it's in an own function called KateDocument::removeTrailingSpaces();
This way, KateDocument::saveFile() does not get even longer.
Comment 38 Davorin Učakar 2011-12-28 17:52:18 UTC
Created attachment 67194 [details]
Version that does not move cursor

My previous patch has one problem: if you scroll the window so the cursor is not visible and save the file, the window centers at the cursor.

I think you should first agree about which cursor position should be saved into undo stack. IMHO it's nice to save it at the end of the last line there blanks were removed (that's how Eclipse does it). However, that's not consistent with "Replace All", which saves cursor at the current position, not at the last replacement (maybe that should be fixed?).

I've also made a patch that doesn't move cursor, but fixes the scrolling problem.
Comment 39 Davorin Učakar 2012-02-18 18:31:22 UTC
@Dominik: it doesn't make KateDocument::saveFile() significantly longer, I don't think it's worth splitting.

I've been using my last patch for almost two months now. It works perfectly for me.

Changing cursor positioning behaviour for undo is probably not an option. "Replace All", KDevelop refactoring & code generating also save current cursor position. I guess changing that behaviour would need changes in most applications that use katepart.
Comment 40 Gregor Tätzner 2012-05-13 15:11:11 UTC
Git commit 8dcd41626a6d1daf9a94673b7c133e03d28a06ea by Gregor Tätzner.
Committed on 13/05/2012 at 16:58.
Pushed by gregort into branch 'master'.

Remove trailing whitspace on saving.

Iterate through text buffer whenever the save function is called and
remove all trailing spaces.

Thanks to Davorin Učakar for this patch!

M  +1    -11   part/buffer/katetextbuffer.cpp
M  +29   -0    part/document/katedocument.cpp
M  +12   -7    part/document/katedocument.h

http://commits.kde.org/kate/8dcd41626a6d1daf9a94673b7c133e03d28a06ea
Comment 41 Dominik Haumann 2012-05-14 21:19:20 UTC
This patch can stay now in trunk but it should be discussed before the 4.9 release whether we should revert it again.

Now, whenever you save the trailing spaces are removed and the edit actions are added to the undo-stack. In theory, you can undo this change. But in practice, saving will again remove these, which render it rather a "you cannot really undo" feature.

Further, in KDE 4.9 there is an option called "Append newline at end of file". This also doesn't add it to the buffer but only when writing the file to disk.

I'm aware of the 270 votes for this "bug", but this doesn't say anything about all the Kate users who are happy with the current behavor.
Comment 42 Dominik Haumann 2012-05-18 10:55:57 UTC
Git commit 39da06bc1dbeb07b6e018edcb8ef5434a5c6e35a by Dominik Haumann.
Committed on 18/05/2012 at 12:52.
Pushed by dhaumann into branch 'master'.

correctly implement remove-trailing-spaces on save

@Gregor: one should only use the edit*() functions to modify the document content
@Allen: thanks for the report!
Related: bug 300209

M  +12   -17   part/document/katedocument.cpp
M  +1    -1    part/document/katedocument.h

http://commits.kde.org/kate/39da06bc1dbeb07b6e018edcb8ef5434a5c6e35a