bugGNU Octave - Bugs: bug #45610, wrong commenting (ctrl+R) behavior...

 
 

bug #45610: wrong commenting (ctrl+R) behavior with CRLF eols in GUI editor

Submitter:  Hartmut <hardy>
Submitted:  Thu 23 Jul 2015 06:54:16 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 25 Nov 2017 09:12:06 AM UTC, comment #32: 

Thanks for testing. I have forgotten to post, that changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/a9db5eed8fce
introduces the suggetsed behavior from comment #30. I think it is more intuitive than the previous one and does not rely on the line length. I close this report and will graft the changesets to stable.

Torsten Lilge <ttl>
Group Member
Sat 25 Nov 2017 08:16:12 AM UTC, comment #31: 

I have compiled a new dev version (a9db5eed8fce) under Linux and tested it (as in comment #28. Result: It works like expected. Fine.

Hartmut <hardy>
Thu 23 Nov 2017 09:35:50 PM UTC, comment #30: 

I have not yet closed the report because I am wondering whether it it would be better to restore the original selection instead of selecting the complete lines, which are commented/uncommented. I have tested some other editors and they keep the selection.

Torsten Lilge <ttl>
Group Member
Fri 10 Nov 2017 05:48:29 PM UTC, comment #29: 

Hartmut, thank you for testing.

Torsten Lilge <ttl>
Group Member
Fri 10 Nov 2017 12:22:16 PM UTC, comment #28: 

I can confirm that the patch from comment #27 solves this issue (at least on linux).

Here is what I did:

  • I've compiled dev Octave (hg id 629858e122f1 with Torsten's patch)
  • make check still passes (15839 Pass, 0 Fail, 5 Xfail expected, 31 Xfail reported, 62 Skipped)
  • I opened three different m-files in the GUI editor, with all possible line endings.
  • Then, I've marked several lines with the mouse and commented them via Ctrl+R, and afterwards un-commented them with Shift+Ctrl+R. The number of marked lines stayed constant.
  • For comparison, I've done the same with Octave 4.2.1 release, and with Windows line endings, the number of marked text lines did NOT stay constant.


My tests have only been on a linux PC. I have NOT done any tests with an mxe-octave on a Windows PC.


Hartmut <hardy>
Thu 09 Nov 2017 08:14:01 PM UTC, comment #27: 

I have pushed a patch with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/629858e122f1
which solves this issue for me.

Torsten Lilge <ttl>
Group Member
Mon 03 Jul 2017 06:34:07 PM UTC, comment #26: 

I have interest in this as well. I ran into the same problem as indicated in #50547 which was closed as duplicate of this one. I noticed shift-ctl-tab continuously selects more lines when un-indenting if more than one line is initially selected. Sure, shift-tab (or tab) works when multiple lines selected, but then one needs to remember and use separate commands for indent/un-indent depending on whether a single line or multiple lines are selected. I had originally overriden the default key command for un-indent to shift-tab instead of shift-ctl-tab so the same command was used for a single line or multiple selected lines, but then multiple line selections begins to expand on each subsequent use.

Marshall <marsian>
Wed 15 Mar 2017 09:15:21 PM UTC, comment #25: 

From bug #50547 (marked as duplicate of this one), it can be concluded that the selection is generally expanded by one line when using a shortcut with modifier on selected text in files with CRLF line endings. When the selection is commented via the menu entry, everything works as expected.

Torsten Lilge <ttl>
Group Member
Mon 26 Sep 2016 07:25:09 PM UTC, comment #24: 

Sorry, I lost track of this one.  Let me read a bit and get up-to-date with the past few posts.  I'll look into the Qscintilla discussion list as well.  Neither do I remember any sort of response.  Based on that, maybe we should simply create a workaround, but document it well so that if Qscintilla is altered for consistent behavior we know how to correct the future bug.

Dan Sebald <sebald>
Mon 26 Sep 2016 07:05:07 PM UTC, comment #23: 

