Confirmation dialog when closing main window #225

francocipollone · 2021-05-26T15:59:11Z

Signed-off-by: Franco Cipollone [email protected]

🎉 New feature

Summary

A new property (dialog_on_exit) was added to the WindowConfig struct to enable displaying a confirmation dialog when exiting the window.
In the .config file:

<!-- Window -->
<window>
  <width>1000</width>
  <height>845</height>
  ...
  ...
  <dialog_on_exit>true</dialog_on_exit>    // New Feature
</window>

Using this property in gazebo: ign-gazebo branch

confirmation_dialog.mp4

Test it

// bring this changes.
cd ign-gui
git checkout francocipollone/citadel_confirmation_dialog
cd ../
// Enable this feature for gazebo app.
// Just one line of code.
cd ign-gazebo
git checkout francocipollone/citadel_confirmation_dialog
// Build and run gazebo.
colcon build --packages-select ign-gui3 ign-gazebo3 
ign gazebo

Press the X button or Quit from the menu.

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

jennuine

Looks good! I left a couple comments

jennuine · 2021-05-26T17:31:58Z

include/ignition/gui/MainWindow.hh

+      /// \brief Show the confirmation dialog on exit
+      bool showDialogOnExit{false};


New publicly visible data members break ABI, to avoid this you can add it to the MainWindowPrivate class in MainWindow.cc and access it using this->dataPtr

Oh my bad, you are right. Thanks for the suggestion.

Check 50ac123:
In order to avoid breaking ABI I added that member variable to MainWindowPrivate class and it was removed from WindowsConfig. This change made me move one level up the parsing of the dialog_on_exit XML node (to Application.cc). Besides, the dialog_on_exit XML attribute isn't part of window XML node anymore (gazebosim/gz-sim@c3d6718).
It ended up not being as tidy as before. PTAL

jennuine · 2021-05-26T17:41:51Z

src/MainWindow.cc

@@ -470,6 +477,12 @@ bool WindowConfig::MergeFromXML(const std::string &_windowXml)
    }
  }

+  // Show dialog on exit
+  if (auto dialogOnExitElem = winElem->FirstChildElement("dialog_on_exit"))


In my opinion, I think the confirmation dialog should always appear by default (regardless if <dialog_on_exit>true</dialog_on_exit> is in gui.config or not) and when the user does not want the confirmation then they would set <dialog_on_exit>false</dialog_on_exit> but lets see what others think

cc @mabelzhang @ahcorde

I don't have a strong opinion either way. I think it is safer for it to be on by default, but then I checked Gazebo-classic, and it doesn't seem to have a confirmation dialog. So... 🤷‍♀️

In general I would to stick with the current behaviour and document this new feature. I open and close many times ign-gazebo and this is adding an overhead for me.

Maybe we can discuss in the next ignition meeting

I think that leaving this off by default is the safer option.
Although ignition-gazebo is the direct consumer of ignition-gui we probably don't want to change the behavior of all the applications that are developed on top of ignition::gui::Application (extern projects).

Signed-off-by: Franco Cipollone <[email protected]>

jennuine

Looks good, left a minor comment. Also, there are a couple of tests failing. UNIT_Application_TEST might be flaky but UNIT_MainWindow_TEST is seg faulting at MainWindowTest.Constructor

jennuine · 2021-06-01T15:42:09Z

src/Application.cc

+  // Closing behavior.
+  if (auto dialogOnExitElem = doc.FirstChildElement("dialog_on_exit"))
+  {
+    bool showDialogOnExit{false};
+    dialogOnExitElem->QueryBoolText(&showDialogOnExit);
+    this->dataPtr->mainWin->SetShowDialogOnExit(showDialogOnExit);
+  }


I think the previous behavior where <dialog_on_exit> was in <window> was better. You can move this section to the above if (auto winElem = doc.FirstChildElement("window")) and then update this if to if (auto dialogOnExitElem = winElem->FirstChildElement("dialog_on_exit"))

Agree, from a user perspective it is better to have it within the <window> node.
Done!

