bugGNU Octave - Bugs: bug #60980, "Position" key in...

 
 

bug #60980: "Position" key in uigetfile appears to have no effect

Submitted by:  None
Submitted on:  Wed 28 Jul 2021 12:37:57 AM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Ready For Test Assigned to:  None
Originator Name:  jlepore Originator Email:  -email is unavailable-
Open/Closed:  Open Release:  dev
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)

Sun 19 Sep 2021 03:36:25 PM UTC, comment #14: 

I pushed that patch removing the (unused) position argument for these functions here:
http://hg.savannah.gnu.org/hgweb/octave/rev/813b7827c5f8

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Sun 19 Sep 2021 02:39:24 PM UTC, comment #13: 

No feedback in about a month.
So I went ahead and implemented the changes outlined in comment #12:
http://hg.savannah.gnu.org/hgweb/octave/rev/a7981aa5822b

I'm still not sure if I understand correctly. But that change probably does not comply with what jwe wrote in comment #8.
Imho, any fix is better than no fix at all. We can probably refactor all of this when we have a plan how we'd like to implement the interface to the event manager instead.

I'll probably follow up with a change that eliminates the "position" argument from `uigetdir`, `uigetfile`, and `uiputfile` soon.

Markus Mützel <mmuetzel>
Project Member
Sun 22 Aug 2021 11:52:45 AM UTC, comment #12: 

Attached is a WIP patch that adds a new function `have_dialogs` to the event manager classes. That function returns `false` for the base class and `true` for the Qt implementation.

The patch only partly implements handling that function (only the `uigetfile` function for now). The other GUI dialogs that could be either implemented via the event manager or otherwise would probably need similar changes.
The patch also includes some work for the originally motivating removal of the "position" property for `uigetfile`. (A final patch should probably separate those two changes.)

With that patch, a file dialog appears when Octave is started with `--no-gui-libs` for `uigetfile('*.*', '', '')`.

I'm only uploading it here to ask if this is an approach worth putting more time into. Or do we need something different?