I rebuilt (on Linux) with the "#ifdef WIN32" / "#endif" lines commented out and found that then the bug seems solved on Linux as well, w/o side effects for CR and LF eols.

But:
I do not understand Dan's patch well - that is, I do not understand how Qscintilla determines the line length and if unconditionally subtracting 1 from it in any case (= also for LF & CR eols) might have any nasty side effects.
So I'm unsure if Dan's patch works for the wrong reason, so to say.

All I can see is that it works.

Dan? Mike?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 25 Sep 2016 09:34:44 PM UTC, comment #22: 

I haven't found a proper bug tracker for Qscintilla. There is one for Scintilla, but this problem seems to be specific to the Qt port. (At least this seemed to be Dan's judgement on this.)

What I found is Dan's question on the Qscintilla discussion list: https://riverbankcomputing.com/pipermail/qscintilla/2015-July/001062.html And there seems to be no answer to Dan's question for the last 14 months now.

So I would assume that we cannot change the behavior of Qscintilla. Therefor my opinion would be to work around this Qscintilla behavior.

Hartmut <hardy>
Sun 25 Sep 2016 06:47:01 PM UTC, comment #21: 

Hartmut,

Even when the GUI editor had no line ending option this bug was there, i.e., when reading .m files with CRLF endings. But admittedly it was a very concealed bug at that.

Maybe you can browse the Qscintilla bug tracker (if it exists at all, I do not know) for a relevant bug report?

I think I'll enter a bug report for the comment character.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 25 Sep 2016 06:06:52 PM UTC, comment #20: 

I can confirm, that this behavior is now also present in the (4.0.3) linux built, if the edited file uses CR-LF line endings (what I really mostly do to have files that I can also use on the less flexible Windows side).

Sorry for not beeing "precise" here. At the time when I tested Dan's patch (comment #14) there wasn't the possibility in the options of the GUI editor to switch line endings, yet. So at that time Dan's patch fully solved the issue. But this isn't the case any more.