Signed-off-by: Franco Cipollone <[email protected]>

codecov · 2021-06-02T14:10:30Z

Codecov Report

Merging #225 (65c1fd2) into ign-gui3 (bf50949) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 65c1fd2 differs from pull request most recent head 6db723c. Consider uploading reports for the commit 6db723c to get more accurate results

@@             Coverage Diff              @@
##           ign-gui3     #225      +/-   ##
============================================
+ Coverage     65.56%   65.68%   +0.12%     
============================================
  Files            23       23              
  Lines          2817     2827      +10     
============================================
+ Hits           1847     1857      +10     
  Misses          970      970

Impacted Files	Coverage Δ
src/Application.cc	`84.89% <100.00%> (+0.20%)`	⬆️
src/MainWindow.cc	`96.93% <100.00%> (+0.04%)`	⬆️

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 bf50949...6db723c. Read the comment docs.

jennuine

Thanks for iterating! Can you add a test for codecov? Adding <window> and <dialog_on_exit> to test/config/test.config should suffice. Also, add the new <dialog_on_exit> element under "Overview" to layout tutorials

Signed-off-by: Franco Cipollone <[email protected]>

francocipollone · 2021-06-02T17:38:15Z

Thanks for iterating! Can you add a test for codecov? Adding <window> and <dialog_on_exit> to test/config/test.config should suffice. Also, add the new <dialog_on_exit> element under "Overview" to layout tutorials

Thanks for the quick re-review!
Addressed! Check ceaaedc and 6db723c

