##A: Why Refactor?
After a few months working with and around Krita's animation systems, @eoinoneill and I want to suggest and discuss a few potential changes.
While it's always possible to just build on top of what's already there, we both think that it will be easier and cleaner in the long
##B: Suggested Changes
- **Remove subclass-specific interface pollution from `KisKeyframe` and `KisKeyframeChannel` base classes.**
A number of non-common methods and data have been added to the `KisKeyframe` and `KisKeyframeChannel` base classes.
This adds unnecessary clutter to the base interfaces and makes it harder to use and implement the derived classes.
```
//...//
qreal KisScalarKeyframeChannel::scalarValue(const KisKeyframeSP keyframe) const
{
KisScalarKeyframe *key = dynamic_cast<KisScalarKeyframe*>(keyframe.data());
Q_ASSERT(key != 0);
return key->value;
}
void KisScalarKeyframeChannel::setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand)
{
QScopedPointer<KUndo2Command> tempCommand;
if (!parentCommand) {
tempCommand.reset(new KUndo2Command());
parentCommand = tempCommand.data();
}
qreal oldValue = scalarValue(keyframe);
KUndo2Command *cmd = new Private::SetValueCommand(this, keyframe, oldValue, value, parentCommand);
cmd->redo();
}
```
- **`KisKeyframeChannel` should have sole responsibility for mapping "times" to "keyframes" & time state should be removed from `KisKeyframe`.**
- **`KisKeyframeChannel` interface should operate on "time" indices whenever possible, and should only return `KisKeyframes` when the value of a keyframe is needed by the caller.**
```
//OLD:
- bool deleteKeyframe(KisKeyframeSP keyframe, KUndo2Command *parentCommand = 0);
- bool moveKeyframe(KisKeyframeSP keyframe, int newTime, KUndo2Command *parentCommand = 0);
//NEW:This adds unnecessary clutter to the base interfaces and makes it harder to use and implement the derived classes. For example:
```
// KisKeyframeChannel.h //
virtual bool hasScalarValue() const = 0;
virtual qreal minScalarValue() const;
virtual qreal maxScalarValue() const;
virtual qreal scalarValue(const KisKeyframeSP keyframe) const;
virtual void setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand = 0);
// KisKeyframeChannel.cpp //
qreal KisKeyframeChannel::scalarValue(const KisKeyframeSP keyframe) const
{
Q_UNUSED(keyframe);
return 0;
}
void KisKeyframeChannel::setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand)
{
Q_UNUSED(keyframe);
Q_UNUSED(value);
Q_UNUSED(parentCommand);
}
// KisScalarKeyframeChannel.cpp //
qreal KisScalarKeyframeChannel::scalarValue(const KisKeyframeSP keyframe) const
{
KisScalarKeyframe *key = dynamic_cast<KisScalarKeyframe*>(keyframe.data());
Q_ASSERT(key != 0);
return key->value;
}
void KisScalarKeyframeChannel::setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand)
{
QScopedPointer<KUndo2Command> tempCommand;
if (!parentCommand) {
tempCommand.reset(new KUndo2Command());
parentCommand = tempCommand.data();
}
qreal oldValue = scalarValue(keyframe);
KUndo2Command *cmd = new Private::SetValueCommand(this, keyframe, oldValue, value, parentCommand);
cmd->redo();
}
```
- **`KisKeyframeChannel` should have sole responsibility for mapping "times" to "keyframes" & time state should be removed from `KisKeyframe`.**
Right now, each `KisKeyframe` maintains an internal "time" state that describes its own location within the channel.
Not only is this duplication of data error prone as care has to be taken to keep things in sync, it implies a one-to-one relationship between virtual KisKeyframes and times.
It also creates the temptation to awkwardly use the KisKeyframe's internal time or circumvent the KisKeyframeChannel's interface:
```
bool KisKeyframeChannel::moveKeyframe(KisKeyframeSP keyframe, int newTime, KUndo2Command *parentCommand)
{
LAZY_INITIALIZE_PARENT_COMMAND(parentCommand);
if (newTime == keyframe->time()) return false;
KisKeyframeSP other = keyframeAt(newTime);
if (other) {
deleteKeyframeImpl(other, parentCommand, false);
}
const int srcTime = keyframe->time();
KUndo2Command *cmd = new KisMoveFrameCommand(this, keyframe, srcTime, newTime, parentCommand);
cmd->redo();
if (srcTime == 0) {
addKeyframe(srcTime, parentCommand);
}
return true;
}
```
Because the `KisKeyframeChannel` is effectively a container of `KisKeyframes` is shouldn't be necessary for the channel to ask a given keyframe for its internal memory of where it is in time.
- **`KisKeyframeChannel` interface should operate on "time" indices whenever possible, and should only return `KisKeyframes` when the value of a keyframe is needed by the caller.**
Related to the last point, this `KisKeyframeChannel` has quite a few methods which take in references to Keyframes (that it already owns) even though the operations only require access to time.
Here's a small example:
```
// Current:
- bool deleteKeyframe(KisKeyframeSP keyframe, KUndo2Command *parentCommand = 0);
- bool moveKeyframe(KisKeyframeSP keyframe, int newTime, KUndo2Command *parentCommand = 0);
// Improved:
+ bool deleteKeyframe(int time, KUndo2Command *parentCommand = 0);
+ bool moveKeyframe(int sourceTime, int targetTime, KUndo2Command *parentCommand = 0);
```
- **Enforce separation between time, virtual keyframes & physical keyframes.
**
The `KisPaintDevice` is responsible for operations of "physical keyframes" using integer `keyframeID` handles, stored within a `KisRasterKeyframe`.
The `KisKeyframeChannel` is responsible for mapping units of time (frames numbers) to "virtual" KisKeyframes.
- **`KisRasterKeyframe` RAII**
A keyframeID is generated by the `KisPaintDevice` upon construction of a `KisRasterKeyframeSP` and freed/released upon destruction.