Skip to content

Implemented Undo support in EventView #83

Merged
merged 1 commit into from Jun 11, 2012

3 participants

@NicholasVanSickle

No description provided.

@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/Commands/CommandDeleteEvent.cpp
@@ -24,10 +24,26 @@ bool CommandDeleteEvent::execute( ControllerInterface* controller )
return controller->deleteEvent( m_event );
}
+bool CommandDeleteEvent::rollback(ControllerInterface *controller)
+{
+ int oid = m_event.id();
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

oid and nid aren't great variable names. oldId and newId (I assume that's what you mean)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/Commands/CommandRelayCommand.cpp
@@ -12,7 +12,7 @@
}
CommandRelayCommand::~CommandRelayCommand()
-{
+{
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Unnecessary whitespace change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on the diff Jun 4, 2012
Charm/TasksView.cpp
@@ -420,7 +420,6 @@ void TasksView::configureUi()
void TasksView::commitCommand( CharmCommand* command )
{
command->finalize();
- delete command;
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Where are the command deletions happening now?

It's handled by the QUndoStack they're pushed to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
@@ -61,6 +63,24 @@
// SLOT( slotCommitTimeout() ) );
// m_commitTimer.setSingleShot( true );
+ m_actionUndo.setText(tr("Undo"));
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Inconsistent indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
+void EventView::slotUndoTextChanged(const QString &text)
+{
+ m_actionUndo.setText(tr("Undo %1").arg(text));
+}
+
+void EventView::slotRedoTextChanged(const QString &text)
+{
+ m_actionRedo.setText(tr("Redo %1").arg(text));
+}
+
+void EventView::slotEventIdChanged(int oid, int nid)
+{
+ foreach(QObject* o, m_undoStack->children()) {
+ UndoCharmCommandWrapper* w = dynamic_cast<UndoCharmCommandWrapper*>(o);
+ Q_ASSERT(w);
+ w->command()->eventIdChanged(oid, nid);
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

w is not a great variable name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
@@ -338,6 +369,25 @@ void EventView::slotUpdateCurrent()
slotUpdateTotal();
}
+void EventView::slotUndoTextChanged(const QString &text)
+{
+ m_actionUndo.setText(tr("Undo %1").arg(text));
+}
+
+void EventView::slotRedoTextChanged(const QString &text)
+{
+ m_actionRedo.setText(tr("Redo %1").arg(text));
+}
+
+void EventView::slotEventIdChanged(int oid, int nid)
+{
+ foreach(QObject* o, m_undoStack->children()) {
+ UndoCharmCommandWrapper* w = dynamic_cast<UndoCharmCommandWrapper*>(o);
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

This could perhaps just be a QObject and use qobject_cast instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
@@ -338,6 +369,25 @@ void EventView::slotUpdateCurrent()
slotUpdateTotal();
}
+void EventView::slotUndoTextChanged(const QString &text)
+{
+ m_actionUndo.setText(tr("Undo %1").arg(text));
+}
+
+void EventView::slotRedoTextChanged(const QString &text)
+{
+ m_actionRedo.setText(tr("Redo %1").arg(text));
+}
+
+void EventView::slotEventIdChanged(int oid, int nid)
+{
+ foreach(QObject* o, m_undoStack->children()) {
+ UndoCharmCommandWrapper* w = dynamic_cast<UndoCharmCommandWrapper*>(o);
+ Q_ASSERT(w);
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

I'd make this a check in release mode too to avoid dereferencing the null pointer (which will just crash anyway making the assert fairly pointless)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
@@ -417,17 +467,19 @@ void EventView::slotEditEvent( const Event& event )
EventEditor editor( event, this );
if( editor.exec() ) {
Event newEvent = editor.eventResult();
+
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Unnecessary whitespace change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.cpp
@@ -435,7 +487,7 @@ void EventView::slotEditEvent( const Event& event )
void EventView::slotEditEventCompleted( const Event& event )
{
CommandModifyEvent* command =
- new CommandModifyEvent( event, this );
+ new CommandModifyEvent( event, event, this );
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Not that it's incorrect but it seems strange to pass the same argument twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/EventView.h
QList<NamedTimeSpan> m_timeSpans;
Event m_event;
EventModelFilter* m_model;
+ QAction m_actionUndo;
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Indentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Charm/ModelConnector.cpp
@@ -64,9 +63,9 @@ void ModelConnector::slotMakeAndActivateEvent( const Task& task )
VIEW.sendCommand( command );
}
-void ModelConnector::slotRequestEventModification( const Event& event )
+void ModelConnector::slotRequestEventModification( const Event& event, const Event& old )
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Old what? Better variable name would make this a bit more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid and 2 others commented on an outdated diff Jun 4, 2012
Charm/UndoCharmCommandWrapper.cpp
+ setText(command->description());
+}
+
+void UndoCharmCommandWrapper::undo()
+{
+ m_command->requestRollback();
+}
+
+void UndoCharmCommandWrapper::redo()
+{
+ m_command->requestExecute();
+}
+
+CharmCommand *UndoCharmCommandWrapper::command()
+{
+ return m_command;
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Considering this is just a wrapper I'd be tempted to just make m_command public. @frankosterfeld may disagree :)

From a design standpoint it's pretty marginal either way, I think. I like having the getter abstraction there just in case, but it's pretty unlikely something would change.

@frankosterfeld
KDAB member

Method should be const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Core/CharmCommand.cpp
CommandEmitterInterface* CharmCommand::owner() const
{
return m_owner;
}
+void CharmCommand::requestExecute()
+{
+ emit emitExecute(this);
+}
+
+void CharmCommand::requestRollback()
+{
+ emit emitRollback(this);
+}
+
+void CharmCommand::requestSlotEventIdChanged(int oid, int nid)
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Bit of a confusing method name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on the diff Jun 4, 2012
Core/CharmDataModel.cpp
@@ -424,9 +424,10 @@ void CharmDataModel::endEventRequested( const Task& task )
Q_ASSERT( eventId != 0 );
Event& event = findEvent( eventId );
+ Event old = event;
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Could this be a reference?

It could not, the next line I call event.setEndDateTime which modifies the event, old is deliberately copy constructed

@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Cool, just checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid MikeMcQuaid commented on an outdated diff Jun 4, 2012
Core/Controller.h
@@ -43,8 +44,8 @@ class Controller : public QObject,
void updateModelEventsAndTasks();
public slots:
-
@MikeMcQuaid
MikeMcQuaid added a note Jun 4, 2012

Whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NicholasVanSickle NicholasVanSickle commented on the diff Jun 4, 2012
Charm/UndoCharmCommandWrapper.cpp
+UndoCharmCommandWrapper::~UndoCharmCommandWrapper()
+{
+ delete m_command;
+}
+
+void UndoCharmCommandWrapper::undo()
+{
+ m_command->requestRollback();
+}
+
+void UndoCharmCommandWrapper::redo()
+{
+ m_command->requestExecute();
+}
+
+CharmCommand *UndoCharmCommandWrapper::command() const

Method is now const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid

Tested and merged.

@MikeMcQuaid MikeMcQuaid merged commit bb9d778 into KDAB:master Jun 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.