bugGNU Octave - Bugs: bug #42490, helpdlg does not return a proper...

 
 

bug #42490: helpdlg does not return a proper graphics handle

Submitter:  Hartmut <hardy>
Submitted:  Mon 02 Jun 2014 01:36:18 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
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

Tue 09 Oct 2018 05:15:21 PM UTC, comment #23: 

I verified the cset with the original example.  helpdlg properly returns a graphics handle and uiwait properly works with that handle.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Mon 08 Oct 2018 07:21:17 AM UTC, comment #22: 

I pushed the patch here:

http://hg.savannah.gnu.org/hgweb/octave/rev/9c47eedc44e2

Marking "ready for test".

Pantxo Diribarne <pantxo>
Group Member
Fri 05 Oct 2018 11:02:20 AM UTC, comment #21: 

@Dan: In this case windows an Mac users would have no icon at all. The fallback mechanism I used is the one that is already in place and works very well for all other icons. Basically it uses QIcon::fromTheme [1] which only works on linux platform.

I am pretty sure that Qt uses native icons on Windows and Mac when displaying a QMessageBox so that fallback icon we will get on those platforms won't match the one we'll get when an actual QMessageBox is displayed. Yes I could extract the icon from a QMessageBox in case none is returned, but honestly I have recently worked on a patch that I couldn't really test myself (since I didn't have a Mac) and I don't want to do this again, not now. So I'd be in favor of leaving this as is and wait for complaints if any.

[1] http://doc.qt.io/qt-5/qicon.html#fromTheme

Pantxo Diribarne <pantxo>
Group Member
Fri 05 Oct 2018 08:12:30 AM UTC, comment #20: 

That's a much better manner of getting the icons of the style to match the GUI.  I don't see the need for the additional fallback icons though.  At least not where it is.  If the Qt resources can't find the proper icon for the msgbox, it will not--in all likelihood--find the same icon for other parts of the GUI using dialog boxes.  So, wouldn't it make sense to put the fallback icons somewhere else in the Qt system?  Does Qt have its own fallback icons?  I think it would be better to leave out the fallback icons and code until some user actually runs across missing icons.  And if so, find a more universal location to install the icons in Qt.

Dan Sebald <sebald>
Fri 05 Oct 2018 07:25:56 AM UTC, comment #19: 

I attached a new patch in which I used the GUI's resource_manager to retrieve standard (at least on linux) "dialog-error", "dialog-information" and "dialog-warning" icons. If those are not present in the theme used by Qt (on MacOS and Windows I guess) the resource_manager should (I could not test myself) fallback to corresponding Tango icons which I added as png and svg.

