Refactor: Animation Timeline Code Reuse and Menu Simplification
ClosedPublic

Authored by emmetoneill on May 19 2018, 8:16 AM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Krita: Abyss
Summary

I've done a relatively significant refactoring of some code related to the animation Timeline Docker with the goal of minimizing code duplication, generalizing functions, and slightly simplifying the right-click context menu options.

I've created a enum called TimelineDirection that represents the concept of LEFT and RIGHT (with optional aliases BEFORE and AFTER). The purpose of this is to pass direction as a parameter to more general functions instead of relying on xLeft and xRight functions with similar code. Some parameter-less xLeft and xRight slots still exist (for the sake of easily hooking up to action trigger signals), but in most cases these functions are simply forwarding a one-line call to one of the more general-use functions.

TimelineDirection parameters also allow us to simplify the right-click context menus. While things like "Insert Frame Left" and "Insert Column Right" still exist, a new "Side" option in the InsertKeyFrameDialog when inserting multiple frames/columns has made the "N Keyframes Left" and "N Keyframes Right" menu actions redundant. As such, the "N" options have been combined into "Insert Multiple Keyframes", and "Insert Multiple Columns" respectively, and the user can set Left or Right side along with Count and Timing within the dialog menu.

Known Bug: For some reason the context menus aren't drawing properly for me. The menu items work as intended, but aren't visible. I've looked through the code a few times and I can't quite determine what the cause of this error is. Any idea what might cause this?


Question: Dmitry, how do you feel about single-line function member function calls being in the header file? For example, in the header:

void slotInsertKeyframeLeft() {insertKeyframes(1, 1, TimelineDirection::LEFT, false);}

Personally I think it's quite clean, but I can also understand if it's a policy to keep all function definitions in the source files even if they're just single-liners. Let me know if I should change them!


Anyway, if this refactor is unhelpful/unwanted/too drastic, then I understand and no hard feelings or anything!

Test Plan

Please take a look at the code changes to see if they make sense and feel like an improvement. There should be a bit less duplicate code, and less need for Left/Right alternate functions.

Some of the menus are visible right now, but the actions all seem to work as intended. Please try the actions out, and test that the functionality of the "Side" radio buttons in the Insert Keyframes dialog box are working as you would expect.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
emmetoneill requested review of this revision.May 19 2018, 8:16 AM
emmetoneill created this revision.
emmetoneill edited the summary of this revision. (Show Details)May 19 2018, 8:26 AM

Added code-reuse refactor for hold frames and hold columns.

emmetoneill edited the summary of this revision. (Show Details)May 20 2018, 6:58 AM
dkazakov requested changes to this revision.May 23 2018, 8:51 PM

Hi, @emmetoneill!

Some menus are absent because the action ids that are passed to the menu do not coincide with the ones defined in ./krita/krita.action.

E.g. "insert_keyframes_left" vs "insert_keyframe_left"

KisActionManager::safePopulateMenu(frames, "insert_keyframe_left", m_d->actionMan);

<Action name="insert_keyframes_left">
      <icon></icon>
      <text>Insert Keyframe Left</text>
      <whatsThis></whatsThis>
      <toolTip>Insert keyframes to the left of selection moving the tail of animation to the right</toolTip>
      <iconText></iconText>
      <activationFlags>100000</activationFlags>
      <activationConditions>0</activationConditions>
      <shortcut></shortcut>
      <isCheckable>false</isCheckable>
      <statusTip></statusTip>
    </Action>

Personally I would prefer if the actions that add exactly one frame and cannot be used for modifying multiple frames would be in singular form. I guess initially they used this form, but it broke at some point... You can use any form you like.

Question: Dmitry, how do you feel about single-line function member function calls being in the header file? For example, in the header:

void slotInsertKeyframeLeft() {insertKeyframes(1, 1, TimelineDirection::LEFT, false);}

Theoretically, KDE coding policy forbids such definitions, but sometimes we break this rule for simple boilerplate things like KisCoordinatesConverter::widgetToViewport() and friends. I guess for this particular case it would not harm :)

So basically, the patch has only one blocker:

  1. Action ids do not coincide with the actions defined in the .action file, so names are not visible. Please also make sure that leftward actions are still available in the .action file and are created somewhere in TimelineFramesView class, that way they will still be accessible with shortcuts.
This revision now requires changes to proceed.May 23 2018, 8:51 PM

Here's an update patch that fixes the menus. I popped a few more of the one-liners that call more general member functions into the header. I also fixed a tiny bug with the separators in the insert frame/hold columns menus and cleaned up some unused headers.

Also, I opted to use the word "multiple" instead of "n" for the menu item labels where appropriate. My thinking was that, (a) because the words "right" and "left" were removed from quite a few of the menu items there was extra room, and (b) using "n" sounds kind of "mathy" or programmer-ish. I just preferred the look of "Insert Multiple Keyframes" instead of "Insert N Keyframes". At any rate, I don't want to change any user-facing stuff without consulting you first, so let me know if you'd prefer it changed back and I'll be happy to do it either way. =]

Please also make sure that leftward actions are still available in the .action file and are created somewhere in TimelineFramesView class, that way they will still be accessible with shortcuts.

I may have to do another revision of this patch, because I'm pretty sure I didn't properly address this point. I did keep things like "insert_keyframe_left" and "insert_keyframe_right", as well as "insert_multiple_keyframes". But did you want me to keep things like "insert_multiple_keyframes_left" and "insert_multiple_keyframes_right" also? If so, how should these behave - should they pop up insert keyframes dialog? or should they act instantly based on the user's previous settings?

Let me know!

Thanks again, Dmitry!

dkazakov requested changes to this revision.May 24 2018, 11:17 AM

Hi, @emmetoneill!

