bugGNU Octave - Bugs: bug #43334, [GUI] Editor inserts CR LF rather...

 
 

bug #43334: [GUI] Editor inserts CR LF rather than just LF

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Tue 30 Sep 2014 08:01:11 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  ttl
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
Release:  * 3.8.2 Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 12 Oct 2014 02:38:05 PM UTC, comment #13: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/d10c711a08d8
to the gui-release branch which adds a drop down menu with the eol-modes to the save-file-as-dialog.

Closing the report.


Torsten Lilge <ttl>
Group Member
Fri 10 Oct 2014 08:50:10 PM UTC, comment #12: 

@Torsten: Just to let you that even with the original bug fix my work debugging io package functions became a lot easier now (I'm using 3.9.0+ on MinGW). Thanks!

Philip Nienhuis <philipnienhuis>
Group Member
Thu 09 Oct 2014 04:25:17 PM UTC, comment #11: 

I have backed out the cset from the gui-release branch and pushed the slightly modified version with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/c766a1f63c40
The only difference is that no strings that has to be translated are included in the patch. The eol mode is only indicated by the strings "CR", "LF" or "CRLF" which don't necessarily have to be translated.

I have already prepared some changesets that add preferences and actions related to the eol mode. Is it possible to cherry-pick the changeset into the gui-release branch (hg graft) or would this cause trouble next time when merging from stable into gui-release?

Torsten Lilge <ttl>
Group Member
Tue 07 Oct 2014 07:45:34 PM UTC, comment #10: 

Since the patch of comment #7 also include changes in the text output (status bar) this patch should land in the gui-release branch. What I can do is to backout the changeset and create a changeset only containing the detection of the eol mode. After the next merge of stable into gui-release (or is it possible to push the changeset in different branches?) the gui parts (status bar, default mode in preferences, preferences for showing eol chars, actions for converting ...) can be added.

I would also prefer menu entries for converting eol chars rather than "hiding" them in a "save as" dialog.

Torsten Lilge <ttl>
Group Member
Tue 07 Oct 2014 02:34:54 PM UTC, comment #9: 

Can the patch of comment #7 be pushed to stable as well?
After all this is a sneaky bug that gave at least me lots of extra work.

AFAICS the behavior after the bug fix would be very transparent, logical, and the thing that the far majority of unwary users would expect. There aren't that many files that have mixed EOL types and if they need cleaning up there's always dos2unix & unix2dos to make a fix so that next time the file is edited in Octave the problem is avoided in the first place.

As to options menus, e.g. notepad++ has configurable line endings for new files.
If EOLs need to be changed I'd rather have it in the Edit menu of the editor.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 06 Oct 2014 10:43:39 PM UTC, comment #8: 

As a gedit user, I guess I'm biased but I think an option on the save menu is more obvious.

The only point during the whole file editing process where it matters what EOL to use is when saving the file.  It goes hand-in-hand with an option to choose the encoding. An entry on the preferences menu to choose a default seems good but an extra entry on the menus seems to much and we'll get menus that are too bloated if adding too many things.

I have attached a screenshoot of the "Save as" menu in gedit (note the discrete "Line ending" menu at the bottom of the window).


Carnë Draug <carandraug>
Group Member
Mon 06 Oct 2014 08:04:37 PM UTC, comment #7: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/85b2d669c21e
The line endings of a loaded file are counted and the eol-mode of the editor is set to the one with the highest count and indicated in the status bar.

What is still missing and what I am planning to implement within the next days:

  • menu entries for changing and converting to another eol-char
  • a preference for the default eol-char for new files

which is currently set depending on the OS

I am not sure if we need a drop down menu in the save dialog when menu actions for the conversion are available.

Torsten Lilge <ttl>
Group Member
Sun 05 Oct 2014 06:56:18 PM UTC, comment #6: 

What would the behaviour be if the file already has a mix of EOL?

Also, a "Line Ending" dropdown menu on the "Save as..." window would be very useful (see gedit for example).

Carnë Draug <carandraug>
Group Member
Sun 05 Oct 2014 05:44:51 PM UTC, comment #5: 

I can confirm this. Qscintilla sets the eol-mode only depending on the OS without checking the loaded file. It is also useful to have a default eol-mode for new files in the preferences.

Torsten Lilge <ttl>
Group Member
Sat 04 Oct 2014 09:09:23 PM UTC, comment #4: 

I found Rafael's complaint:
http://savannah.gnu.org/bugs/?42513, last paragraph in OP

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Oct 2014 08:58:25 PM UTC, comment #3: 

Well I figured that would be obvious.

I'm doing development (mostly io package and some core Octave stuff) on both Windows and Linux.

1. I think no serious editor should be allowed to mix up different EOL types in one file. As far as I'm aware Octave's GUI editor is the only one who does this. (I deem Windows' notepad.exe not a serious editor)

2. Having mixed LF / CR-LF EOLs gives lots of extra lines diffs only caused by different EOL types.

3. Some months ago Rafael Labiossierre stressed me to make sure there are only LF EOLs in the io package as it gave him lots of extra work preparing Debian packages. (I can't find the post right now, was in spring IIRC)
At the time I figured those CRLFs were my fault - as I didn't know where they came from I usually ran Octave files through dos2unix just to make sure EOLs are LF only. Yet I think that sort of extra steps shouldn't be required.
Only now it is clear how those CRLF crept in and I conclude that Octave's GUI editor must have had this bug for a very long time,

I know from your comments in bug #41281 that Octave may be oblivious to EOL types; but version control systems and packaging systems are less immune.

Is this answer sufficient to convince you?

Philip Nienhuis <philipnienhuis>
Group Member
Sat 04 Oct 2014 06:55:58 PM UTC, comment #2: 

What trouble does this cause?  You are editing files on a Windows system, after all.  Isn't the expected EOL sequence on Windows systems CRLF?

John W. Eaton <jwe>
Group administrator
Sat 04 Oct 2014 04:03:29 PM UTC, comment #1: 

I found that CRLFs rather than LFs are inserted when a new line is inserted in existing files.
The CRLF sequence is put right before the new line (= where the cursor was before the <Return> key was hit).

Philip Nienhuis <philipnienhuis>
Group Member
Tue 30 Sep 2014 08:01:11 PM UTC, original submission:  

I find that on Windows the GUI editor inserts CR-LF EOLs when editing .m-files.

IMO when editing Octave .m-files no CR-LF EOLs should inserted.

There may be a case for code that should also run in Matlab; so perhaps the editor's line endings should be made configurable.
Or maybe the editor should detect EOL type when reading the file (but then still a choice must be made for new files).

If/when this is fixed I suggest to fix it on stable as well (I hit it on 3.8.2 but it could well be in 3.8.0 too)

Philip Nienhuis <philipnienhuis>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-10-12 ttl StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2014-10-06 carandraug Attached File- Added gedit-saveas-window.png, #32235
    2014-10-06 ttl StatusNone Ready For Test
    2014-10-05 ttl Assigned toNone ttl

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code