(file #51802)

Markus Mützel <mmuetzel>
Project Member
Sat 31 Jul 2021 09:34:58 AM UTC, comment #11: 

@jwe: Thanks for the clarification.
Would that logic be based on whether or not the GUI libraries are loaded? IIUC, that decides whether `qt_interpreter_events` is available. Otherwise, we'd need to fallback to FLTK if possible.

Is there currently a way to query the interpreter if these libraries are loaded?

Markus Mützel <mmuetzel>
Project Member
Fri 30 Jul 2021 07:34:58 PM UTC, comment #10: 

Maybe there is a quick fix that will at least allow it to work but I'm not sure whether the event manager should be disabled if starting without gui libs or the logic needs to change about deciding which implementation of these ui* functions to use.  Probably the latter, because I think the event manager should always be enabled.

John W. Eaton <jwe>
Project Administrator
Fri 30 Jul 2021 07:25:11 PM UTC, comment #9: 

@Dmitri: I was using a build from the current default branch on Ubuntu. It is working for me with a build from the stable branch, too.

@jwe: Tbh, I only understand half of that at most.
Does that mean that the FLTK implementation of these dialogs are broken at the moment and can't be used on the default branch? Or can we change something simple that could help make this work again (like querying if the gui libs are loaded)? Should we do that?

Markus Mützel <mmuetzel>
Project Member
Fri 30 Jul 2021 07:12:39 PM UTC, comment #8: 

@Markus: I think we need to decide how the event manager should really work.  As with Qt signals and slots, in some cases it is useful to have signals connected to multiple slots or for multiple signals to be connected to the same slot.  In others, there should be only one.  The current design of having an event function return a flag to say that another function should handle the signal is probably not be the best way.

John W. Eaton <jwe>
Project Administrator
Fri 30 Jul 2021 07:12:03 PM UTC, comment #7: 

Seems to work as expected on linux (6.3.1)

octave --no-gui-libs
GNU Octave, version 6.3.1
Copyright (C) 2021 The Octave Project Developers.
This is free software; see the source code for copying conditions.
There is ABSOLUTELY NO WARRANTY; not even for MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.  For details, type 'warranty'.

Octave was configured for "x86_64-pc-linux-gnu".

Additional information about Octave is available at https://www.octave.org.

Please contribute if you find this software useful.
For more information, visit https://www.octave.org/get-involved.html

Read https://www.octave.org/bugs.html to learn how to submit bug reports.
For information about changes from previous versions, type 'news'.

octave:1> uigetfile

The dialog window is attached.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Fri 30 Jul 2021 06:44:32 PM UTC, comment #6: 

While looking into this, I think I ran into another bug.
In `uigetfile`, the function `__event_manager_enabled__` is used to decide whether the event_manager is used to show the dialog (i.e., Qt). If that returns 0, the FLTK should be used to show that dialog.
I was starting Octave with `./run-octave --no-gui-libs` and tried to check the FLTK implementation.
The issue is that even if I start Octave that way `__event_manager_enabled__` returns true. No working dialog is displayed at all and the function fails with an error.

Is this an error in the command line argument processing? Should the event manager be disabled when starting with --no-gui-libs?
Or should `__event_manager_enabled__` return true, only if there is an actual implementation that is derived from the event_manager classes that can be used?

Am I using the wrong command line arguments?
How should I start Octave to test the FLTK implementation of that function?

Markus Mützel <mmuetzel>
Project Member
Fri 30 Jul 2021 03:45:39 PM UTC, comment #5: 

Given results quoted in comment #4, I think we should remove the argument.  Since it wasn't really every implemented I don't think we even need to deprecate it.

Rik <rik5>
Project Administrator
Fri 30 Jul 2021 06:24:14 AM UTC, comment #4: 

Fwiw, in Matlab R2021a:

>> [fname2, fpath2, fltidx2] = uigetfile('*.*', '', '', 'Position', [1000,1000])
Error using uigetputfile_helper/parseArguments (line 146)
Extraneous arguments entered, please check the documentation.

Error in uigetputfile_helper (line 28)
[dialog_filter, dialog_title, dialog_filename, dialog_pathname, dialog_multiselect] = parseArguments();

Error in uigetfile (line 133)
[filename, pathname, filterindex] = uigetputfile_helper(0, varargin{:});

It looks like this is a "pure" Octave extension that was never really completed.
Best way forward (also in light of compatibility with Matlab) is probably to remove it completely imho. Maybe, we could issue a warning for the next two versions before disallowing that syntax.

Markus Mützel <mmuetzel>
Project Member
Thu 29 Jul 2021 08:37:52 PM UTC, comment #3: 

Octave usually doesn't bother to implement undocumented Matlab capabilities.  Because they aren't documented it is hard to ensure compatible behavior.  And because they aren't documented, they can change without warning between versions of Matlab.

Rik <rik5>
Project Administrator
Thu 29 Jul 2021 04:45:04 PM UTC, comment #2: 

I can confirm on both Windows and Ubuntu.

It looks like this is an extension to the Matlab function that isn't completely implemented. Their documentation doesn't mention a "position" property:
https://de.mathworks.com/help/matlab/ref/uigetfile.html

The position is assigned to a variable here. But it is never used for the Qt graphics toolkit:
https://hg.savannah.gnu.org/hgweb/octave/file/53de043ea532/libinterp/corefcn/event-manager.cc#l311

Looking back in the history of the repository, it looks like this was never implemented.

The same goes for corresponding implementation with the FLTK graphics toolkit:
https://hg.savannah.gnu.org/hgweb/octave/file/53de043ea532/libinterp/dldfcn/__fltk_uigetfile__.cc#l78

Maybe, we should just remove that property from the documentation and simplify the code by removing the position property entirely.

Markus Mützel <mmuetzel>
Project Member
Thu 29 Jul 2021 04:17:07 PM UTC, comment #1: 

Copying message of the original report from mailing list because it looks like it doesn't show up in savannah:

[fname2, fpath2, fltidx2] = uigetfile("*.*", "", "", "Position", [1000, 1000])

Position of dialog box does not change regardless of [px py].

Markus Mützel <mmuetzel>
Project Member
Wed 28 Jul 2021 12:37:57 AM UTC, original submission:  
Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51802:  bug60980-event-manager-dialogs.patch added by mmuetzel (12KiB - application/octet-stream)
file #51713:  uigetfile_das.png added by dasergatskov (17KiB - image/png)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2021-09-19 mmuetzel StatusConfirmed => Ready For Test
    2021-08-22 mmuetzel Attached File- => Added bug60980-event-manager-dialogs.patch, #51802
    2021-07-30 dasergatskov Attached File- => Added uigetfile_das.png, #51713
    2021-07-29 mmuetzel StatusNone => Confirmed
        Release6.1.0 => dev
        Operating SystemMicrosoft Windows => Any

    Back to the top


    Powered by Savane 3.6