Differential D8359

Add properties dialog to change file layer properties.
ClosedPublic
Actions

Authored by woltherav on Oct 18 2017, 2:38 PM.

Details

Reviewers

dkazakov

Group Reviewers

Krita

Commits

R37:1cfb64ab4bc2: [FEATURE] Add properties dialog to change file layer properties.

Summary

This allows us to change file layer properties by resummoning the creation dialog.

The patch works in principle, how ever there are two issues:

The main one is that I don't know how to update the whole canvas afterwards, an important change when having different sized files. EDIT: I figured it out, there needs to be m_view->document()->image()->refreshGraph(); after changing the file layer settings.
There's no undo command for this, I am not sure how relevant it is to make an undo command for this, considering it is only about pointing to a new file...
I need someone to check if the creation of "*flayer" makes sense, as the others are sharedpointers.

Other than that, please do a proper code review, because I have no idea what I am doing with nodes :D

Edit: This would fix https://bugs.kde.org/show_bug.cgi?id=380467, which would be the last of the file layer bugs, if I am seeing that correctly :D

Test Plan

Create a document
add file layer
select file layer
press properties
change stuff, press ok.
observe results.

Diff Detail

Repository

R37 Krita

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

woltherav created this revision.Oct 18 2017, 2:38 PM

woltherav edited the summary of this revision. (Show Details)Oct 18 2017, 3:52 PM

woltherav edited the summary of this revision. (Show Details)Oct 19 2017, 2:51 PM

Hi, @woltherav!

Here is the patch to fix the updates:

P117 Masterwork From Distant Lands

1	diff --git a/libs/ui/kis_file_layer.cpp b/libs/ui/kis_file_layer.cpp
2	index faef31c..716374c 100644
3	--- a/libs/ui/kis_file_layer.cpp
4	+++ b/libs/ui/kis_file_layer.cpp
5	@@ -178,6 +178,8 @@ void KisFileLayer::slotLoadingFinished(KisPaintDeviceSP projection, int xRes, in
6	qint32 oldX = x();
7	qint32 oldY = y();
8
9	+ const QRect oldLayerExtent = m_paintDevice->extent();
10	+
11	m_paintDevice->makeCloneFrom(projection, projection->extent());
12	m_paintDevice->setDefaultBounds(new KisDefaultBounds(image()));
13
14	@@ -204,7 +206,7 @@ void KisFileLayer::slotLoadingFinished(KisPaintDeviceSP projection, int xRes, in
15	m_paintDevice->setX(oldX);
16	m_paintDevice->setY(oldY);
17
18	- setDirty();
19	+ setDirty(m_paintDevice->extent() \| oldLayerExtent);
20	}
21
22	KisNodeSP KisFileLayer::clone() const

The flayer variable is used correctly

Undo... Ideally, there should be an undo command, because both adjustment and generator layers' properties are undoable. Just to be consistent from the UIX point of view.

Please decide yourself whether you want to implement the undo command or not. If not, just push the patch with my update patch.

libs/ui/kis_layer_manager.cc
249	It is both fine to use either raw or shared pointers here. There is a `layer` shared pointer, which guarantees the object will never be deleted. If you passed `flayer` anywhere is a function, it would be a good idea to explicitly convert it into a SP, but you don't pass it anywhere, so it is fine.

This revision is now accepted and ready to land.Oct 23 2017, 10:42 AM

I tried making an undo command, but the problem is that I cannot figure out how to get the undo to act on the file layer, when the file layer definition is in ui and the command in image/commands.

I put my diff here. https://phabricator.kde.org/P120
I am not smart enough for the undo system, I am afraid.

Added dmitry's patch and added a undo command. This changes quite a bit, so please check carefully that I didn't do weird things.

woltherav requested review of this revision.Oct 29 2017, 11:06 PM

Hi, @woltherav!

I have tested your patch and polished out s few bugs.

Summary:

you shouldn't manually call command->redo(), it is done by QUndoStack automatically, when the command is added to it.
setModified() is also ustomatically tracked by the undo stack
setDirty() in the command is not necessary, because it is issued by setFileName() method of the file layer.

Please apply the following patch and push! :)

