How to keep my API clean when introducing QUndoCommands?

ModelTech

I have the following challenge and my lack of C++ knowledge is causing me to have no clue how to solve it...

I have a simple class declared as follows in profile.h:

class LIB_EXPORT Profile : public QObject {
signals:
    void timeChanged();
public:
    explicit Profile(const double Time = 0.0);
    double time() const;
    void setTime(const double);
private:
    double Time;
}

where the implementation in profile.cpp is as follows:

Profile::Profile(const double T) : QObject() { Time = T; }

double Profile::time() const { return Time; }

void Profile::setTime(const double T) {
    if (Time != T) {
        Time = T;
        emit timeChanged();
    }
}

The Profile class is part of a library as can be seen by the LIB_EXPORT macro (I followed the guidelines on https://wiki.qt.io/How_to_create_a_library_with_Qt_and_use_it_in_an_application).

Now, I would like to be able to use a QUndoStack to allow undoing/redoing changes to the Time attribute of a Profile. I can imagine that the relevant QUndoCommand could look somewhat like this:

class TimeCommand : public QUndoCommand {
public:
    explicit TimeCommand(Profile *P, const double OldTime, const double NewTime, QUndoCommand *);
    void undo() Q_DECL_OVERRIDE;
    void redo() Q_DECL_OVERRIDE;
private:
    Profile *P;
    double OldTime, NewTime;
}

with implementation:

TimeCommand::TimeCommand(Profile *X, const double O, const double N, QUndoCommand *Parent) : QUndoCommand(Parent) {
    P = X;
    OldTime = O;
    NewTime = N;
}

void TimeCommand::undo() { P->setTime(OldTime); }

void TimeCommand::redo() { P->setTime(NewTime); }

I can imagine a working situation where I make the TimeCommand accessible from outside my library and avoid using Profile::setTime directly. My challenge is that I want to ensure that any change to the Time attribute of a Profile goes by means of a TimeCommand and I want to ensure this in my library, basically by changing the Profile::setTime method shown above. That means, I would like to somehow modify Profile::setTime to push a TimeCommand to the QUndoStack without adding or changing any public aspects to the declaration of the Profile class (and actually keep the TimCommand declaration/implementation hidden from users of my library). I am happy with having some extra private method _setTime to solve it, but I have no clue how to do it then... Any suggestions are very welcome.

SGaist

Hi,

Something is a bit strange here. You want to make TImeCommand available while at the same time hide it from the users of your library which is a bit contradictory, isn't it ?

What exactly is your goal with Profile and the undo framework ?

ModelTech

Yes, it may seem contradictory but it is not in my view... In the mean time, I have worked out a possible solution based on the idea of opaque pointers. The question for this solution is however whether it claims two times the amount of memory or not...

I came up with the following for profile.h:

class ProfilePrivate;

class LIB_EXPORT Profile : public QObject {
    Q_OBJECT
signals:
    void timeChanged();
public:
    explicit Profile(QUndoStack *, const double Time = 0.0);
    double time() const;
    void setTime(const double);
private:
    ProfilePrivate *DPointer;
}

and in profile.cpp the following:

struct ProfilePrivate {
    ProfilePrivate(Profile *Q, QUndoStack *S, const double T) : QPointer(Q), Stack(S), Time(T) { }
    Profile *QPointer;
    QUndoStack *Stack;
    double Time;
};

class TimeCommand : public QUndoCommand {
public:
    explicit TimeCommand(ProfilePrivate *, const double);
    void undo() Q_DECL_OVERRIDE;
    void redo() Q_DECL_OVERRIDE;
private:
    ProfilePrivate *Private;
    double Old, New;
};

TimeCommand::TimeCommand(ProfilePrivate *P, const double T) : QUndoCommand(), Private(P), Old(P->Time), New(T) { }

void TimeCommand::undo() {
    Private->Time = Old;
    emit Private->QPointer->timeChanged();
}

void TimeCommand::redo() {
    Private->Time = New;
    emit Private->QPointer->timeChanged();
}

Profile::Profile(QUndoStack *S, const double T) : QObject(), DPointer(new ProfilePrivate(this, S, T)) { }

double Profile::time() const { return DPointer->Time; }

void Profile::setTime(const double T) {
    if (DPointer->Time != T) {
        DPointer->Stack->push(new TimeCommand(DPointer, T));
        DPointer->Time = T;
        emit timeChanged();
    }
}

Hopefully, I didn't make any typos in copying from my actual code, but this works in the existing framework and hides what I want to hide. I have however not yet been able to fully test performing an undo action (I was able to check that TimeCommands get added to the QUndoStack when calling Profile::setTime, but I still need to extend the GUI to perform undo/redo actions). Most importantly is however the question whether this requires much more memory space than in the original situation as I need all the memory I can get for other things...

Edit: Just found out that this solution works as intended :) Only need to add few bits of code for case that Profile / ProfilePrivate no longer exists etc.

SGaist

By the way, QPointer is a Qt class name so it's really not a good idea for a variable name.

ModelTech

Thanks, will change that.

I also just found out that one thing is not working as I hoped: The emits in the TimeCommand::undo and TimeCommand::redo don't seem to do anything. Any idea how I can emit a signal of another class?