(file #45147)

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 09:26:40 PM UTC, comment #18: 

@Dan: I don't like "demo"'s behavior either, but it is another matter.

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 08:35:18 PM UTC, comment #17: 

Yes, use the existing QIcons and naming convention.  Let the framework deal with the look and feel.  Adding binary icons only makes OctaveGUI more custom.

Dan Sebald <sebald>
Wed 03 Oct 2018 08:31:33 PM UTC, comment #16: 

@Pantxo: "Usually "demo" closes all figures but one and uses the latter for graphics. Here the first figure is cleared when it should be closed."  I don't know about others, but I certainly would prefer demo not close any existing figures.  Instead, it would be nice if it opened a new figure, i.e., first available non existing figure...possibly deleting that figure at the end of the demo (and an option to leave it in existence).  The thing is, I'm often in the middle of work and just want to learn how a command works; I don't want to close some important figures/plots that I'm working on.

Dan Sebald <sebald>
Wed 03 Oct 2018 08:28:36 PM UTC, comment #15: 

Actually, I see that the GUI's resource_manager already has what we need. resource_manager::icon(icon_name) returns a QIcon for a set of named icons (IIUC named according to the freedesktop naming scheme) and fallback to Tango icons if the current theme does not provide the named icon (such as on Windows). We should probably use this.

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 02:00:40 PM UTC, comment #14: 

I posted bug #54774 about the "resize" figure property having no effect.


Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 09:33:49 AM UTC, comment #13: 

@Dan: Ok, this is indeed probably the way to go for fetching standard icons. I'll look into this.

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 09:23:13 AM UTC, comment #12: 

Ah, OK so forcing the action to wait with uiwait() achieves what I described (i.e., "Msg 1" first, then "Msg 2" follows):


>> uiwait(h1 = msgbox("Msg 1", "title 1", "none", "modal")); h2 = msgbox("Msg 2", "title 2", "none", "modal");


If there is access to Qt, the icons are determined by the Style, for which the pixmaps can be gotten via

http://doc.qt.io/archives/qt-4.8/qstyle.html#standardIcon
http://doc.qt.io/archives/qt-4.8/qstyle.html#StandardPixmap-enum
http://doc.qt.io/archives/qt-4.8/qicon.html#pixmap

In particular, these pixmaps:

QStyle::SP_MessageBoxInformation 9 The "information" icon.
QStyle::SP_MessageBoxWarning 10 The "warning" icon.
QStyle::SP_MessageBoxCritical 11 The "critical" icon.
QStyle::SP_MessageBoxQuestion 12 The "question" icon.

can be used to create a standard icon, and from the icon the actual pixmap can be generated according to the specified size.

Dan Sebald <sebald>
Wed 03 Oct 2018 08:48:44 AM UTC, comment #11: 

@Dan: As for the "demo msgbox" clearing the first figure and removing the second, indeed "demo" behaves weirdly here. Usually "demo" closes all figures but one and uses the latter for graphics. Here the first figure is cleared when it should be closed.

Also the behavior you describe for


>> h1 = msgbox("Msg 1", "title 1", "none", "modal"); h2 = msgbox("Msg 2", "title 2", "none", "modal");


is expected and is how Matlab handles modal dialogs: the latest created modal dialog takes precedence. See the Matlab documentation for msgbox and bug #54720 for details.

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 08:35:52 AM UTC, comment #10: 

@Dan: Thanks for testing. I do think this is the way to go (and the only way in which we can be Matlab compatible) and I also thing the other dialogs (questdlg, inputdlg, ...) should be implemented the same way (even though there is no constraint of returning a figure handle for those). The fact that dialogs can be resized is another bug: the "resize" figure property of dialogs is correctly set "off" but this property is currently ignored by all graphics toolkits. 

As I said I had anticipated that this png file inclusion was not correct. I see "File scripts/gui/__error-icon__.png: git binary diffs are not supported" when you apply the patch. The --git option is the only way I know of for including binary data in a patch, and applying the patch on another computer works for me:


$ hg qimport ~/Downloads/msgbox_fig4.patch
adding msgbox_fig4.patch to series file
$ hg qpush
applying msgbox_fig4.patch
patching file libgui/graphics/BaseControl.cc
patching file libgui/src/octave-qt-link.cc
patching file libgui/src/octave-qt-link.h
patching file libinterp/corefcn/octave-link.cc
patching file libinterp/corefcn/octave-link.h
patching file scripts/gui/__error-icon__.png
patching file scripts/gui/__help-icon__.png
patching file scripts/gui/__warn-icon__.png
patching file scripts/gui/errordlg.m
patching file scripts/gui/helpdlg.m
patching file scripts/gui/module.mk
patching file scripts/gui/msgbox.m
patching file scripts/gui/warndlg.m
adding scripts/gui/__error-icon__.png
adding scripts/gui/__help-icon__.png
adding scripts/gui/__warn-icon__.png
committing files:
libgui/graphics/BaseControl.cc
libgui/src/octave-qt-link.cc
libgui/src/octave-qt-link.h
libinterp/corefcn/octave-link.cc
libinterp/corefcn/octave-link.h
scripts/gui/__error-icon__.png
scripts/gui/__help-icon__.png
scripts/gui/__warn-icon__.png
scripts/gui/errordlg.m
scripts/gui/helpdlg.m
scripts/gui/module.mk
scripts/gui/msgbox.m
scripts/gui/warndlg.m
committing manifest
committing changelog
now at: msgbox_fig4.patch


And all works as expected (including "demo msgbox")

I agree that reusing icons from the GUI for those dialogs would be cool but I fear those icons are platform dependent (on my system, the icons are not those shown here https://doc.qt.io/qt-5/qmessagebox.html#Icon-enum) and we would need a way to fetch them at run time.

Pantxo Diribarne <pantxo>
Group Member
Wed 03 Oct 2018 07:56:13 AM UTC, comment #9: 

The patch reports:


@linux ~/octave/octave/octave $ patch -p1 < ~/octave/bug/42490/msgbox_fig4.patch
patching file libgui/graphics/BaseControl.cc
patching file libgui/src/octave-qt-link.cc
patching file libgui/src/octave-qt-link.h
patching file libinterp/corefcn/octave-link.cc
patching file libinterp/corefcn/octave-link.h
File scripts/gui/__error-icon__.png: git binary diffs are not supported.
File scripts/gui/__help-icon__.png: git binary diffs are not supported.
File scripts/gui/__warn-icon__.png: git binary diffs are not supported.
patching file scripts/gui/errordlg.m
patching file scripts/gui/helpdlg.m
patching file scripts/gui/module.mk
patching file scripts/gui/msgbox.m
patching file scripts/gui/warndlg.m


and later there is a compile error:


make[2]: *** No rule to make target 'scripts/gui/__error-icon__.png', needed by 'all-am'.  Stop.
make[2]: *** Waiting for unfinished jobs....
  GEN      build-aux/subst-script-vals.sh


I created some empty files:


touch scripts/gui/__error-icon__.png
touch scripts/gui/__help-icon__.png
touch scripts/gui/__warn-icon__.png


which creates its own problems:


>> demo msgbox
msgbox example 1:
 msgbox ("A bare dialog");

Press <enter> to continue:
msgbox example 2:
 msgbox ("An informative string", "Documentation", "help");

msgbox example 2: failed
Magick++ exception: Magick: Improper image header (/home/sebald/octave/octave/octave/scripts/gui/__help-icon__.png) reported by coders/png.c:2673 (ReadPNGImage)


in which no message box appears when there is an error reading the PNG.  The message box should still appear but just a blank icon.

Also, do something like the following:


>> figure(1)
>> h1 = plot(1:50);
>> figure(2)
>> h2 = plot(50:-1:1);
>> demo msgbox
msgbox example 1:
 msgbox ("A bare dialog");

Press <enter> to continue:


and the Figure 1 contents goes blank.  The Figure 2 is usurped by the message box and eventually deleted on close.

Rerunning the "demo msgbox", if I type return real quickly multiple message boxes will appear.  So that makes me wonder about the following:


>> h1 = msgbox("Msg 1", "title 1", "none", "modal"); h2 = msgbox("Msg 2", "title 2", "none", "modal");


The message boxes do appear to be modal, but I wonder if msgbox 1 should appear as modal and wait for response, then after that display msgbox 2 and wait for response.

In any case, I really don't know if this is the way to go.  Don't we want the message boxes to have the look and feel of the GUI, e.g., the "demo listdlg" and "demo questdlg"?  The msgbox objects, being like figures, can be resized resulting in odd effects.  Compare that with the questdlg and listdlg which can't be resized.  I understand that the proposed approach utilizes the underlying handle-graphics, which is good in a way, but can't it be made to use the default Qt system icons, which ultimately go back to the system icons?

Dan Sebald <sebald>
Tue 02 Oct 2018 07:57:50 PM UTC, comment #8: 

forgot the attachment. Here it is.

(file #45127)

Pantxo Diribarne <pantxo>
Group Member
Tue 02 Oct 2018 07:51:00 PM UTC, comment #7: 

I attached a preliminary patch in which I rewrote msgbox.m to make use of graphics objects and return a handle to a figure (same for helpdlg, errordlg and warndlg which are msgbox wrappers).

I am not sure about how to handle icon data. Since those images are for use in gui scripts, I put the corresponding png files (extracted from Tango theme) in the script/gui directory. For this I tried to blindly copy how "default.img" file is handled in the build system but it doesn't look like it works:

I can do


im = imread ("default.img");


but


## This doesn't work
im = imread ("__error-icon__.png");
##-> error: imread: unable to find file '__error-icon__.png'

## This works
im = imread (which ("__error-icon__.png"));


Can someone test the patch (see "demo msgbox").

Pantxo Diribarne <pantxo>
Group Member
Wed 10 Feb 2016 07:45:03 AM UTC, comment #6: 

FYI, if code is still in the state I think it is, the Qt dialog code is something I wrote, and it was not initially intended to be a full-blown implementation of handles or whatnot for reasons of limitations.  Also, the dialogs have to be modal because of this limitation.  The limitation at the time was the lack of a well-designed cross-thread communication between Octave core and whatever GUI it is running in.  Without that, about all that can be done is suspend the Octave core while the GUI dialog is responded to, return the answer, then take the core out of sleep state and read the result.  Having a good cross-thread comm where commands and results can be exchanged would solve not only this problem, but make breakpoints and such easy as well.

Dan Sebald <sebald>
Wed 04 Jun 2014 01:24:22 AM UTC, comment #5: 

The octave_link variant does bring up a Qt dialog box instead. But the same bug is present, the value returned is always 1, not a valid handle to the dialog box.

The matlab help pages for helpdlg and the other dialog functions also says that the windows should be non-modal by default, returning control to the interpreter while the dialog is still up.

Mike Miller <mtmiller>
Group Member
Tue 03 Jun 2014 08:08:18 PM UTC, comment #4: 

In the mean time I saw that <scripts>/..../gui/private/message_dialog.n can also call _octave_link_message_dialog_ (in libinterp/corefcn/octave-link.cc). So Java doesn't have to be invoked at all to produce dialog windows.

Makes me wonder what graphics subsystem is invoked by octave_link() (OpenGL?) (I quickly got lost in the octave_link.h file). After all a graphics dialog window is produced, so there must be a window handle somewhere. (But then maybe one w/o usable public methods.)

So, in the end I suppose a fix would require quite a bit of overhaul that could better wait until Qt dialog and more ui* functions have been implemented in a structured way.

The Java variants might be patched then to return a Java window handle - also needs some overhaul. But that would yield some closer "ML compatibility" (i.e., Java based that is)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 03 Jun 2014 07:39:19 PM UTC, comment #3: 

That was my thinking too.  I believe eventually Octave can do this with the Qt toolkit, so I've marked this as postponed until some time after 4.0.

Rik <rik5>
Group administrator
Tue 03 Jun 2014 07:17:36 PM UTC, comment #2: 

As far as helpdlg.m (and its other *dlg siblings) is concerned, they currently produce Java-based windows. I wonder if Octave's ui.... functions would be able to deal with Java window handles anyway.

AFAIU Matlab's GUI was Java-based so it made more sense there to return a Java window handle. (I'm not sure about the GUI of ML's recent versions though.)

Maybe this bug can be attacked once Qt-based *dlg functions are implemented. Should this bug report be marked as "postponed" and/or "feature request" until then?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 02 Jun 2014 03:42:42 PM UTC, comment #1: 

Confirmed.  This seems to be a general incompatibility with the *dlg functions.  It does not appear that they were ever coded with the intent of returning a graphics handle.

Rik <rik5>
Group administrator
Mon 02 Jun 2014 01:36:18 PM UTC, original submission:  

This behavior is fully documented in the Octave doc. But it is incompatible to Matlab.

Here is an (artificial) example that works under Matlab and does not under Octave:


clear all;
h = helpdlg('Are you ready?')
uiwait(h)
display('Lets start');


The difference seems to be that

  • helpdlg does NOT return a proper graphics handle of its own window, but always returns the value 1  (as is said in the manual)
  • uiwait can only work with proper graphics handles (which is fine, but generates an error message in this case)


Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45147:  msgbox_fig8.patch added by pantxo (142KiB - text/x-patch)
file #45127:  msgbox_fig4.patch added by pantxo (33KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by pantxo (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 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-19 mtmiller Carbon-CopyRemoved 80942 -
    2018-10-09 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-10-08 pantxo StatusPatch Submitted Ready For Test
    2018-10-05 pantxo Attached File- Added msgbox_fig8.patch, #45147
    2018-10-03 pantxo Priority1 - Later 5 - Normal
    2018-10-02 pantxo Attached File- Added msgbox_fig4.patch, #45127
    2018-10-02 pantxo CategoryPlotting GUI
        StatusPostponed Patch Submitted
    2018-04-27 mtmiller Priority5 - Normal 1 - Later
        Release3.8.1 dev
    2016-02-11 rik5 Carbon-CopyRemoved 72865 -
    2016-02-05 pantxo Dependencies- bugs #47056 is dependent
    2014-06-03 rik5 StatusConfirmed Postponed
    2014-06-02 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code