Iss575: Copy, move, reverse or delete range of frames

[davidlamhauge]

Very flexible tool, that will save a ton of work.
If not used carefully, it can damage originals. Warnings are issued at the OK-button, in such cases.

[J5lx]

So it took a few more days than I’d originally hoped when you asked me earlier this week but here we go (finally). As before, there’s a lot of cosmetic or minor stuff and also a lot of stuff that I found more than once, thus the high number of comments.

One additional thing that I’ve noticed is that the layout of the dialog is kind of inconsistent (in regard to margins, sizes of the widgets etc.). It’d be cool if you could go over that and make it consistent. From a quick glance, it seems that one cause is the use of different layouts ([HV]Box vs Grid).

Lastly, I feel like I should point out that even with the remaining issues this is already in a state where I feel that, as far as I can judge, it has earned its place in the program. The only thing that’s left now is only the final touch.

[davidlamhauge]

Ok @J5lx and @candyface . I have made the requested changes - I think.
Moving the four functions copyFrames, moveFrames, reverseFrames and deleteFrames to layer.cpp made it a little difficult to keep track of the changes I made, but I think I got them all.
Thanks for the time you spent on reviewing, and if there are further issues, I'm ready to dive in...

[davidlamhauge]

There are some of your change requests that I haven't done @J5lx . It is the member variables in the CopyMultipleKeyFramesDialog.cpp at setStartEnd(int method).
Here is part of the switch:

case 0:
mManiStartAt = mCopyStart;
mManiEndAt = mManiStartAt - 1 + (mLastFrame + 1 - mFirstFrame) * mNumLoops;
    if (ui->sBoxLastFrame->value() >= ui->sBoxStartFrame->value())
        mLabWarning = tr("Originals may be overwritten!");
    else
        mLabWarning.clear();
    break;

So I use mCopyStart - but I could have used ui->sBoxStartFrame->value(), like I do in line 4.
Can we leave as is, or should I replace the member variables with the spinBox values?

[MrStevns]

LGTM 👍

[J5lx]

Can we leave as is, or should I replace the member variables with the spinBox values?

A thought a little about this and actually I don’t think it’s a big difference whether we use one or the other. Personally, I tend slightly towards using the spinbox values directly everywhere, but in the end what really matters is that it’s consistent. Also, as mentioned in chat, there are a few things you seem to have missed (see unresolved conversations above).

[davidlamhauge]

The last requests have been fixed.
I can see that I've forgot to delete a couple of uncommented lines (Q_UNUSED). I'll do that when I know if there are any other changes I've forgot.

[J5lx]

We’re getting there! 👍 See also davidlamhauge#4. I also noticed you completely removed those kf == nullptr lines while @candyface suggested Q_CHECK_PTR, is that intentional?

[davidlamhauge]

About the nullptr, CandyFace wrote that they could also be removed. I chose that.
I hope this settles it, but I want it to be right, so I'm ready for more comments :-)

[J5lx]

Alright, I just wanted to make sure it was a conscious decision and not an oversight. So as far as I am concerned all that’s missing now is for davidlamhauge#4 to get merged (it’s a PR on your own repo, so feel free to merge as you see fit).

[davidlamhauge]

Oh yeah... I thought I had merged it, but I had only approved it, so now there were merge conflicts. I sorted it out.
Thanks again. I have to look into that Doxygen. I like to write comments, because it helps me when I return to code long time later, but if it can be part of the documentation as well, it's even better.

[MrStevns]

Reminder to self: should not be merged before #953

Some of this functionality will also be available via #1343.

[J5lx]

Most of this functionality has since been added in #1343, so I’m closing this PR as we discussed on Discord.