Edit: It had to do with the QPointer name ... So now all seems sorted and I believe that the memory usage is not that bad with the extra struct/class

SGaist

You don't emit signals from other classes. You can use invokeMethod or subclass the target class and add a function that will emit whatever signals is needed.

By the way, undo and redo should use setTime since it's the one that is responsible for updating your Profile class time and propagate when there's a change.

ModelTech

That was my original idea actually. However, when I would use Profile::setTime in TimeCommand::undo and TimeCommand::redo, it will create another QUndoCommand on the stack and that, I believe, should not happen here. Or am I wrong?

ModelTech

Ok. I can think of another solution (Have not actually tried this yet) as follows:

in profile.h:

class ProfilePrivate;

class LIB_EXPORT Profile : public QObject {
    Q_OBJECT
signals:
    void timeChanged();
public:
    explicit Profile(QUndoStack *, const double Time = 0.0);
    ~Profile();
    double time() const;
    void setTime(const double);
private:
    ProfilePrivate *dPointer;
}

and in profile.cpp the following class declarations:

class TimeCommand : public QUndoCommand {
public:
    explicit TimeCommand(ProfilePrivate *, const double);
    void undo() Q_DECL_OVERRIDE;
    void redo() Q_DECL_OVERRIDE;
private:
    ProfilePrivate *Private;
    double Old, New;
};

class ProfilePrivate : public QObject {
    Q_OBJECT
signals:
    void timeChanged();
public:
    explicit ProfilePrivate(QUndoStack *, const double); 
    double time() const;
    QUndoStack *stack();
    void setTime(const double);
private:
    QUndoStack *Stack;
    double Time;
};

with the following implementations (in profile.cpp):

// TimeCommand

TimeCommand::TimeCommand(ProfilePrivate *P, const double T) : QUndoCommand() {
    Private = P;
    Old = Private->time();
    New = T;
    Private->setTime(New);
}

void TimeCommand::undo() { Private->setTime(Old); }

void TimeCommand::redo() { Private->setTime(New); }

// ProfilePrivate

ProfilePrivate::ProfilePrivate(QUndoStack *S, const double T) : QObject(), Stack(S), Time(T) { }

double ProfilePrivate::time() const  { return Time; }

QUndoStack *ProfilePrivate::stack() { return Stack; }

void ProfilePrivate::setTime(const double T) {
    Time = T;
    emit timeChanged();
}

// Profile

Profile::Profile(QUndoStack *S, const double T) : QObject() {
    dPointer = new ProfilePrivate(S, T);
    connect(dPointer, SIGNAL(timeChanged()), this, SIGNAL(timeChanged()));
}

Profile::~Profile() { delete dPointer; }

double Profile::time() const { return dPointer->time(); }

void Profile::setTime(const double T) {
    if (dPointer->time() != T) {
        dPointer->stack()->push(new TimeCommand(dPointer, T));
    }
}

Some questions/remarks about this idea:

Can I avoid the Profile destructor by making the relevant Profile the parent of the corresponding ProfilePrivate?
I wonder what happens with calling TimeCommand::undo/redo if the ProfilePrivate to which it refers is deleted (via the Profile destructor). I guess this will result in a crash. So, how do I avoid a crash?
I am not happy with the extra indirection for the getter Profile::time in terms of the performance penalty. Is there a way to avoid this?
This solution will cost more memory than the previous solution as a QObject requires more memory than a normal/non-QObject struct/class. Not sure whether I like this...

SGaist

Then there might be a question better first answered: why do you want that private and hidden QUndoStack ? Nobody can use it from what you wrote until now and it's not clear what you want to achieve with it.

ModelTech

It is actually exactly what I want: I want the library to be responsible for managing the stack and not a user of this class. This has to do with how a class like Profile is going to be used in the context of the rest of my application. Let me try to explain...

First of all, my application is strongly based on the model-view concept, which is why I need the signal for Profile as Profile is one of the model classes. Secondly, there will be two different types of users of the model classes like Profile. The first type of users are IDE developers and the second type are plugin developers. The IDE developers will have access to the stack (The Profile constructor will actually not have QUndoStack as an argument but something else through which the stack is accessible in the corresponding ProfilePrivate). The plugin developers should however not be bothered with it in any way. They should have a simple and clean interface for accessing and modifying elements of the model. The IDE views should of course be updated on modifications (Note that Qt is brilliant here with its signal-slot concept in a model-view architecture) and it is this part of the application that is responsible for providing the undo/redo functionality. For plugin developers it is important to have fast getters and to keep memory usage for storing the model low since one type of plugin that they are be able to develop are analysis tools. Such analysis tools may very well be based on simulation or formal verification algorithms and the latter generally require huge amounts of memory. Plugin developers will also be able to create import, export and transformation plugins. For transformation plugins (which is more or less a model modification script), I will use the macro facilities of QUndoStack to enable undoing whatever transformation a plugin developer build up based on methods like Profile::setTime. All plugins work on given concrete instances of the model (also import plugins) in such a way that a plugin developer never needs access to the undo stack (the stack is cleared by the IDE after a load or import). Again, they should also not be bothered with it, they should only be bothered with easily creating great plugins ;)