* Release 3.5.1 (#195) Signed-off-by: Steve Peters <[email protected]> * check_test_ran.py: remove grep/xsltproc (#203) We aren't using QTest anymore in ign-gui, so remove the parts of check_test_ran.py that translated QTest xml files into junit, since they don't work easily on Windows and aren't needed any longer. Fixes #198. Signed-off-by: Steve Peters <[email protected]> * Fixed material specular in scene3D (#218) Signed-off-by: ahcorde <[email protected]> * Remove tools/code_check and update codecov (#222) Signed-off-by: Louise Poubel <[email protected]> * Removed duplicated code with rendering::sceneFromFirstRenderEngine (#223) Signed-off-by: ahcorde <[email protected]> * Emit more events from Scene3D (#213) * Start porting events from ign-gazebo Signed-off-by: Louise Poubel <[email protected]> * Remove test that fails due to #216 Signed-off-by: Louise Poubel <[email protected]> * Move Scene3d_TEST.cc to test/integration There seems to be a problem with loading the ignition-rendering-ogre plugin from the Scene3D test if it links to that plugin. Making Scene3D_TEST.cc into an integration test works because it doesn't directly call any plugin methods. This also changes the linking for the Grid3D plugin to only link to the ignition-rendering core library target instead of the IGNITION-RENDERING_LIBRARIES variable which includes the ogre component library plugins. Signed-off-by: Steve Peters <[email protected]> * process qt events to allow scene to initialize Signed-off-by: Steve Peters <[email protected]> * Add test helper to check event Signed-off-by: Louise Poubel <[email protected]> * added more tests Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Move TestHelper code to .cc file, fix windows? Signed-off-by: Steve Peters <[email protected]> * Fix windows? Signed-off-by: ahcorde <[email protected]> * Fix windows? Signed-off-by: Alejandro Hernández <[email protected]> * Expect values of Vector3 point in click events Signed-off-by: Steve Peters <[email protected]> * Remove debug message Signed-off-by: Steve Peters <[email protected]> * Remove unused variable Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: ahcorde <[email protected]> * Avoid grid3D crash (#227) Signed-off-by: ahcorde <[email protected]> * Confirmation dialog when closing main window (#225) * Adds confirmation dialog when closing window * Updates docs and extends test coverage. * Adds dialog_on_exit atribute to example .config Signed-off-by: Franco Cipollone <[email protected]> * update codeowners (#232) Signed-off-by: Jenn Nguyen <[email protected]> * 🎈 3.6.0 (#233) Signed-off-by: Louise Poubel <[email protected]> * Bump required ign-rendering version to 4.8 (#234) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Franco Cipollone <[email protected]> Co-authored-by: Jenn Nguyen <[email protected]>

francocipollone added enhancement New feature or request 🏰 citadel Ignition Citadel labels May 26, 2021

francocipollone requested a review from chapulina as a code owner May 26, 2021 15:59

jennuine requested changes May 26, 2021

View reviewed changes

francocipollone added 2 commits May 31, 2021 18:56

Adds confirmation dialog when closing window

957e569

Signed-off-by: Franco Cipollone <[email protected]>

Avoids breaking ABI.

50ac123

Signed-off-by: Franco Cipollone <[email protected]>

francocipollone force-pushed the francocipollone/citadel_confirmation_dialog branch from a8367c9 to 50ac123 Compare May 31, 2021 21:57

francocipollone requested a review from jennuine May 31, 2021 22:22

jennuine reviewed Jun 1, 2021

View reviewed changes

Moves <dialog_on_exit> node back to the window node.

0124e27

Signed-off-by: Franco Cipollone <[email protected]>

francocipollone requested a review from jennuine June 2, 2021 14:18

jennuine reviewed Jun 2, 2021

View reviewed changes

francocipollone added 2 commits June 2, 2021 14:19

Updates docs and extends test coverage.

ceaaedc

Signed-off-by: Franco Cipollone <[email protected]>

Adds dialog_on_exit atribute to example .config

6db723c

Signed-off-by: Franco Cipollone <[email protected]>

jennuine approved these changes Jun 2, 2021

View reviewed changes

scpeters approved these changes Jun 3, 2021

View reviewed changes

scpeters merged commit dd80dc9 into ign-gui3 Jun 3, 2021

scpeters deleted the francocipollone/citadel_confirmation_dialog branch June 3, 2021 19:53

francocipollone mentioned this pull request Jun 4, 2021

Enables confirmation dialog when closing Gazebo. gazebosim/gz-sim#850

Merged

8 tasks

mjcarroll mentioned this pull request Jun 21, 2021

Confirmation dialog on exiting Ignition gazebosim/gz-sim#820

Closed

jennuine mentioned this pull request Aug 25, 2022

Added Reset button to world_control #476

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirmation dialog when closing main window #225

Confirmation dialog when closing main window #225

francocipollone commented May 26, 2021 •

edited

Loading

jennuine left a comment

jennuine May 26, 2021

francocipollone May 31, 2021 •

edited

Loading

jennuine May 26, 2021

mabelzhang May 26, 2021

ahcorde May 26, 2021

francocipollone May 31, 2021

jennuine left a comment

jennuine Jun 1, 2021

francocipollone Jun 2, 2021

codecov bot commented Jun 2, 2021 •

edited

Loading

jennuine left a comment

francocipollone commented Jun 2, 2021

		/// \brief Show the confirmation dialog on exit
		bool showDialogOnExit{false};

Confirmation dialog when closing main window #225

Confirmation dialog when closing main window #225

Conversation

francocipollone commented May 26, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

jennuine left a comment

Choose a reason for hiding this comment

jennuine May 26, 2021

Choose a reason for hiding this comment

francocipollone May 31, 2021 • edited Loading

Choose a reason for hiding this comment

jennuine May 26, 2021

Choose a reason for hiding this comment

mabelzhang May 26, 2021

Choose a reason for hiding this comment

ahcorde May 26, 2021

Choose a reason for hiding this comment

francocipollone May 31, 2021

Choose a reason for hiding this comment

jennuine left a comment

Choose a reason for hiding this comment

jennuine Jun 1, 2021

Choose a reason for hiding this comment

francocipollone Jun 2, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2021 • edited Loading

Codecov Report

jennuine left a comment

Choose a reason for hiding this comment

francocipollone commented Jun 2, 2021

francocipollone commented May 26, 2021 •

edited

Loading

francocipollone May 31, 2021 •

edited

Loading

codecov bot commented Jun 2, 2021 •

edited

Loading