gnudatalanguage / gdl

GDL - GNU Data Language
GNU General Public License v2.0
274 stars 61 forks source link

WIDGET_CONTROL, SET_VALUE=..., /APPEND fails if repeated #1743

Closed klimpel closed 7 months ago

klimpel commented 7 months ago

The idl code WIDGET_CONTROL, SET_VALUE='abc', status WIDGET_CONTROL, SET_VALUE='def', status, /APPEND WIDGET_CONTROL, SET_VALUE='g', status, /APPEND fails with the error message "InterpreterLoop: Exception: basic_string::replace: __pos (which is 8) > this->size() (which is 4)".

At this point the wxTextCtrl shows "abc def " with the empty beginning of the third line marked in blue.

The idl code WIDGET_CONTROL, SET_VALUE='abc', status WIDGET_CONTROL, SET_VALUE='def', status, /APPEND WIDGET_CONTROL, SET_VALUE='g', status, /USE_TEXT_SELECT fails in a nearly identical way, the only difference being "pos (which is 9)" instead of "pos (which is 8)" in the error message.

But the idl code WIDGET_CONTROL, SET_VALUE='abc', status WIDGET_CONTROL, SET_VALUE='def', status, /USE_TEXT_SELECT WIDGET_CONTROL, SET_VALUE='g', status, /APPEND does not fail.

The error gets thrown by the line lastValue.replace(from,to-from,value); in GDLWidgetText::InsertText. The problem seems to be that "lastValue" somehow gets reset to its previous value in case of /APPEND. But before it got written to wxTextCtrl, and had the correct expected value in that moment. No idea how that happens. There is a SetLastValue member function, but no simple connection to /APPEND. Maybe the "txt->SetSelection" call caused by /APPEND triggered anunfortunate wxWidgets event.

The problem occurs on Windows, both with the 1.0.4 release, and with the latest "Weekly Binary Release (unstable)".

GillesDuvert commented 7 months ago

Thanks @klimpel I'm glad to see widgets used and tested, as we cannot run automated checks on them. This kind of test explores the uncharted parts of IDL... Will be corrected ASAP.

klimpel commented 7 months ago

I have now built gdl myself. Indeed, txt->SetSelection triggered a wxEVT_COMMAND_TEXT_UPDATED event (why???), which got handled by gdlwxFrame::OnText as requested: this->AddToDesiredEvents( wxEVT_COMMAND_TEXT_UPDATED, wxCommandEventHandler(gdlwxFrame::OnText),text);

I have fixed it in my local version by avoiding txt->SetSelection:


+  txt->GetSelection(&from, &to);
   //really append/replace
   if (append) { //see discussion wxTextEntry::GetInsertionPoint() at https://docs.wxwidgets.org/trunk/classwx_text_entry.html
     if (multiline) {
-      txt->SetSelection(pos-1,pos);
+      from = pos-1;
+      to = pos;
     } else {
-      txt->SetSelection(pos,pos);
+      from = pos;
+      to = pos;
     }
   }
-  txt->GetSelection(&from, &to);
   bool doNotAddNl=(noNewLine_ || (!multiline) );

In my local version, I also modified the call to replace, to avoid the exception even if lastValue would still be wrong:

   int insertedLength=value.size();
-  lastValue.replace(from,to-from,value);
+  lastValue.replace(std::min(size_t(from),lastValue.size()),to-from,value);
   //recompute nlines, maxlinelength from start to be sure
GillesDuvert commented 7 months ago

@klimpel I've tested on my computer and this behaviour is not present. I'm unable to reproduce. I use the general test procedure in "testsuite/interactive_tests/test_widgets.pro", started specifically with these options test_widgets,/col,present="TEXT" And I send your commands to the text widget id=46 (e.g., status=46) which is an editable, /ALL_EVENTS widget.

So perhaps you are in a different setup. May I have a complete sequence of your text widget creation?

klimpel commented 7 months ago

started specifically with these options test_widgets,/col,present="TEXT" And I send your commands to the text widget id=46 (e.g., status=46) which is an editable, /ALL_EVENTS widget.

I tested with test_widgets,/col,present="TEXT" and status=46 now, and still get the same behavior. Or nearly the same behavior, namely the first /APPEND causes the following message:

GDL> ** Structure WIDGET_TEXT_DEL, 6 tags,memsize =24, data length=22/22:
   ID              LONG                46
   TOP             LONG                 1
   HANDLER         LONG                 1
   TYPE            INT              2
   OFFSET          LONG                 2
   LENGTH          LONG                 6
(unhandled event: ok)

The second /APPEND still produces the exact same error message as in my setup.

So perhaps you are in a different setup. May I have a complete sequence of your text widget creation?

pro WidgetAppend
base = WIDGET_BASE(TITLE='Text Widget to demonstrate /APPEND bug',/COLUMN,XSIZE=620,/ALIGN_LEFT,/TLB_MOVE_EVENTS)
block1 = WIDGET_BASE(base,/FRAME,XSIZE=610,/ALIGN_CENTER,/COLUMN)
status= WIDGET_TEXT(block1,XSIZE=90,YSIZE=15,/SCROLL,VALUE=' ',/ALIGN_LEFT)
WIDGET_CONTROL,base,/REALIZE
XMANAGER,"WidgetAppend",base,/NO_BLOCK

WIDGET_CONTROL, SET_VALUE='abc', status
WIDGET_CONTROL, SET_VALUE='def', status, /APPEND
WIDGET_CONTROL, SET_VALUE='g', status, /APPEND
end

My computer runs Windows 10 Pro, 22H2, OS build 19045.3930, but I remember that I also tried on Windows 11 and Windows Server 2022 with my original setup, and got the same error message.

GillesDuvert commented 7 months ago

Ah, so it would be a Windows port problem only. wxWidgets do not completely react the same on all platforms (and that's quite understandable). I'm more than happy to include your fix, just have to check it does not perturb the OSX and Linux ports. TBC, and thanks for the fix!

GillesDuvert commented 7 months ago

Patch added in #1750