bugGNU Octave - Bugs: bug #39722, history list adds extraneous...

 
 

bug #39722: history list adds extraneous carriage return when recalling commands

Submitter:  Dan Sebald <sebald>
Submitted:  Wed 07 Aug 2013 11:53:43 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 15 Aug 2013 06:54:57 PM UTC, comment #8: 

You are right; I have pushed the patch with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/7fb4461997aa

Thanks for the patch.

Torsten Lilge <ttl>
Group Member
Wed 14 Aug 2013 05:13:29 AM UTC, comment #7: 

I've thought about that.  You could give it a try if you like.  My feeling though is to fix things in the proper location rather than try to work around it, especially when what needs to be fixed is a main component of the software.

Dan Sebald <sebald>
Wed 14 Aug 2013 04:56:01 AM UTC, comment #6: 

What if we just make one string from the selected lines in the history and only emit one signal with the multi-line-string as argument?

Torsten Lilge <ttl>
Group Member
Tue 13 Aug 2013 08:54:34 PM UTC, comment #5: 

As far as I can tell, after the patch is applied the last command should be evaluated and not leave a text string on the command line.  When a single command is selected and "evaluate" is chosen the command is run, so why not in the list multiselection given the same mechanism is used?:


      if ((*it).isValid())
        emit command_double_clicked ((*it).data().toString());


It's just strange and unpredictable, I think.  When there is a single command, that just goes into the FIFO and then the core takes it out--no obvious problem.  When there are multiple commands, that's when the problem arises.

It could be that the core is inadvertently doing first in, last out queue so that commands are backward or rearranged from the order they go in.  However, the bigger issue is that commands are being lost signifying a mutex problem.

Dan Sebald <sebald>
Tue 13 Aug 2013 08:04:34 PM UTC, comment #4: 

Okay, so let's leave the reordering of commands to bug #39723.

IMHO, in the case where a block of commands from the history is evaluated, the last line should also be evaluated without extra <return> from the user (as this extra <return> is not necessary when evaluating a single command from the history).

When copying a single command or a block of commands, no newline is needed.

Torsten Lilge <ttl>
Group Member
Mon 12 Aug 2013 11:08:53 PM UTC, comment #3: 

I've printed the text that is issued via


        emit command_double_clicked ((*it).data().toString());


and the correct text is appearing in the proper order.  I believe this is a flaw in communications between the GUI and the core:

https://savannah.gnu.org/bugs/?39723

I will bring this up on the discussion list.  I think the evaluate is correct from the GUI's perspective.

Other than that, I think the patch does sensible things.  When a block is copied, there is no CR added after the last line, but I think I prefer that for use in an editor...but perhaps other would disagree.  I think that is something we could leave temporarily to get feedback from beta-users.

We could make the following code a function:


  for (QModelIndexList::iterator it = rows.begin (); it != rows.end (); it++)
    {
      if ((*it).isValid ())
        {
          if (prev_valid_row)
            text += "\n";
          text += (*it).data().toString();
          prev_valid_row = true;
        }
    }


since it is duplicated in the code.

Dan Sebald <sebald>
Mon 12 Aug 2013 08:06:13 PM UTC, comment #2: 

There are some issues when several entries are selected and evaluated via the context menu (also existing before the patch):

  • The last command is printed but not evaluated
  • The order of the selected lines is not preserved when the lines are evaluated and depends on the entry where the context menu is activated


Torsten Lilge <ttl>
Group Member
Thu 08 Aug 2013 01:27:40 AM UTC, comment #1: 

I think I see how these newline characters arose.  When copying many items in the list, there are no carriage returns between lines.  However, the approach of always adding a newline character isn't quite correct.  It is better to add newline characters only between the valid lines.  The attached patch generally does that.

Test all the circumstances.  Select a block of commands in the history and right click "Create script".  You'll see lines in the editor with the last line not having a carriage return.  Select a block of commands and right click "Copy" and then past that in the editor and paste that in the command-line window.  There will again be carriage returns appropriately between commands, but the last command will not be executed until typing return.  I think I like that behavior in the editor... and probably in the command window too.  I'm not too concerned if that aspect of it is changed.  Double clicking on a single line of text in the history window correctly issues a command without the extra spaces.

There is a bug, however.  I think that another reason someone introduced the carriage return sent to Octave core is that somewhere along the way lines of text are being lost in the "emit" of the signal/slot communication.  The extra carriage return may have made that situation happier, but I think we should address that bug in a better way.  I will create a separate bug report.

(file #28790)

Dan Sebald <sebald>
Wed 07 Aug 2013 11:53:43 PM UTC, original submission:  

When double-clicking an item in the history window list the command that appears has an extra carriage return in it.  That carriage return is putting space into the command line and then it also shows up in the history with an extra carriage return.

This behavior leads to this kind of strangeness when one keeps selecting the most recent command in the history buffer:


>>  test assert

PASSES 57 out of 57 tests
>>  test assert


PASSES 57 out of 57 tests
>>  test assert



PASSES 57 out of 57 tests
>>  test assert




PASSES 57 out of 57 tests
>>  test assert





PASSES 57 out of 57 tests
>>  test assert






PASSES 57 out of 57 tests
>>






I will write a short patch that removes this carriage return, but I wonder if the "\n" was added because of something strange someone might have found on a different OS.

Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #28790:  octave-history_buffer_action-2013aug07.patch added by sebald (3KiB - application/octet-stream)

 

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 sebald (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-08-15 ttl StatusNone Fixed
        Open/ClosedOpen Closed
    2013-08-08 sebald Attached File- Added octave-history_buffer_action-2013aug07.patch, #28790

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code