P126 Masterwork From Distant Lands

1	diff --git a/libs/ui/kis_change_file_layer_command.h b/libs/ui/kis_change_file_layer_command.h
2	index ab13530..88ae8b9 100644
3	--- a/libs/ui/kis_change_file_layer_command.h
4	+++ b/libs/ui/kis_change_file_layer_command.h
5	@@ -44,14 +44,16 @@ public:
6	public:
7	void redo() override {
8	m_node->setScalingMethod(m_newMethod);
9	+
10	+ // setFileName() automatically issues a setDirty call
11	m_node->setFileName(m_newPath, m_newFileName);
12	- m_node->setDirty();
13	}
14
15	void undo() override {
16	m_node->setScalingMethod(m_oldMethod);
17	+
18	+ // setFileName() automatically issues a setDirty call
19	m_node->setFileName(m_oldPath, m_oldFileName);
20	- m_node->setDirty();
21	}
22	private:
23	KisFileLayerSP m_node;
24	diff --git a/libs/ui/kis_layer_manager.cc b/libs/ui/kis_layer_manager.cc
25	index 721cb4a..3222d4b 100644
26	--- a/libs/ui/kis_layer_manager.cc
27	+++ b/libs/ui/kis_layer_manager.cc
28	@@ -340,30 +340,27 @@ void KisLayerManager::layerProperties()
29	dlg.setScalingMethod(scalingMethodOld);
30
31	if (dlg.exec() == QDialog::Accepted) {
32	- flayer->setName(dlg.layerName());
33	- KisFileLayer::ScalingMethod scalingMethod = dlg.scaleToImageResolution();
34	- flayer->setScalingMethod(scalingMethod);
35	- QString fileName = dlg.fileName();
36	+ const QString fileNameNew = dlg.fileName();
37	+ KisFileLayer::ScalingMethod scalingMethodNew = dlg.scaleToImageResolution();
38
39	- if(fileName.isEmpty()){
40	+ if(fileNameNew.isEmpty()){
41	QMessageBox::critical(m_view->mainWindow(), i18nc("@title:window", "Krita"), i18n("No file name specified"));
42	return;
43	}
44	- flayer->setFileName(basePath, fileName);
45
46	- if (fileNameOld!= fileName \|\| scalingMethodOld != scalingMethod) {
47	+ flayer->setName(dlg.layerName());
48	+
49	+ if (fileNameOld != fileNameNew \|\| scalingMethodOld != scalingMethodNew) {
50	KisChangeFileLayerCmd *cmd
51	= new KisChangeFileLayerCmd(flayer,
52	basePath,
53	fileNameOld,
54	scalingMethodOld,
55	basePath,
56	- fileName,
57	- scalingMethod);
58	- // FIXME: check whether is needed
59	- cmd->redo();
60	+ fileNameNew,
61	+ scalingMethodNew);
62	+
63	m_view->undoAdapter()->addCommand(cmd);
64	- m_view->document()->setModified(true);
65	}
66	}
67	} else { // If layer == normal painting layer, vector layer, or group layer

This revision is now accepted and ready to land.Oct 30 2017, 11:41 AM

Closed by commit R37:1cfb64ab4bc2: [FEATURE] Add properties dialog to change file layer properties. (authored by woltherav). · Explain WhyOct 30 2017, 3:03 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		libs/image/kis_types.h (4 lines)
M		libs/ui/CMakeLists.txt (1 line)
M		libs/ui/dialogs/kis_dlg_file_layer.h (3 lines)
M		libs/ui/dialogs/kis_dlg_file_layer.cpp (19 lines)
A	M	libs/ui/kis_change_file_layer_command.h (66 lines)
M		libs/ui/kis_file_layer.h (1 line)
M		libs/ui/kis_file_layer.cpp (8 lines)
M		libs/ui/kis_layer_manager.cc (33 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20947			Oct 18 2017, 2:33 PM	★	★
Diff 2	21192			Oct 23 2017, 5:23 PM	★	★
Diff 3	21574	bb3aaec	R37:1cfb64ab4bc2e1af2eeedda2c625a19e15147cb8	Oct 30 2017, 1:47 PM	★	★