bugGNU Octave - Bugs: bug #55139, m-editor locale failure caused...

 
 

bug #55139: m-editor locale failure caused crash

Submitter:  John Donoghue <lostbard>
Submitted:  Fri 30 Nov 2018 06:03:00 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  JohnD Open/Closed:  * Closed
Release:  * 5.0.0 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 23 Dec 2018 08:59:06 PM UTC, comment #19: 

This works perfectly for me. Thanks.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Sat 22 Dec 2018 05:27:17 PM UTC, comment #18: 

Changeset http://hg.savannah.gnu.org/hgweb/octave/rev/f73bcf1d0b5a
provides SYSTEM as alias for codecForLocale if SYSTEM is not supported.

I have pushed the changeset to the stable branch for completing the fix for this bug in the upcoming 5.0 release.

Torsten Lilge <ttl>
Group Member
Sat 22 Dec 2018 06:58:07 AM UTC, comment #17: 

@Markus: I see; this makes sense. I will prepare a changeset.


Torsten Lilge <ttl>
Group Member
Mon 17 Dec 2018 07:40:29 PM UTC, comment #16: 

Ok - its showing in caps for me now!

John Donoghue <lostbard>
Group Member
Thu 13 Dec 2018 09:24:10 PM UTC, comment #15: 

Changeset http://hg.savannah.gnu.org/hgweb/octave/rev/3a90eff58fb5 should fix the lower case issue from comment #11.

Torsten Lilge <ttl>
Group Member
Thu 13 Dec 2018 07:28:52 PM UTC, comment #14: 

I updated MXE-Octave to 37cc0e3b1efe, so Qt now compiles with ICU here, too.

What I wanted to say in comment #8 is that maybe we should add some logic to still be able to use "system" and call either "octave_locale_charset_wrapper" or "QTextCodec::codecForLocale()" to get a valid codec name if it is selected by the user.

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Dec 2018 07:15:54 PM UTC, comment #13: 

From the changes I made this would mean that "SYSTEM" is not supported by your system, i.e. is not included in the available codec names and does not lead to a valid codec by codecForName(). I will check the code again.

Torsten Lilge <ttl>
Group Member
Thu 13 Dec 2018 06:58:58 PM UTC, comment #12: 

I can no longer find "system" in the drop-down list in the preferences. The only way to sync Octave's encoding to the one set-up n the OS is to call _mfile_encoding_ ("system") from the command window.
After that, it still doesn't show in the drop-down list.

Just a very minor issue. But could "system" show in the drop-down list again?

Markus Mützel <mmuetzel>
Group administrator
Tue 11 Dec 2018 07:24:18 PM UTC, comment #11: 

Works for me.

One nick pick I would have, it that the default endcong is showing as "windows-1252" (ie: lower case) but the list is all upper case.

John Donoghue <lostbard>
Group Member
Sun 09 Dec 2018 09:03:58 PM UTC, comment #10: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/b23954fd0ea6
which should fix the remaining issue with the combo box. I have tested it on my system by exchanging the default preference from "UTF-8" to some string that does not name a valid codec.

Torsten Lilge <ttl>
Group Member
Sat 08 Dec 2018 09:30:51 PM UTC, comment #9: 

"System" is still supported and is still the default on windows.

But there wasn't a check if Qt is really able to create a codec from this name. This was added in the editor and has to be added in the creation of the combo box used in the prefs dialog and in some dialogs of the editor when encoding errors are detected.

Torsten Lilge <ttl>
Group Member
Sat 08 Dec 2018 09:25:21 PM UTC, comment #8: 

Thanks for clearing this up. I incorrectly assumed that we were using "system" internally in Octave to indicate that we want to use the system locale. That is what we do in libinterp/corefcn/input.cc @ l.544.
Maybe we should do something similar for Qt?
I am not sure whether we should drop supporting "system" entirely though.

Markus Mützel <mmuetzel>
Group administrator
Sat 08 Dec 2018 08:15:38 PM UTC, comment #7: 

Yes - "System" is not a valid code name in ICU.

libgui/src/m-editor/file-editor-tab.cc, line 1754:

QTextCodec *codec = QTextCodec::codecForName (_encoding.toLatin1 ());

So if encoding there was Systsem, it would (and was) fail. It now checks that if it didnt get a codec, to use the local, however settings:

libgui/src/resource-manager.cc line 395:

resource_manager::do_combo_encoding

will only set the selected  value in the settings combo box if 'enc' was set (which it isnt as "System" wasnt a codec)

So at that point the combox box selects the first one in the list.


On my machine, the one returned by QTextCodec::codecForLocale() is "windows-1252"


John Donoghue <lostbard>
Group Member
Sat 08 Dec 2018 12:41:46 PM UTC, comment #6: 

If I follow correctly, the issue is that setting Octave's internal code page to "system" fails somehow when Qt is compiled with ICU. Is this correct?

If I correctly understand the code, we call "octave_locale_charset_wrapper" to get the current locale of the system if the mfile_encoding is set to "system". So why doesn't it return a valid code page name with ICU?

@JohnD: Which locale does your Windows use? Can I test this on my system (with German locale)?

Markus Mützel <mmuetzel>
Group administrator
Wed 05 Dec 2018 10:04:04 PM UTC, comment #5: 

Ummm … not sure why I wrote file browser

I meant the editor window, the encoding field in the status bar shows as "System" until something else is selected.

The final issue really is just because the default is "Systsem", but there is not a System encoding, so the dialog shows the first encoding as selected.

John Donoghue <lostbard>
Group Member
Wed 05 Dec 2018 08:49:38 PM UTC, comment #4: 

John, thanks for testing. I have pushed the patch as changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/a7be718a9dd3
for fixing the crash. I am going to take care of the remaining issue now. Which file browser do you you mean by

"It shows system as the encoding in the file browser until something else is selected in the preferences (or apply is clicked on what was selected as the first entry)"

Torsten Lilge <ttl>
Group Member
Mon 03 Dec 2018 04:11:12 PM UTC, comment #3: 

I tried in window, and it fixed the crash.

Only issue I see that is related, it that on initial startup (including had no previous settings), It I open the Preferences dialog is is showing 'ADOBE-STANDARD-ENCODING' as the selected.

Which I guess is happening in /libgui/src/resource-manager.cc as a endocing isnt selected.


It shows system as the encoding in the file browser until something else is selected in the preferences (or apply is clicked on what was selected as the first entry)

John Donoghue <lostbard>
Group Member
Sat 01 Dec 2018 03:48:03 PM UTC, comment #2: 

John, could you please test the attached patch?

(file #45563)

Torsten Lilge <ttl>
Group Member
Fri 30 Nov 2018 06:03:49 PM UTC, comment #1: 

I forgot to add the qt link

[1] https://bugreports.qt.io/browse/QTBUG-72183

John Donoghue <lostbard>
Group Member
Fri 30 Nov 2018 06:03:00 PM UTC, original submission:  

Mainy a window issues, but could happen in Linux:

In /libgui/src/m-editor/file-editor-tab.cc file_editor_tab::load_file (const QString& fileName)

The codec is gotten using:

 QTextCodec *codec = QTextCodec::codecForName (_encoding.toLatin1 ());

However the returned value is never checked but used as is, so if returns 0, causes a crash.

The default values in Windows is "System" which according to Qt [1] is not guarenteed to be available, and indeed isnt if compiled with ICU.

The return code should be changed and a default used when it didnt load? Perhaps the codec from QTextCodec::codecForLocale(); ?

In addition, on init of the preference dialog, the available codecs are added to the locale combox box, it also adds "System" for windows.


Perhaps the default should be the name from QTextCodec::codecForLocale() as well.



John Donoghue <lostbard>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45563:  bug55139_v01.patch added by ttl (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by ttl (Updated the item)
  • -email is unavailable- added by lostbard (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-23 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-12-22 ttl StatusNone Ready For Test
        Release4.4.1 5.0.0
    2018-12-13 mmuetzel StatusReady For Test None
    2018-12-09 ttl StatusNone Ready For Test
    2018-12-01 ttl Attached File- Added bug55139_v01.patch, #45563

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code