So I would also opt for a broader solution, now. Some patch that solves this issue for Linux and for Windows builts would be nice. (The switch should now probably be between "different line feed settings", compared to Dan's patch with a switch at compile time between "different OS".)

I can't judge if it's worth to wait for a fix in upstream Qscintilla. Knowing if there's a formal Qscintilla bug report anywhere would really help with this decision, and make me vote for waiting a bit longer. (But I tend to prefer a workaround solution here, since it seems to take very long for Qscintilla to react here.)

Note: Having a user option for the comment character (% or #) in the GUI editor would be nice, too. But that should probably go to a different "feature request" bug report.


Hartmut <hardy>
Sun 25 Sep 2016 08:15:21 AM UTC, comment #19: 

To be precise, this only happens with "CRLF" endings on any operating system. With "LF" EOLs (mostly used in Octave) and "CR" EOLs the bug isn't there.

Dan's patch works, but only for Windows builds. It doesn't fix the issue for Linux builds (I just tried) - I can imagine scenarios where someone edits Matlab scripts from the Windows world on Linux that go back to Matlab on Windows in a later stage.

From the URL in comment #13 it seems this Qscintilla bug has been there for a decade now. Do we still have hope it'll ever get fixed upstream?  Is there any formal upstream Qscintilla bug report at all, or only the forum thread mentioned in comment #13?

As an aside, I sometimes wonder why Ctrl-R introduces "%" rather than "##" comments, as the latter is he convention in Octave. Did I overlook a setting somewhere?

(==> title, OS & release adapted)


Philip Nienhuis <philipnienhuis>
Group Member
Sat 24 Sep 2016 08:57:48 PM UTC, comment #18: 

This bug is still present in Octave-4.2.0-rc2 (official Windows installer from alpha.gnu.org).

There is a tiny and tested PATCH from Dan for more than a year now (see comment #13), could we go ahead and use it for the 4.2 release?

Hartmut <hardy>
Sat 21 Nov 2015 03:20:22 PM UTC, comment #17: 

@Dan: Can this bug be closed and marked as fixed upstream?  Or do we need to patch Octave to work around Qscintilla?

Rik <rik5>
Group administrator
Thu 24 Sep 2015 07:32:22 PM UTC, comment #16: 

@Dan: Have you heard something from the QScintilla people in the meantime? Or should we just go ahead and apply your patch to fix this issue?

Hartmut <hardy>
Tue 28 Jul 2015 06:54:15 AM UTC, comment #15: 

Great!  Thanks for testing.  It will take a few days for someone to apply the changeset.  In the mean time, if I hear anything from the QScintilla discussion list I'll make note of it here.

Dan Sebald <sebald>
Mon 27 Jul 2015 08:36:26 PM UTC, comment #14: 

I've compiled and tested Octave with this second patch (jul27) on both Windows 7 and Ubuntu 14.04.

The result is: It works totally fine on both OS :)

If you for example mark the text "ne3 line4 lin" on either OS, then after pressing Ctrl+R you get "%line3 %line4 %line5 " as marked text, the line ends are properly marked as well.

Hartmut <hardy>
Mon 27 Jul 2015 07:51:39 AM UTC, comment #13: 

OK, let's just pre-processor conditionally remove 1 from the length for WIN32.  (I believe WIN32 is the correct flag to condition on because of this code/comment

http://www.qtcentre.org/threads/2229-Carriage-Return-in-QString?p=161439#post161439

).  The reason I think doing so is fine is because of a comment I put in the changeset:


      // QScintilla's text() creates a QString for which CR or CR+LF is/are
      // part of the string and hence included in Qt's QString length().
      // However, QScintilla's setSelection() indexing apparently ignores
      // the CR or CR+LF.  Hence, on Windows the length is too long by 1.


For example, say the line of text is "line 3".  Well, after % placement we have:

LINUX: %line 3<CR>
WIN32: %line 3<CR><LF>

In the Linux case, the length is 8.  (QString includes <CR> which goes toward the count.)  In the Windows case, the length is 9.  But then, because QScintilla ignores the <CR> or <CR><LF>, the 8 in the Linux case puts the cursor at the first character of the next line while the 9 in the Windows case puts the cursor at the second character of the next line.

To me, that seems a little inconsistent on part of QScintilla.  As I see it, QScintilla's "text(lineTo)" should not be placing a <CR> or <CR><LF> in the QString.  I've sent an email to the QScintilla list to see what their thoughts are on this, so hold off applying the new changeset.  But feel free to test it, of course.

(file #34532)

Dan Sebald <sebald>
Sun 26 Jul 2015 09:57:55 PM UTC, comment #12: 

I now have the building environment for a Windows version of Octave up and running :) It took hours to compile... I hope the second compilation round goes significantly faster.

Regarding your first patch to this bug:
On Windows the behavior is totally fine with this patch! If you mark for example "ne3 line4 lin" and then press Ctrl+R, then you get the following text marked "%line3 %line4 %line5 ". The space sign at the end is ment to show you that all line end characters are properly included into the marked text region. Pressing Ctrl+R several times, only indents the text region further, with no additional lines. If you want me to do other tests on the patched Windows version, just let me know.

So my conclusion would be: This patch is totally fine for the Windows version. But it does have not so nice side effects on the Linux version.

Hartmut <hardy>
Sat 25 Jul 2015 04:03:33 PM UTC, comment #11: 

Regression:  OK, there are a couple approaches to take.  One would be a per-processor conditional that would leave the code as it is for everything by Windows and for the Windows condition subtract 1 from the index.  A second approach would be to get a little more detailed about exactly where the end of the highlight goes, e.g., right after the end of the CR or LF ending the last line, unless at the EOF character.  We could experiment with both.  I'll look at this later this weekend and maybe by then you'll have the MXE patch process working.

Dan Sebald <sebald>
Sat 25 Jul 2015 11:28:30 AM UTC, comment #10: 

Assuming you are building stable 4.0.0 with the patch, you should be able to copy the patch to a file stable-octave-1-patchname.patch to the mxe-octave/src directory before running make and it should apply in during the build.


John Donoghue <lostbard>
Group Member
Sat 25 Jul 2015 08:53:55 AM UTC, comment #9: 
  • test on windows: Yes, I've read all these wiki pages  for mxe-octave. I would probybly be able to compile a Windows installer from source with this mxe-octave repository (but haven't actually done it). But I have found no instructions for how to make mxe-octave use a LOCAL octave repository (with your patch applied), normally mxe-octave seems to always download a fresh clone of the official octave repo (stable, default) and cross compile this. And the official octave repo obviously doesn't have this patch included, yet.


  • test on Linux: Yes, you are right. I had only tested that the Ctrl+R behavior on Linux is not disturbed by your patch. But now I realised that the scope of this patch is much wider. And it DOES change the behavior of marking whole text lines. I personally would consider this a significant regression regarding usability under Linux. So this patch DOES have side effects :(
Hartmut <hardy>
Fri 24 Jul 2015 11:08:36 PM UTC, comment #8: 
Dan Sebald <sebald>
Fri 24 Jul 2015 11:02:42 PM UTC, comment #7: 

I'm pretty clueless with the Windows build.  I'll ask the discussion list.

I tried the change on Linux as well.  I did notice that the highlighted region no longer included the "blank" or "wrap around" space at the end of the last line.  That's to be expected, given the change, but I'm a little concerned that it might affect the user's preferred behavior of line select.  For example, I've always preferred that the last CR be included in the selected text so that when Cntrl-X or yank is done, all the lines are removed and there isn't a blank line left behind.  Maybe I'm describing that well, but think of placing the cursor at the front of a line and then use shift-downarrow to select whole lines.  If more subtle behavior is desired for the way the lines are selected, we might need to alter the programming slightly.

It's odd though that Windows behaves a bit differently than linux.  Let's see what response is to this change first.

Dan Sebald <sebald>
Fri 24 Jul 2015 09:42:54 PM UTC, comment #6: 

I have applied your patch, and compiled Octave for my Ubuntu Linux (= not Windows) with it. Here I verified that your patch does not have any side effects on the Ctrl+R behavior on Linux.

But I am currently not able to compile mxe-octave with a patched Octave version for Windows. If you tell me an easy way to do this, I could give it a try, though.

So sorry, I am currently unable to tell you if your patch has the intended positiv effect on the Ctrl+R behavior under Windows.

Hartmut <hardy>
Thu 23 Jul 2015 10:17:55 PM UTC, comment #5: 

Please try the attached changeset.


(file #34495)

Dan Sebald <sebald>
Thu 23 Jul 2015 09:23:15 PM UTC, comment #4: 

When I highlight "ne2 line3 line4 lin" and then do the Ctrl+R, the following text is marked as a result: "%line2 %line3 %line4 %line5 l". This means I always get only the very first character of the following line as "bonus".

Hartmut <hardy>
Thu 23 Jul 2015 09:09:29 PM UTC, comment #3: 

The highlight / Ctrl+X / Ctrl+V was meant to illuminate pretty much what you did with the first example.  So yes, the highlighted text after the Cntrl+R operation is too much and not computed correctly.  That should be enough info to work with.  Thanks.

One other thing you could try is increase the number of lines.  That is, highlight "ne2 line3 line4 lin" and do the Ctrl+R.  Is the result

"%line2 %line3 %line4 %line5 li"

?  Or

"%line2 %line3 %line4 %line5 l"

?

Dan Sebald <sebald>
Thu 23 Jul 2015 07:44:31 PM UTC, comment #2: 

Here is what I observe (on Windows, Octave 4.0.0):

  • mark the presiously cited lines like this: "ne2 line3 lin". Then press Ctrl+R. result: the markes text region is extended to "%line2 %line3 %line4 l". So the last "l" is too much!
  • I didn't understand what you wanted me to test with the Ctrl+X, Ctrl+V procedure. After inserting text with Ctrl+V NOTHING is marked any more. And Ctrl+X cuts out all the marked text. I don't observe any unexpected behavior here, neither with Ctrl-C followed by Ctrl-V, nor by Ctrl-X followed by Ctrl-V.
  • If I mark two full lines plus half a line like this "line2 line3 lin", then after pressing Ctrl-R the following text is marked: "%line2 %line3 %line4 l". So as observed before, the first character of the following line is wrongly added to the marked text area.
Hartmut <hardy>
Thu 23 Jul 2015 07:21:35 PM UTC, comment #1: 

This is probably a one line fix of sorts--something like an index going one line further than necessary or the highlighted text including an extra carriage return.

Could you watch closely at what happens to the highlighted region when you do those steps again?  When the Cntr-R is typed, does the highlight wrap past the next line?  Do a Cntrl-X or Cntrl-C followed by Cntrl-V and try to discern exactly what region is highlighted.  Is it lines 3, 4 and 5?  Or maybe 3, 4, 5 and the CR right before line 6?  Rather than select all of line 5, could you maybe select line 3, 4 and half of line 5?  Try Cntrl-R again and see what happens to the highlight region.

Ah, just thinking here, if this is a Windows only issue, it could be the CR/LF problem again.  That is, the operation of placing more text at the front of the highlighted lines means adjusting the start and stop indeces of the highlighted text.  Maybe that isn't being computed correctly on Windows (whereas on linux the number of extra characters matches the number of lines).

Dan Sebald <sebald>
Thu 23 Jul 2015 06:54:16 PM UTC, original submission:  

Here is how to reproduce the behavior:

  • Use Windows (I used Windows 7, it does not happen under Ubuntu Linux) with Octave 4.0.0 (official release)
  • Type some lines text into the window of the GUI editor, let's say 8 lines. For example this text:


line1
line2
line3
line4
line5
line6
line7
line8

  • mark some lines with the mouse, let's say line 3 to 5.
  • Press "Ctrl+R" three times to add three comment marks in front of all these marked lines.
  • Look at the result, in my case I get the following:


line1
line2
%%%line3
%%%line4
%%%line5
%%line6
%line7
line8


As you can see, with each usage of the keyboard shortcut "Ctrl+R" the marked text region becomes larger by one line. So with the second keyboard shortcut, one more line is commented out. And with the third keyboard shortcut, even a second additonal line is commented out.

This behavior was very surprising to me. And I normally use keyboard shortcuts to work faster, and then mostly didn't realize that I commented out more lines than I intended to. This can really screw up your code :(

Can someone reproduce this? Can we fix this?

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34532:  octave-cntl+R_bug45610-djs2015jul27.patch added by sebald (1KiB - application/octet-stream)
file #34495:  octave-cntl+R_bug45610-djs2015jul23.patch added by sebald (1006B - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by marsian (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by hardy (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-25 ttl StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-11-09 ttl StatusIn Progress Ready For Test
    2017-03-15 rik5 Carbon-CopyRemoved 72865 -
    2017-03-15 ttl Dependencies- bugs #50547 is dependent
    2016-09-25 philipnienhuis Release4.0.0 dev
        Operating SystemMicrosoft Windows Any
        Summarywrong commenting (ctrl+R) behavior in GUI editor under Windows wrong commenting (ctrl+R) behavior with CRLF eols in GUI editor
    2015-11-21 rik5 StatusNone In Progress
    2015-07-27 sebald Attached File- Added octave-cntl+R_bug45610-djs2015jul27.patch, #34532
    2015-07-23 sebald Attached File- Added octave-cntl+R_bug45610-djs2015jul23.patch, #34495

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code