Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add confirmation dialogue on exit with unsaved changes. #74

Closed
wants to merge 5 commits into from
Closed

Add confirmation dialogue on exit with unsaved changes. #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9899099
Squashed commit of the following:
AndyZe Apr 11, 2016
8ba43a5
Detecting tree changes with slots & signals
AndyZe Apr 19, 2016
bf26bde
The logic behind saving is good, now I need to reformat saveURDF func…
AndyZe Apr 20, 2016
5038982
Reformatted saveURDF() to minimize duplicate code.
AndyZe Apr 20, 2016
b06bf68
Tweaking closeEvent() and quit() for best performance.
AndyZe Apr 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions urdf_editor/include/urdf_editor/industrial_robot_builder.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
<addaction name="action_Save"/>
<addaction name="actionSave_As"/>
<addaction name="separator"/>
<addaction name="actionE_xit"/>
<addaction name="action_Exit"/>
</widget>
<addaction name="menu_File"/>
</widget>
Expand Down Expand Up @@ -184,12 +184,12 @@
<string>Ctrl+N</string>
</property>
</action>
<action name="actionE_xit">
<action name="action_Exit">
<property name="icon">
<iconset theme="application-exit"/>
</property>
<property name="text">
<string>E&amp;xit</string>
<string>&amp;Exit</string>
</property>
<property name="shortcut">
<string>Ctrl+Q</string>
Expand Down
6 changes: 5 additions & 1 deletion urdf_editor/include/urdf_editor/urdf_editor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ private slots:

void on_action_Save_triggered();

bool unsaved_changes();

void on_actionSave_As_triggered();

void on_action_New_triggered();

void on_actionE_xit_triggered();
void on_action_Exit_triggered();

private:
Ui::URDFEditor *ui;
Expand All @@ -37,6 +39,8 @@ private slots:

urdf_editor::URDFPropertySharedPtr urdf_tree_;
QString file_path_;

void closeEvent(QCloseEvent *event);
};

#endif // __URDF_EDITOR_H__
13 changes: 13 additions & 0 deletions urdf_editor/include/urdf_editor/urdf_property.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ namespace urdf_editor

bool clear();

bool unsavedChanges;

private slots:
void on_treeWidget_customContextMenuRequested(const QPoint &pos);

Expand All @@ -41,6 +43,17 @@ namespace urdf_editor

void on_propertyWidget_valueChanged();

void on_unsavedChanges();

signals:
void jointAddition();

void jointDeletion();

void linkAddition();

void linkDeletion();

private:
bool populateTreeWidget();

Expand Down
76 changes: 69 additions & 7 deletions urdf_editor/src/urdf_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <rviz/visualization_manager.h>

#include <QFileDialog>
#include <QMessageBox>

#include <qteditorfactory.h>
#include <qtpropertymanager.h>
Expand Down Expand Up @@ -43,9 +44,38 @@ void URDFEditor::on_action_Open_triggered()
}
}

bool URDFEditor::unsaved_changes()
{
QMessageBox::StandardButton reply;
reply = QMessageBox::question(this, "Save changes?", "Save your changes before exiting?", QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel);

if (reply == QMessageBox::Yes)
{
on_actionSave_As_triggered();
return true;
}
if (reply == QMessageBox::Cancel)
return false;
if (reply == QMessageBox::No)
return true;
}

void URDFEditor::on_action_Save_triggered()
{
urdf_tree_->saveURDF(file_path_);
QMessageBox msgBox;
if (urdf_tree_->saveURDF(file_path_))
{
urdf_tree_->unsavedChanges = false;
msgBox.setWindowTitle("Success");
msgBox.setText("The file was saved.");
msgBox.exec();
}
else
{
msgBox.setWindowTitle("FAILURE");
msgBox.setText("An error occurred during saving.");
msgBox.exec();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation, and did you have a particular reason to still show the dialogue for the case saving completes successfully? From your earlier comments I figured you wanted to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the confirmation useful after "Save" but annoying after "Save As," because there are 3 other boxes to click through on "Save As."

}

void URDFEditor::on_actionSave_As_triggered()
Expand All @@ -54,19 +84,51 @@ void URDFEditor::on_actionSave_As_triggered()
if (!file_path.isEmpty())
{
file_path_ = file_path;
urdf_tree_->saveURDF(file_path);
ui->action_Save->setDisabled(false);

QMessageBox msgBox;
if (!urdf_tree_->saveURDF(file_path))
{
msgBox.setWindowTitle("FAILURE");
msgBox.setText("An error occurred during saving.");
msgBox.exec();
}
else
{
urdf_tree_->unsavedChanges = false;
ui->action_Save->setDisabled(false);
}
}
}