I have tested your patch, it works and looks mostly fine. I have managed to find one small regression though:

  1. Select three consecutive frames
  2. Press "Insert keyframe right"

Expected (old) behavior:

  1. Three new frames are added at the right of the current selection

Regression in the patch:

  1. One new frame is added

The same problem happens with "Insert frame left" and their columns counterparts. I guess this small regression is the last thing that blocks the patch from being pushed :)

But did you want me to keep things like "insert_multiple_keyframes_left" and "insert_multiple_keyframes_right" also?

I guess I just didn't understand something in the patch, because the labels were hidden :) The patch in a current state looks very good, all the necessary shortcuts seems to be present in the list.

plugins/dockers/animation/timeline_frames_view.cpp
1314

I would prefer to use a ternary 'if' operator in this case. That would let you use 'const' restriction on a variable, which helps in understanding the code later.

const int insertionColumn = 
    direction == TimelineDirection::RIGHT ? 
    maxColumn + 1 : 
    minColumn

But I know that the use of ternary operator is much of a matter of taste. You can use whatever version you like yourself :)

This revision now requires changes to proceed.May 24 2018, 11:17 AM
emmetoneill added a comment.EditedMay 25 2018, 12:25 AM

I have tested your patch, it works and looks mostly fine. I have managed to find one small regression though:

  1. Select three consecutive frames
  2. Press "Insert keyframe right"

    Expected (old) behavior:
  3. Three new frames are added at the right of the current selection

    Regression in the patch:
  4. One new frame is added

    The same problem happens with "Insert frame left" and their columns counterparts. I guess this small regression is the last thing that blocks the patch from being pushed :)

From my testing just now, this appears to be the case in the current master branch also, so I don't think it's a regression in my patch. Nevertheless, I'd be willing to take a crack at fixing it if you want me to.

(But, is that really the expected behavior, though? Frankly, I'm not sure that adding three new frames (one per selected frame) would be the behavior that I'd expect from that particular sequence of inputs.)

edit: I also want to mention another strange, bug-like behavior that I have come across in testing. Selecting "Remove Frame/Column and Pull" seems to remove/pull the next frame instead of the current one. This also is not a regression in this patch, as it's also currently in master. Is that the intended behavior?

I guess I just didn't understand something in the patch, because the labels were hidden :) The patch in a current state looks very good, all the necessary shortcuts seems to be present in the list.

Oh ok! Just wanted to make sure I wasn't skipping over anything. =]

I would prefer to use a ternary 'if' operator in this case. That would let you use 'const' restriction on a variable, which helps in understanding the code later.

Interesting tip! I've gone ahead and made that change.

From my testing just now, this appears to be the case in the current master branch also, so I don't think it's a regression in my patch. Nevertheless, I'd be willing to take a crack at fixing it if you want me to.

I was testing against 29ad1486d81d23a0cfcfec4a893c419d45174af5, which was before your first patch, and it works. I guess it has been broken somewhere in between.

(But, is that really the expected behavior, though? Frankly, I'm not sure that adding three new frames (one per selected frame) would be the behavior that I'd expect from that particular sequence of inputs.)

In our timeline we are trying to resemble the behavior of spreadsheet applications, so that people could use already learned mechanics :)

edit: I also want to mention another strange, bug-like behavior that I have come across in testing. Selecting "Remove Frame/Column and Pull" seems to remove/pull the next frame instead of the current one. This also is not a regression in this patch, as it's also currently in master. Is that the intended behavior?

No, it should remove the current frame. It works as expected in 29ad1486d81d23a0cfcfec4a893c419d45174af5. So it looks like it is one more regression :)

emmetoneill marked an inline comment as done.May 27 2018, 6:28 PM

Alright! I'm looking into it right now.

Resolved the regression caused by my previous patch 7c639729115b3a3bb20e1b0a1046c1bfbb6f41fe, which caused only one frame to be added by the "insert frames/columns left/right" actions regardless of number of selected frames. These actions now correctly insert new blank keyframes on the correct side of the cursor based on the number of currently selected frame slots.

The cause of this regression was that I changed the default parameter for count in the insertKeyframes function from -1 to 1 without realizing that a count value of -1 was as a way to tell the function to use the current selection to determine the number of frames to add. I've now added a couple of comments to make that intention more immediately clear.


I wasn't able to fix the other buggy/unexpected behavior with the "Remove Frames and Pull" action just yet, but I was able to test more and further confirm it.

In essence, "Remove Frames and Pull" seems to work inconsistently when the frame after the currently selected frame is null/empty/hold.

You can test this by creating two adjacent frames. Draw an "A" on the first one and a "B" on the second. If you select the first (A) and try "Remove Frames and Pull" you'll see the expected behavior - the frame with the "A" will be deleted and the frame with the "B" will shift back to take its place. However, if you select the second frame (B) and try "Remove Frames and Pull" you'll see that neither frame is deleted, although the subsequent frames will still be shifted back.

This is probably a small error with indices or early-returning in either KisTimeBasedItemModel::removeFramesAndOffset or KisTimeBasedItemModel::createOffsetFramesCommand, but I'll need a bit more time to look at it. This one actually wasn't a regression in my prior patch, as it existed in the patch just before mine 6509644e3cb89610fce1f2925b0126c9bc1439c0. Not that it matters, I'll keep looking into it, of course. =]

Also, I can add this one to the bug tracker if needed.

dkazakov accepted this revision.May 29 2018, 9:16 AM

Hi, @emmetoneill!

The patch looks and works fine! Please push!

About the Remove and Pull bug, yes, please either add a bug report about it or just make a patch :)

This revision is now accepted and ready to land.May 29 2018, 9:16 AM
Restricted Application edited projects, added Krita; removed Krita: Abyss. · View Herald TranscriptMay 29 2018, 8:49 PM