Differential D11532

Refactor animation actions a bit
ClosedPublic
Actions

Authored by scottpetrovic on Mar 20 2018, 10:08 PM.

Details

Reviewers

dkazakov

Group Reviewers

Krita

Summary

This patch accomplishes a couple things:

The actions on the timeline header can now be assigned to shortcuts.
Refactored the action docker actions to use the same createAction pattern that other actions are using (removed a todo)
The KisAction class can now change its display text. This is needed for a timeline header action where the text changes depending on how many frames you have selected.

We are currently calling a few of these action names "columns", but I wonder if we should change the name to "frames" or something more consistent with the rest of our terminology?

Test Plan

Checked to make sure the new actions were available in the shortcut manager. Assigned and tested the "move n columns" actions to make sure they work.

Diff Detail

Repository

R37 Krita

Lint

Lint Skipped

Unit

Unit Tests Skipped

scottpetrovic requested review of this revision.Mar 20 2018, 10:08 PM

scottpetrovic created this revision.

rempt added a subscriber: rempt.Mar 22 2018, 3:13 PM

rempt added inline comments.

libs/ui/kis_action.h
106 ↗	(On Diff #30055)	This is, actually, not necessary, since KIsAction inherits QWidgetAction which inherits QAction, so setText is already available. It's even already used: QAction Window::createAction(const QString &id, const QString &text, const QString &menuLocation) { KisAction action = d->window->viewManager()->actionManager()->createAction(id); action->setText(text); action->setObjectName(id); For instance.
plugins/dockers/animation/animation_docker.h
42	Spurious whitespace change?

Just an FYI, whenever we use 'columns' in the timeline gui, it means that it will affect all visible layers, not just a single layer, so let's not change column to frame.

I removed the setText function in KisAction. For some reason when I was originally working on this my compiler complained that I needed this. Removing it now though seems ok.

Any verbiage changes we can maybe wait until later.

Hi, @scottpetrovic!

Could you tell what exactly this patch solves? Right now I see two changes in this patch:

It moves creation of the actions into a separate method. At the same time, the the actions are still created manually, not using the action manager. It means that the TODO is still unfixed, so you'd better move it to the new place as well :)
It also implements new "Insert %1 left" feature. It is nice, but it is not what the description of the patch says :)

@dkazakov - currently the the actions in the timeline ruler header cannot be assigned to shortcuts (right clicking on timeline header). This patch makes those actions available to be assigned to a shortcut. If you look here "TimelineRulerHeader::setActionManager", you can see the action manager is creating those actions now. Previous to this commit, the TimelineRulerHeader had no concept of an action manager.

I didn't add any new features, so I don't understand what you mean by "also implements new "Insert %1 left" feature".

The refactor just involved moving all the action creation logic to one area. All the actions are in the "actions" XML file, so I am not sure what "create the actions using the action manager" means. I thought the action manager created the actions based off the XML file. There is probably something I am not understanding.

With my patch though, I can assign the existing actions to shortcuts now...so that was the ultimate purpose

It looks like I got crazy and didn't recognize the feature I've written several years ago :)

Please push!

This revision is now accepted and ready to land.Apr 9 2018, 6:20 PM

scottpetrovic mentioned this in R37:f51c2d9967e8: D11532 Add actions from timeline header so they can be assigned to shortcuts.Apr 10 2018, 1:36 AM

pushed out. closing

Revision Contents
Changeset List

			Path	Packages
M			krita/krita.action (51 lines)
M			krita/kritamenu.action (3 lines)
M			plugins/dockers/animation/animation_docker.h (4 lines)
M			plugins/dockers/animation/animation_docker.cpp (273 lines)
M			plugins/dockers/animation/timeline_docker.cpp (2 lines)
M			plugins/dockers/animation/timeline_frames_view.h (4 lines)
M			plugins/dockers/animation/timeline_frames_view.cpp (14 lines)
M			plugins/dockers/animation/timeline_ruler_header.h (4 lines)
M			plugins/dockers/animation/timeline_ruler_header.cpp (46 lines)