void URDFEditor::on_action_New_triggered()
{
file_path_.clear();
urdf_tree_->clear();
ui->action_Save->setDisabled(true);
file_path_.clear();
urdf_tree_->clear();
ui->action_Save->setDisabled(true);
}

void URDFEditor::on_action_Exit_triggered()
{
QCloseEvent closing;
closeEvent( &closing );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have misunderstood the QCloseEvent documentation, but doesn't Qt call that method for us? Are we supposed to call that ourselves?


void URDFEditor::on_actionE_xit_triggered()
void URDFEditor::closeEvent( QCloseEvent *event )
{
if ( urdf_tree_->unsavedChanges == true ) // If there were unsaved changes
{
if ( !unsaved_changes() ) // User canceled the quit
event->ignore();
else
{
event->accept();
QMainWindow::closeEvent(event);
QApplication::quit();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here: from the example I got the idea that just ignoring or accepting the event would be enough to have Qt continue the application close sequence. From the code here it seems that is not the case, and an explicit quit() is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gavanderhoorn quit() is needed when the user tries to quit by File->Exit. Otherwise the window remains open.

Clicking the X to exit works perfectly without quit().

I'm not sure why the X and File->Exit are set up differently. It seems like they should have the same behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more testing, I found that closeEvent() can be skipped and quit() suffices for both cases (X and File->Exit). However, closeEvent() "looks nicer" because it closes the windows rapidly. I suggest keeping it the way it will be on this upcoming commit. Sorry that I don't have a better explanation.

}
else
{
event->accept();
QApplication::quit();
}
}
38 changes: 23 additions & 15 deletions urdf_editor/src/urdf_property.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

#include <QVBoxLayout>
#include <QMessageBox>

#include <urdf_parser/urdf_parser.h>

Expand Down Expand Up @@ -71,6 +70,16 @@ namespace urdf_editor
connect(property_editor_.get(), SIGNAL(customContextMenuRequested(QPoint)),
this, SLOT(on_propertyWidget_customContextMenuRequested(QPoint)));

connect(this, SIGNAL(jointAddition()), SLOT(on_unsavedChanges()));

connect(this, SIGNAL(jointDeletion()), SLOT(on_unsavedChanges()));

connect(this, SIGNAL(linkAddition()), SLOT(on_unsavedChanges()));

connect(this, SIGNAL(linkDeletion()), SLOT(on_unsavedChanges()));

unsavedChanges=false; // No changes to be saved, yet

}

URDFProperty::~URDFProperty()
Expand Down Expand Up @@ -124,20 +133,7 @@ namespace urdf_editor
bool savedCorrectly = false;
savedCorrectly = doc->SaveFile(file_path.toStdString());

QMessageBox msgBox;
if (savedCorrectly)
{
msgBox.setWindowTitle("Success");
msgBox.setText("The file was saved.");
}
else
{
msgBox.setWindowTitle("FAILURE");
msgBox.setText("An error occurred during saving.");
}
msgBox.exec();

return true;
return savedCorrectly;
}

bool URDFProperty::populateTreeWidget()
Expand Down Expand Up @@ -211,6 +207,8 @@ namespace urdf_editor
QTreeWidgetItem* item = addLinkTreeItem(parent, new_link);
// now add the property
addLinkProperty(item, new_link);
emit linkAddition();
unsavedChanges = true;
}

QTreeWidgetItem* URDFProperty::addLinkTreeItem(QTreeWidgetItem* parent, urdf::LinkSharedPtr link)
Expand Down Expand Up @@ -251,6 +249,8 @@ namespace urdf_editor
QTreeWidgetItem* item = addJointTreeItem(parent, new_joint);
// now add the property
addJointProperty(item, new_joint);
unsavedChanges = true;
emit jointAddition();
}

QTreeWidgetItem* URDFProperty::addJointTreeItem(QTreeWidgetItem* parent, urdf::JointSharedPtr joint)
Expand Down Expand Up @@ -333,11 +333,13 @@ namespace urdf_editor
{
link_names_.removeOne(sel->text(0));
link_root_->removeChild(sel);
emit linkDeletion();
}
else if (isJoint(sel))
{
joint_names_.removeOne(sel->text(0));
sel->parent()->removeChild(sel);
emit jointDeletion();
}
}
}
Expand Down Expand Up @@ -715,6 +717,12 @@ namespace urdf_editor
void URDFProperty::on_propertyWidget_valueChanged()
{
rviz_widget_->loadRobot(model_);
unsavedChanges = true;
}

void URDFProperty::on_unsavedChanges()
{
unsavedChanges = true;
}

}