bugGNU Octave - Bugs: bug #60646, launching qt doc browser with doc...

 
 

bug #60646: launching qt doc browser with doc function leaves temporary files

Submitted by:  A.R. Burgers <arb>
Submitted on:  Thu 20 May 2021 09:48:47 AM UTC  
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  GNU/Linux

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)

Wed 16 Jun 2021 05:16:28 AM UTC, comment #26: 

I also think that saving settings (including widget state and geometry) in the destructor is not working since the widgets already were be closed and consequently save wrong state and geometry. 

Torsten Lilge <ttl>
Project Member
Wed 16 Jun 2021 12:45:30 AM UTC, comment #25: 

I also meant to mention that I think I understand now why calling the octave_dock_widget::save_settings function from the octave_dock_widget destructor wsan't working properly: save_settings is a virtual function but when called from the destructor it won't behave as a virtual function normally does.  So any overloaded versions in derived classes won't be called.

John W. Eaton <jwe>
Project Administrator
Wed 16 Jun 2021 12:41:02 AM UTC, comment #24: 

I pushed this additional change:

http://hg.savannah.gnu.org/hgweb/octave/rev/3b5ffc4c70e9

With it, things seem to work reasonably well for me, but there is probably still room for improvement.  In the past, closing the main window meant the application was also exiting.  But with the new terminal widget that won't be the case, so it seems to me that we need to do something different with most (or all?) of those connections to the QApplication::aboutToQuit signal.

John W. Eaton <jwe>
Project Administrator
Tue 15 Jun 2021 07:04:34 PM UTC, comment #23: 

Another question:  Do we really need the following signal/slot connections now (from main_window::construct in libgui/src/main-window.cc)?

    connect (qApp, &QApplication::aboutToQuit,
             m_command_window, &terminal_dock_widget::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_history_window, &history_dock_widget::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_file_browser_window, &files_dock_widget::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_doc_browser_window, &documentation_dock_widget::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_workspace_window, &workspace_view::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_editor_window, &file_editor_interface::save_settings);

    connect (qApp, &QApplication::aboutToQuit,
             m_variable_editor_window, &variable_editor::save_settings);

It seems to me that now that we are able to close the main window without exiting the application, saving the settings should be linked to something other than the QApplication::aboutToQuit signal.

If I start Octave in command line mode with the new terminal widget enabled and

  • execute the "desktop" command
  • close the desktop and return to the command line
  • exit Octave

then, the only save_settings functions that are executed are for dock widgets that remain open when Octave exits and they appear to be called twice: first because of the connection to the QApplication::aboutToQuit signal and then again from the base_qobject destructor when we now explicitly close them.

Now that the dock widgets and the resource manager object are both owned by the base_qobject, it should be safe to save the settings from the dock widget destructors because we can ensure that the resource manager object is deleted after all the dock widgets and the main window have been deleted.

So it seems like the attached patch should work.  With it, I do see that the settings are saved when the objects are deleted, but if I start the desktop from the command line multiple times, the window layout seems to be lost and I end up with only the command window open, or the command window and any dock widgets that were undocked when the desktop was last closed (they do remain undocked, as expected).

Is the geometry and layout info wiped out before the destructor is called?  How?

