##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:
+ 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.