bugGNU Octave - Bugs: bug #44219, Deleted GUI file editor tabs not...

 
 

bug #44219: Deleted GUI file editor tabs not forgotten upon command line "exit"

Submitter:  Dan Sebald <sebald>
Submitted:  Tue 10 Feb 2015 08:03:39 AM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
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

Mon 16 Feb 2015 03:29:06 AM UTC, comment #13: 

OK, thanks.  I see lupdate-qt4 on my system.

My guess is that at some point down the road users will ask for specific behavior regarding shutdown when Octave core is doing a long process job.  Maybe they'll want a dialog to popup after five seconds saying "Octave is busy or not responding.  Would you like to force exit?"  That kind of thing should be easy using some Qt timers with the new configuration.

Dan Sebald <sebald>
Sun 15 Feb 2015 01:50:36 PM UTC, comment #12: 

I have pushed the patch with small cosmetic changes in check_file_modified (changeset http://hg.savannah.gnu.org/hgweb/octave/rev/21015ca26566).

The language files have not been updated for a very long time. This is done by the qt-tool "lupdate" which collects the strings directly from the source files. The Italian translation is new, that's why the strings from recent changes are showing up only there and in no other language files.

Torsten Lilge <ttl>
Group Member
Sat 14 Feb 2015 05:59:39 PM UTC, comment #11: 

Thanks for testing.  Attached is an upgrade.

- The default confirm_shutdown() was returning false, when it should return true.  octave-cli now exits.

- I picked the wrong save-modified-file message.  I meant to choose the "Do you want to cancel closing, save or discard the changes?", which seems to generically cover all cases.  Again, if we want it customized, I suggest passing in a QString under the different scenarios (note that the -1 case no longer exists because with the changeset "exit" at the command line now confirms exit).

Regarding the character strings.  I see that they show up in libgui/languages/it_IT.ts.  Does it seem like there should be a different way of managing the original string?  If GUI dialog boxes are managed in the layout program, is translation maintenance part of that?

As for the discussion in #42307, I disagree with the approach of using C++ api, especially via some callback method across threads.  The big drawback is that requires knowledge of the API routines in order to do GUI programming, routines that really aren't defined in stone or documented.  On the other hand, commands are well defined, have extensive documentation, have a generic return format of octave_value, and keep the GUI from being entwined with the core worker process so that someone else can write a GUI in some other framework without too much trouble.  The way things are laid out right now--with a GUI main process and an octave core worker process--lends itself to the use of commands.  There is always going to be some element of coordination to deal with in an asynchronous environment like a GUI.  Can the GUI call API functions directly?  No, because octave core could be busy doing some processing.  So either it is queuing callback function calls (across threads), or queuing commands.  The language already has the ability to do everything needed, as far as a GUI is concerned.  Realtime graphics may be another issue, but that would require more thought and better design--it's down the road, another stage, given the way open-source projects work.

(file #33075)

Dan Sebald <sebald>
Sat 14 Feb 2015 04:34:23 PM UTC, comment #10: 

Dan, thanks for the patch. It really looks nice, but there are still some issues:

  • "exit" does not work anymore in CLI mode
  • In my opinion, there have to be different messages in the dialog box when a modified editor tab is closed. With the patch, the dialog box is asking the user if he wants to cancel exiting octave even when just manually closing the tab. Instead, it should be clear what exactly is meant by "Cancel".
  • There is a discussion in bug #42307 about the preferable use of the C++ API compared to executing a command in the terminal (comment #2 in bug #42307). The shift from Fquit to executing "exit" in the terminal goes into the opposite direction


Torsten Lilge <ttl>
Group Member
Sat 14 Feb 2015 08:27:59 AM UTC, comment #9: 

Please try out the attached patch.  It is more along the lines of what I think is good flow for shutdown events.  The "state variable" approach is gone, i.e., some variable is used to indicate multiple function calls, or block multiple function calls.  In my experience, I've never seen those convoluted sorts of approaches work real well.  I think once the programmer tunes into the changes in the patch, it all becomes much easier to work with.

The key changes are the following.

1) Instead of having the GUI attempt to replicated what the normal Octave shutdown does, I've placed the following right at the front of Octave's "quit" and "exit" routines:


@@ -806,6 +806,12 @@ to run using @code{atexit}.\n\
 {
   octave_value_list retval;

+  // Confirm OK to shutdown.  Note: A dynamic function installation similar
+  // to overriding polymorphism for which the GUI can install its own "quit"
+  // yet call this base "quit" could be nice.  No link would be needed here.
+  if (! octave_link::confirm_shutdown ())
+    return retval;
+
   if (! quit_allowed)
     error ("quit: not supported in embedded mode");
   else


2) Rather than place a callback function in some event queue, put "exit" in the command queue.

@@ -985,8 +1012,7 @@ void
 main_window::closeEvent (QCloseEvent *e)
 {
   e->ignore ();
-  if (confirm_exit_octave())
-    octave_link::post_event (this, &main_window::exit_callback);
+  queue_command ("exit");
 }

 void


The reason I consider this more robust is that the same exact flow of events is being used to to exit no matter if the user types "exit" at the command line or the GUI's close-app button or shortcut keys are used to effectively do the same thing.  When done in this fashion, there is no tricky use of state variables because the shutdown confirm and actual forced exit can be handle separately via two simple signal/slot combos.

One aspect that works the shutdown confirmation work is that the worker process (Octave core) goes into a sleep state while the GUI attempts to shutdown.  If the user cancels, a cross-thread return variable is set before the worker thread is awoken to continue its exit process if warranted.

The other changes have to do with the closing of the string of editor tabs.  I've broken that up into smaller routines, of sorts, to give better flexibility.  I think you will like the new behavior a little better.

More than that, the new setup is much easier to work with.  I'm sure there will be some quirks here, but tweaking is very easy now.  For example, I see that shutting down with modified files does not bring the "Editor" tab to focus.  It brings the specific file to focus, but that file doesn't appear unless the Editor tab is visible.  We'll probably have to also bring the Editor tab into focus under that condition.  But these sorts of things are the gist of the changeset.

(file #33073)

Dan Sebald <sebald>
Thu 12 Feb 2015 09:07:22 PM UTC, comment #8: 

Correct.

Dan Sebald <sebald>
Thu 12 Feb 2015 08:55:24 PM UTC, comment #7: 

I can not reproduce the first issue, but I now get your second point (I guess we were talking about different problems here). The CheckClose () you mention in comment #3 should be an unconditional close since all saves were done in the first loop, right?

Torsten Lilge <ttl>
Group Member
Thu 12 Feb 2015 08:04:54 PM UTC, comment #6: 

I tried the patch.  Somehow I ran across the situation where I saved a file, exited and then the editor said a file was changed outside the editor and asked if I would like to reload.  I indicated "no" and then Octave did not finish exiting.

Anyway, I still think that Octave should save/inquire/confirm all files, and then close.  Not save/confirm/close individually.  If there are two files not saved when exiting, I confirm the first and then cancel the second.  Then exit and when open again the open file list is not the same as when I first initiated.  I don't think that is the expectation of the user.

I'll take a shot at both of the issues here, but I will do it in separate changesets, one for better editor interaction, one for cross-thread exit flow that is more close to what I think the setup should be like.

Dan Sebald <sebald>
Thu 12 Feb 2015 06:52:34 PM UTC, comment #5: 

Yes, well the source of the problem is lack of a means to override the behavior of "exit" at the command line.  There are many commands in Octave that potentially initiate some action in the GUI, not just "exit".  Not enough thought went into that at the very start of the GUI project.

There might be something a little tricky that could be done in the special case of "exit".  We could check the command being sent to Octave core and if we find "exit", do the save/confirm/cancel method before sending that command.  But that doesn't work either because although I doubt there is any need for the user to redefine what "exit" means via a script file, there are tricky parsing issues like if the user enters "x = y+3; save x; exit".

I will look at your changeset and consider this issue, but I'm pretty sure we need exit redefined--if that is just a contrived, cross-thread GUI call for the short term.

Dan Sebald <sebald>
Thu 12 Feb 2015 07:32:49 AM UTC, comment #4: 

IMHO having two separate routines for saving and closing does not solve the following conflict when exiting the application via the gui (Ctrl-Q, Alt-F4, ...):

  • We have to save the session and check whether possible modifications have to be saved or discarded, or whether the user wants to cancel exiting
  • When the user definitely wants to exit, we call octave to exit via Fquit
  • Now we are entering the same path as if "exit" was entered into the console (where saving session and checking for modifications have to be done, too) and we have to prevent that in our case (gui exit),
    • we save the session again but with already closed files or
    • we still have unchanged files open and ask the user again if modifications have to be saved


I have attached a patch that at least does not need to call check_closing from the editor's destructor. But still there is a flag indicating that exiting was initiated by the gui. However, it is really possible that I got stuck in my way of viewing the problem and there is a much more elegant solution.



(file #33057)

Torsten Lilge <ttl>
Group Member
Wed 11 Feb 2015 07:19:15 PM UTC, comment #3: 

I must have not formatted something correctly and the bulk of my comment was deleted.  OK, retype...

I've looked at this patch and would like to revisit this. The close mechanism looks a little more clumsy than it needs to be, highlighted by this line of code:


  // Here, we really want to exit and all tabs are closed
  _check_closing_done = true;  // check is already done, prevent a second check
                               // which would store an empty file list


Whenever I see that kind of a construct in an object-oriented framework I think there should be a better solution, given past experience.  A ground-upward solution that reuses smaller routines inside more comprehensive functions.  I think rather than trying to do the save/close simultaneously, it's better to go through the list twice, first save then close.

I assume that when the user closes a tab when not exiting (i.e., just normal operation), there is a check for save, then close.  Let's call that CheckSave().  (I'll use contrived names for illustration.)  Something like:


void CheckClose ()
{
  if (CheckSave() != CANCEL)
    Close();
}


Now, upon "exit", if we go through the list and first just save, the user canceling will not have closed any tabs, so the user can type exit again and again without having any tabs closed, so no need for  _check_closing_done.  That is:


  // Save all tabs, checking for user cancel.
  file_editor_tab *editor_tab;
  for (int index = _tab_widget->count ()-1; index >= 0; index--)
    {
      editor_tab = static_cast <file_editor_tab *> (_tab_widget->widget (index));
      if ((! editor_tab->CheckSave (closing_state)) && closing_state == 1)
        return false;
    }

  // Close all tabs.  All files were just saved so checking is redundant.
  for (int index = _tab_widget->count ()-1; index >= 0; index--)
    {
      editor_tab = static_cast <file_editor_tab *> (_tab_widget->widget (index));
      editor_tab->CheckClose ();
    }


Also, let's try to get that static_cast out of the code.  In this case it isn't too dangerous, but in my mind, using a cast means we are deviating from the paradigm of the widget framework.

Should I attempt a changeset?  Or do you understand what I'm saying and can fix things in a straightforward way?

Dan Sebald <sebald>
Wed 11 Feb 2015 06:56:21 PM UTC, comment #2: 

I've looked at this patch and would like to revisit this.  The close mechanism looks a little more clumsy than it needs to be, highlighted by this line of code:


  // Here, we really want to exit and all tabs are closed
  _check_closing_done = true;  // check is already done, prevent a second check
                               // which would store an empty file list
-varbatim-

Whenever I see that kind of construct, I've learned that in an object-oriented framework there is likely a much better way, one that typically involves building from the ground up using more basic routines that can be pieced together and reused in more comprehensive functions.

I assume that when not at exit, and the user presses close for a tab, there is a dialog that pops up if the file is not saved asking SAVE | DISCARD | CANCEL ?.  Let's call this operation CheckSave().  (I'm just making up function names for illustration.)  So, when a tab X is selected, we have something like:

+verbatim+
void CheckClose()
{
  if (CheckSave() != CANCELLED)
    Close();
}


Now, this file_editor::check_closing (int closing_state) routine attempts to combine the checking and closing at once for the whole list which leads to the problem that _check_closing_done is meant to solve.  Instead, I think going through the list twice, first checking/saving, and then closing solves the problem.  If the first loop doesn't make it all the way through because the user types cancel somewhere, then all the files will still be open and typing "exit" again and again will always have the same files open in the editor.  That is:


  // Save all tabs, checking if user cancels.
  file_editor_tab *editor_tab;
  for (int index = _tab_widget->count ()-1; index >= 0; index--)
    {
      editor_tab = static_cast <file_editor_tab *> (_tab_widget->widget (index));
      if ((! editor_tab->CheckSave (closing_state)) && closing_state == 1)
        return false;
    }

  // Close all tabs, they were all just saved so checking is redundant.
  for (int index = _tab_widget->count ()-1; index >= 0; index--)
    {
      editor_tab = static_cast <file_editor_tab *> (_tab_widget->widget (index));
      editor_tab->CheckClose ();
    }


Also, let's try to get that static_cast out of there.  It's pretty safe in this case, but using a static cast, in my mind, means we are moving outside the paradigm of the Widget framework.  There might be a way we can use one of the widget's virtual functions to access the file editor tab's close function.

Do you want me to work on a change set?  Or do you understand what I'm saying and can rework things quickly?

Dan Sebald <sebald>
Tue 10 Feb 2015 09:30:09 PM UTC, comment #1: 

Thanks for the report. This side effect of a previous changeset (possibility to cancel closing in case of unsaved files) is fixed with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/b30cd22e6471

Torsten Lilge <ttl>
Group Member
Tue 10 Feb 2015 08:03:39 AM UTC, original submission:  

With some files open in the file editor, exit Octave with "exit" at the command line.  Open Octave and the files are present in the editor.  Now, close all the files and exit Octave with "exit" at the command line.  Open Octave and the files are opened again when they shouldn't be.

Repeat the above process, but this time use the "Exit Cntrl-Q" of the File dropdown menu.  The files do not return to the editor on the second third launch, as they shouldn't.

It seems somehow the "exit" at the command line that triggers the GUI to close is not calling the part of the GUI that saves the state of the file editor.

Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #33075:  octave-shutdown-djs2014feb14_1130.patch added by sebald (28KiB - application/octet-stream)
file #33073:  octave-shutdown-djs2014feb14.patch added by sebald (27KiB - application/octet-stream)
file #33057:  check_exit_20140212.patch added by ttl (3KiB - text/x-diff)

 

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 sebald (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-14 sebald Attached File- Added octave-shutdown-djs2014feb14_1130.patch, #33075
    2015-02-14 sebald Attached File- Added octave-shutdown-djs2014feb14.patch, #33073
    2015-02-12 ttl Attached File- Added check_exit_20140212.patch, #33057
    2015-02-10 ttl StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code