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

 
 

bug #55139: m-editor locale failure caused crash

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

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 17 Dec 2018 07:40:29 PM UTC, comment #16:

Ok - its showing in caps for me now!

John Donoghue <lostbard>
Project 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 <ttl>
Project 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>
Project Member
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 <ttl>
Project 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>
Project Member
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>
Project 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 <ttl>
Project 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 <ttl>
Project 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>
Project Member
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>
Project 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>
Project Member
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>
Project 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 <ttl>
Project 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>
Project Member
Sat 01 Dec 2018 03:48:03 PM UTC, comment #2:

John, could you please test the attached patch?

(file #45563)

Torsten <ttl>
Project 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>
Project 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>
Project 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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.3