Added a button that allows shutting down both the client and server.

[peci1]

🎉 New feature

Summary

Adds a button that allows closing the server by calling /server_control::stop service (in case the server runs in a different manager or on a different machine than the gui).

Depends on gazebosim/gz-sim#1240 to be able to actually stop the server.

Test it

ign gazebo -v4 -s &
ign gazebo -v4 -g
# add the Shutdown plugin
# click the cross
# both the GUI and server process should be stopped

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[peci1]

What's weird is that even if I run just the GUI and no server, the node->Request() returns true, so the error message is never written to the console. How is that possible?

[peci1]

Please, also check the QML file after me. I haven't worked with QT quick before, so I pretty much don't know what I'm doing and I just copied the file from WorldControl.

[jennuine]

LGTM, just some minor comments.

Also, instead of creating a new plugin, would it be better to extend the close dialog menu (#225)? We could modify it to say something like, "Cancel", "Shutdown all", "Close GUI only". What are your thoughts @chapulina ?

[peci1]

Thanks for the nits, @jennuine . Regarding the close dialog extension - in my use-case, I want to run the app full-screen and without a window manager, so there is no window close button, and I also hide the hamburger menu with the Quit option. So having a separate plugin providing the close button inside the app is a good way for me.

Maybe the button in the plugin could just trigger the close dialog? If so, it'd be nice to be able to configure the available options for the dialog - e.g. in my use case I would not want to offer to close the GUI only and leave the server running. Would it also be possible to pass more configuration to the dialog, like the name of the server control service?

[chapulina]

instead of creating a new plugin, would it be better to extend the close dialog menu

Yeah I also feel like a plugin to shutdown the application is a bit uncommon.

in my use-case, I want to run the app full-screen and without a window manager, so there is no window close button, and I also hide the hamburger menu with the Quit option.

Sounds like a good use case for

• Allow placing plugins on top of main toolbar #54

This way, the plugin could be more integrated into the UI and look like a native button.

Maybe the button in the plugin could just trigger the close dialog?

That sounds like a good way to reduce duplication, so we have one code path that leads to closing the application.

Would it also be possible to pass more configuration to the dialog, like the name of the server control service?

Those could come under <window>, the inconvenient thing is that this tag can only come from a config file at the moment, not from an SDF for example.

[codecov]

Codecov Report

Merging #335 (b3de450) into ign-gui3 (419613d) will increase coverage by 0.93%.
The diff coverage is 91.52%.

❗ Current head b3de450 differs from pull request most recent head 0eb31aa. Consider uploading reports for the commit 0eb31aa to get more accurate results

@@             Coverage Diff              @@
##           ign-gui3     #335      +/-   ##
============================================
+ Coverage     66.77%   67.71%   +0.93%     
============================================
  Files            25       28       +3     
  Lines          2992     3110     +118     
============================================
+ Hits           1998     2106     +108     
- Misses          994     1004      +10     

Impacted Files	Coverage Δ
src/MainWindow.cc	95.80% <88.70%> (-1.13%)	⬇️
src/Application.cc	85.79% <93.18%> (+1.05%)	⬆️
include/ignition/gui/MainWindow.hh	100.00% <100.00%> (ø)
src/plugins/shutdown_button/ShutdownButton.cc	100.00% <100.00%> (ø)
src/plugins/shutdown_button/ShutdownButton.hh	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 419613d...0eb31aa. Read the comment docs.

[peci1]

Would it also be possible to pass more configuration to the dialog, like the name of the server control service?

Those could come under <window>, the inconvenient thing is that this tag can only come from a config file at the moment, not from an SDF for example.

What? SDF can configure GUI?

[peci1]

Sounds like a good use case for #54

This way, the plugin could be more integrated into the UI and look like a native button.

I'm not sure I can help with #54. So what if this PR makes the close button plugin and once #54 is resolved, it could just be adjusted to better fit the toolbar UI?

[chapulina]

What? SDF can configure GUI?

Just the plugins right now. The plugins inside <world><gui>, such as these, are passed to ign-gui the same way as the <plugin> tags in a .config file.

So what if this PR makes the close button plugin and once #54 is resolved, it could just be adjusted to better fit the toolbar UI?

Yeah I think that's reasonable.

[chapulina]

The general idea is ok to me, just have some minor comments

[peci1]

I've pushed the new implementation which expands on the functionality of the exit confirmation dialog and makes ShutdownButton plugin just a thin layer that invokes this dialog (or performs its action if the dialog is not to be shown).

See examples/config/dialog_on_exit_shutdown.config for the newly added options.

Example run:

ign gazebo -v4 -s &
ign gazebo -v4 -g --gui-config examples/config/dialog_on_exit_shutdown.config

[peci1]

[peci1]

Some things to consider improving:

• The alignment and order of the buttons in the dialog is not fixed - it is determined by the OS. That's pretty bad as e.g. the Linux order is weird. It is because I wanted to retain the standardButtons functionality of the dialog, so the "shutdown" button is represented as DISCARD type button (it actually makes sense, I think).
• I'm not sure if the duality of the possible content of <dialog_on_exit> is okay (it can be either boolean or a XML snippet). See the docs where I tried to describe it.
• Some strings that could possibly be localized in the future moved to .hh/.cc files from the QML (default texts of the buttons and prompt). As you can see from the screenshot, the Cancel button is localized by the OS, which creates a bit messy impression (it is no problem to also add configurable text of the cancel button, but it seemed unnecessary to me).
• I would like to automatically recognize that the GUI and server are in the same process and accordingly simplify the confirmation dialog (i.e. it's not possible to close only the GUI). But that would need to be done in ign-gazebo/src/ign.cc if I'm right?

[jennuine]

I'm not sure if the duality of the possible content of <dialog_on_exit> is okay (it can be either boolean or a XML snippet). See the docs where I tried to describe it.

It is possible but IMO is getting unnecessarily complex. For the confirmation dialog, I think we should simplify things either by:

1. Always show all options (ie., cancel, quit server & gui, quit gui only) in the dialog when <dialog_on_exit>true</dialog_on_exit> is set
2. Add new element to the config, something like <dialog_on_exit_extra> or <extend_dialog_on_exit> which takes a bool (default would be false)

I'm leaning towards (2) because there may be users who do not know (or need to know) about the 2 processes and it would avoid confusion (especially when they are run in the same process; eg, "Shutdown GUI only was clicked but why did it shutdown the server?"). Do you have any thoughts @chapulina ?

[peci1]

Thanks for the suggestions, jennuine. I went with <dialog_on_exit_options> and I like it much more than the previous dual approach. I also managed to move the default texts back to QML, without even needing to add more properties - I just check whether the string coming from configuration is non-empty.

[chapulina]

I would like to automatically recognize that the GUI and server are in the same process and accordingly simplify the confirmation dialog (i.e. it's not possible to close only the GUI). But that would need to be done in ign-gazebo/src/ign.cc if I'm right?

Yeah that would need to be done on Gazebo's side, maybe after it creates the MainWindow on Gui.cc.

---

Thanks for iterating, @peci1, it's working well for me, I just have minor comments.

[peci1]

Thanks for the review, Louise. I'll work through it today.

[peci1]

I added some tests. They're not exactly unit tests of the getters/setters as I find those useless (setters are setting... hmm). I instead wrote them as little integration tests spawning the GUI, displaying the dialog and looking for the elements that are expected to be found.

I had just one difficulty - no matter what I tried, I could not get the list line of ExitDialogDefaultButtons test succeed, so I commented it out with a TODO note. It examines whether the mainwindow stopped being visible after triggering the clicked() signal of one of the close buttons. However, the test tells the window is still there, even though I called QCoreApplication::processEvents(); and waited a bit. So maybe I'm missing some other event processing step. But the signal gets called, because e.g. the server stopping request is published. Any thoughts?

[chapulina]

They're not exactly unit tests of the getters/setters as I find those useless (setters are setting... hmm)

You'd be surprised how many typo and copy/paste errors those catch 😇

instead wrote them as little integration tests spawning the GUI, displaying the dialog and looking for the elements that are expected to be found.

These are much nicer though, thanks!

the test tells the window is still there

I got the test to work with this, but it doesn't look pretty:

diff --git a/include/ignition/gui/qml/Main.qml b/include/ignition/gui/qml/Main.qml
index 7a12f382..70a33c87 100644
--- a/include/ignition/gui/qml/Main.qml
+++ b/include/ignition/gui/qml/Main.qml
@@ -54,6 +54,12 @@ ApplicationWindow
   property bool exitDialogShowCloseGui: MainWindow.exitDialogShowCloseGui
   property string exitDialogShutdownText: MainWindow.exitDialogShutdownText
   property string exitDialogCloseGuiText: MainWindow.exitDialogCloseGuiText
+
+  /**
+   * Flag to indicate if the close event was triggered by the close dialog.
+   */
+  property bool closingFromDialog: false
+
   /**
    * Tool bar background color
    */
@@ -83,7 +89,12 @@ ApplicationWindow
   onClosing: {
     close.accepted = !showDialogOnExit
     if(showDialogOnExit){
-      confirmationDialogOnExit.open()
+      if (closingFromDialog) {
+        close.accepted = true;
+      }
+      else {
+        confirmationDialogOnExit.open();
+      }
     } else if (defaultExitAction == ExitAction.SHUTDOWN_SERVER) {
       MainWindow.OnStopServer()
     }
@@ -373,7 +384,8 @@ ApplicationWindow
       DialogButtonBox {
         onClicked: function (btn) {
           if (btn == this.standardButton(Dialog.Ok)) {
-            Qt.quit()
+            closingFromDialog = true;
+            window.close();
           }
           else if (btn == this.standardButton(Dialog.Discard)) {
             MainWindow.OnStopServer()
@@ -381,7 +393,8 @@ ApplicationWindow
             timer.interval = 100;
             timer.repeat = false;
             timer.triggered.connect(function() {
-              Qt.quit()
+              closingFromDialog = true;
+              window.close();
             });
             timer.start();
           }
diff --git a/src/MainWindow_TEST.cc b/src/MainWindow_TEST.cc
index 357af4e6..6d6a4bd4 100644
--- a/src/MainWindow_TEST.cc
+++ b/src/MainWindow_TEST.cc
@@ -16,6 +16,7 @@
 */
 
 #include <gtest/gtest.h>
+#include <chrono>
 #include <thread>
 
 #include <QQmlProperty>
@@ -40,6 +41,7 @@ char* g_argv[] =
 
 using namespace ignition;
 using namespace gui;
+using namespace std::chrono_literals;
 
 /////////////////////////////////////////////////
 // See https://github.com/ignitionrobotics/ign-gui/issues/75
@@ -603,13 +605,18 @@ TEST(MainWindowTest,
   EXPECT_TRUE(mainWindow->QuickWindow()->isVisible());
   QMetaObject::invokeMethod(
     buttonRoles[ButtonRole::DestructiveRole], "clicked");
-  QCoreApplication::processEvents();
-  std::this_thread::sleep_for(std::chrono::milliseconds(150));
-  QCoreApplication::processEvents();
+
+  int sleep = 0;
+  const int maxSleep = 100;
+  for (; mainWindow->QuickWindow()->isVisible() && sleep < maxSleep; ++sleep)
+  {
+    std::this_thread::sleep_for(100ms);
+    QCoreApplication::processEvents();
+    sleep++;
+  }
+
   EXPECT_TRUE(shutdownCalled);
-  // TODO(peci1) Could not get this to work, probably some more event processing
-  //  is needed
-  // EXPECT_FALSE(mainWindow->QuickWindow()->isVisible());
+  EXPECT_FALSE(mainWindow->QuickWindow()->isVisible());
 }
 
 /////////////////////////////////////////////////

[peci1]

I got the test to work with this, but it doesn't look pretty:

I thinks it's a clean solution, I applied it.

Only the test calling server shutdown needed the wait, as the other closing gui only has no wait in the callback. I hope this works the same on all OSes.

I fixed the asterisk placement.

So I think this PR is ready to go :)

[jennuine]

Looks good, left some minor comments

[jennuine]

Hmm, the Quit Server and GUI button isn't shutting down the server for me

[peci1]

Hmm, the Quit Server and GUI button isn't shutting down the server for me

Do you have up-to-date ign-gazebo 3? The shutdown functionality has only been implemented lately. It works for me locally and it also calls the correct service in tests, so it should work as long as the service server is implemented.

[jennuine]

Do you have up-to-date ign-gazebo 3?

Whoops, I updated the wrong workspace. It works well for me, thanks for iterating!

[peci1]

Hmm, I don't understand either of the CI failures...

[chapulina]

Hmm, I don't understand either of the CI failures...

I believe they're flaky due to

• Deflake X-display tests on GitHub actions #58

[chapulina]

Thanks for iterating, merging!

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1