(file #51566)

John W. Eaton <jwe>
Project Administrator
Tue 15 Jun 2021 04:37:34 PM UTC, comment #22: 

Oops, I pushed the wrong button.

@Torsten: Thanks.  I also pushed the following change:

http://hg.savannah.gnu.org/hgweb/octave/rev/61eb6602189b

Saving and restoring settings seems to work much better now.  But I am curious to know why we don't just save settings from the destructors of these objects?

Also, marking as ready for test.  Is the original problem of deleting temporary files solved?

John W. Eaton <jwe>
Project Administrator
Tue 15 Jun 2021 04:34:36 PM UTC, comment #21: 

@Torsten: Thanks.  I also pushed the following change:

John W. Eaton <jwe>
Project Administrator
Mon 14 Jun 2021 08:09:21 PM UTC, comment #20: 

@John: I have pushed http://hg.savannah.gnu.org/hgweb/octave/rev/1511b7cd3474 which seems to work well even without deleteLater.

Regarding the calls to clear (), I agree that these should not be required anymore.

Torsten Lilge <ttl>
Project Member
Mon 14 Jun 2021 07:28:25 PM UTC, comment #19: 

@Torsten: OK.  I'm also wondering now whether the lines like

    if (m_history_window->isFloating ())
      m_history_window.clear ();

in the main_window destructor are needed now?  These variables like m_history_window should only be deleted if they are children of the main_window object and since they are stored as QPointer objects they will be set to nullptr when they are deleted.  Then if they are needed again at the command line, they will be recreated.

John W. Eaton <jwe>
Project Administrator
Mon 14 Jun 2021 06:27:04 PM UTC, comment #18: 

I am currently building for testing a version where the widgets are only closed if m_main_window is the nullptr. In the other case the widgets should be closed when closing the main window.

Torsten Lilge <ttl>
Project Member
Mon 14 Jun 2021 06:18:04 PM UTC, comment #17: 

The behavior is better after the following change but I don't fully understand why.

http://hg.savannah.gnu.org/hgweb/octave/rev/f8322ca49578

John W. Eaton <jwe>
Project Administrator
Mon 14 Jun 2021 05:12:06 PM UTC, comment #16: 

I pushed the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/23cf249a0ad8

Since these widgets are all supposed to be executing in the same thread as the base_qobject, I didn't use deleteLater.  Then I noticed Octave exiting with a segfault so I applied the following additional change:

http://hg.savannah.gnu.org/hgweb/octave/rev/7db4a4e94556

Now I'm not seeing a segfault on exit.  But something is still not right with saving settings and restoring the default window layout.  I'm seeing a layout with just the two tabbed widgets (no file browser, history, or workspace view) and using the menu item to restore the default layout doesn't add the missing widgets.  I have to explicitly select them, then use the "Restore Default Window Layout" menu item to fix the sizes.  Then after exiting and restarting, I'm back to the two tabbed widgets with nothing else.  Strange.

John W. Eaton <jwe>
Project Administrator
Fri 11 Jun 2021 08:28:16 PM UTC, comment #15: 

comment #14:

> @ttl:  So deleting a window/widget doesn't trigger a close signal?  If closing causes other signals to be emitted, do we need to use deleteLater for the widgets that are closed or is it safe to just delete them immediately after calling close?


I think this is only necessary when the 'close' and 'delete' are executed in another thread than the closeEvent. But maybe deleteLater would be the more robust way. 

> I suppose we need to do the same for all the dock widgets that are now owned by base_qobject?


Yes, the documentation widget was just an example I have chosen for the test.

> Is it safe to call close on a widget that is already closed?  Maybe we could use

> if (m_main_window)
>   delete m_main_window;
> else
>   {
>     if (m_documentation_widget)
>       m_documentation_widget->close ();
>
>     if (...)
>       ...->close ();
>
>     ...
>   }
>
> // Or just delete?
> m_documentation_widget->deleteLater ();
> ...


Since we are not setting the property Qt::WA_DeleteOnClose we have to delete the widgets after closing (or better deleteLater). deleteLater () does not trigger close. So we have to use to upper code but with deleting the widgets afterwards.

There is also the destroyed signal triggered just before the widget is deleted but I am reluctant to use this for any required actions like saving the settings.

Torsten Lilge <ttl>
Project Member
Fri 11 Jun 2021 07:49:38 PM UTC, comment #14: 

@ttl:  So deleting a window/widget doesn't trigger a close signal?  If closing causes other signals to be emitted, do we need to use deleteLater for the widgets that are closed or is it safe to just delete them immediately after calling close?

I suppose we need to do the same for all the dock widgets that are now owned by base_qobject?

Is it safe to call close on a widget that is already closed?  Maybe we could use

if (m_main_window)
  delete m_main_window;
else
  {
    if (m_documentation_widget)
      m_documentation_widget->close ();

    if (...)
      ...->close ();

    ...
  }

// Or just delete?
m_documentation_widget->deleteLater ();
...

John W. Eaton <jwe>
Project Administrator
Fri 28 May 2021 09:29:27 AM UTC, comment #13: 

I have attached a patch that exemplary closes the documentation widget (and therefore triggers saving the settings) if the widget is existing and if the main window is not.

(file #51490)

Torsten Lilge <ttl>
Project Member
Fri 28 May 2021 06:29:59 AM UTC, comment #12: 

@jwe: The '?'-items in your list sound good to me.

Regarding your other questions:

  • When starting the CLI, I think that no widgets should be opened automatically even if widgets were opened in the previous session.
  • GUI/CLI should not be handled separately regarding restoring the docking state of the widgets
Torsten Lilge <ttl>
Project Member
Tue 25 May 2021 05:37:32 PM UTC, comment #11: 

We do not currently keep track of which ones were opened from the command line.

With the new terminal widget, it will be possible to have the following sequence (the lines marked with ? is the behavior that I would expect but might still be up for debate):

  • open a dockable widget from the command line
  • open the desktop from the command line

   ? open dockable widgets should remain undocked

  • dock one of the widgets that was initially opened from the command line or undock one that was originally opened (and docked) with the main desktop window
  • close the desktop window and return to the command line

   ? any undocked widgets should remain open
   ? widgets that were originally opened at the command line but were docked when the desktop were closed should be closed with the main desktop window

When restarting Octave in command line mode, should it open and display widgets that were undocked the last time it was closed?

If starting Octave in GUI mode after a sequence like the one described above for a command line session, should the dockable widget configuration from the previous command-line session be restored, or should CLI/GUI widget positions be handled separately?

John W. Eaton <jwe>
Project Administrator
Tue 25 May 2021 04:40:08 PM UTC, comment #10: 

A setting, that is only saved when closing, is the geometry of a widget or the main window. Otherwise we would have to catch the resize event and rewrite the settings triggered by the event handler.

Does the cli somehow keeps track of the widgets it has opened? If yes, would it be possible to call close () for each of them when octave exists?

Torsten Lilge <ttl>
Project Member
Tue 25 May 2021 02:20:47 PM UTC, comment #9: 

I realize now that with the current design of saving the settings in the destructor and having those objects managed by the base_qobject, then, at the point when they are deleted, they will never be attached to the main_window.  So, yes, saving settings there is the wrong thing to do.

But why should we even need to save the settings when the application closes?  Aren't the settings saved any time there is a change in the appearance?  Or are they normally only saved when the window is closed?

John W. Eaton <jwe>
Project Administrator
Tue 25 May 2021 06:45:58 AM UTC, comment #8: 

About the resource manager, I think we should now be able to control how long it exists.  If needed, shouldn't a deleteLater call prevent it from being deleted until after all possible signals for it are delivered?

John W. Eaton <jwe>
Project Administrator
Tue 25 May 2021 06:44:04 AM UTC, comment #7: 

I just noticed that after my recent changes that when starting Octave with the --gui option, only the command window is showing.  I can bring all of the others back individually or by resetting the default window layout but then at the next restart they are once again not shown.  Argh!!!

John W. Eaton <jwe>
Project Administrator
Tue 25 May 2021 06:15:47 AM UTC, comment #6: 

I remember there was a reason why it might be better to not save the settings in the destructor because the resource manager might not be existing anymore (see bug #45366, comment #1, cset http://hg.savannah.gnu.org/hgweb/octave/rev/6e9f5408c0db). Since the resource manager is not implemented as a singleton anymore, this might not be a problem anymore.

I can confirm the behavior of the doc browser which shortly shows the desired page and then switches to the contents. This really looks like a regression from changeset http://hg.savannah.gnu.org/hgweb/octave/rev/b4e2c6150976, that I have pushed recently in order to fix an empty contents page at startup of the doc browser. I will have a look at this and why it only happens when a specific documentation is requested started from the command line and not when requested from the gui.

Torsten Lilge <ttl>
Project Member
Mon 24 May 2021 09:28:45 PM UTC, comment #5: 

For me, this change fixes the problem with savings settings:

http://hg.savannah.gnu.org/hgweb/octave/rev/b16748c8aa4d

Is it OK to call save_settings from the dock widget destructor?

John W. Eaton <jwe>
Project Administrator
Mon 24 May 2021 09:15:46 PM UTC, comment #4: 

This change seems to fix the tmp file problem for me:

http://hg.savannah.gnu.org/hgweb/octave/rev/f292dd157a08

but settings still don't appear to be saved when I start Octave in command line mode.  For example, if I bring up a dock widget, change its size and then exit Octave, the new size is not used the next time I start Octave.

Another thing I noticed now is that if I start Octave in command line mode and execute "doc svd", I briefly see the documentation for the svd function and then it is replaced by the contents page.  But maybe that should be a separate bug report.

John W. Eaton <jwe>
Project Administrator
Sat 22 May 2021 08:03:19 AM UTC, comment #3: 

It looks like the closeEvent of the dock widgets isn't triggered either, which prevents the widgets from saving their settings.

Torsten Lilge <ttl>
Project Member
Thu 20 May 2021 03:15:37 PM UTC, comment #2: 

The code to delete the files is still in the documentation destructor (gui/src/documentation.cc) but it looks like that is no longer being called, maybe because the documentation browser might not have a parent widget when Octave is closed.  This issue may also apply to other widgets that are now owned by base_qobject instead of main_window.  I'll check it out.

John W. Eaton <jwe>
Project Administrator
Thu 20 May 2021 02:56:26 PM UTC, comment #1: 

Confirmed, with the whole GUI or with only the doc browser launched from cli. This is a regression: temporary file and directory are correctly deleted in Octave 6.2.

Pantxo Diribarne <pantxo>
Project Member
Thu 20 May 2021 09:48:47 AM UTC, original submission:  

after launching octave-7.0.0 (no gui, but not the -cli version), from tmpdir, then interactively type "doc" to launch qt doc browser

{/tmp }147: octave-7.0.0 --quiet
octave:1> doc
octave:2> quit

will leave temporary qhelp file and associated directory in tmpdir

ls
.oct-qhelp-H61MF6/
oct-qhelp-H61MF6
.oct-qhelp-ZnKCtD/
oct-qhelp-ZnKCtD

A.R. Burgers <arb>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51566:  save-settings-diff.txt added by jwe (6KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by arb (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-06-15 jwe Attached File- => Added save-settings-diff.txt, #51566
    2021-06-15 jwe StatusIn Progress => Ready For Test
    2021-05-28 ttl Attached File- => Added cli_close_gui_widgets_v01.patch, #51490
    2021-05-24 jwe StatusConfirmed => In Progress
    2021-05-20 pantxo CategoryInterpreter => GUI
        StatusNone => Confirmed

    Back to the top


    Powered by Savane 3.6