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:

  1. 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.
  2. 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...
  3. 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
  1. Create a document
  2. add file layer
  3. select file layer
  4. press properties
  5. change stuff, press ok.
  6. observe results.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
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
dkazakov accepted this revision as: dkazakov.Oct 23 2017, 10:42 AM
dkazakov added a subscriber: dkazakov.

Hi, @woltherav!

  1. Here is the patch to fix the updates:

P117 Masterwork From Distant Lands
1diff --git a/libs/ui/kis_file_layer.cpp b/libs/ui/kis_file_layer.cpp
2index 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

  1. The flayer variable is used correctly
  1. 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
woltherav added a comment.Oct 23 2017, 3:40 PM

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.

woltherav updated this revision to Diff 21192.Oct 23 2017, 5:23 PM

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
dkazakov accepted this revision.Oct 30 2017, 11:41 AM

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
1diff --git a/libs/ui/kis_change_file_layer_command.h b/libs/ui/kis_change_file_layer_command.h
2index 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;
24diff --git a/libs/ui/kis_layer_manager.cc b/libs/ui/kis_layer_manager.cc
25index 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

Diff 20947

View Options

libs/ui/dialogs/kis_dlg_file_layer.h

Loading...
View Options

libs/ui/dialogs/kis_dlg_file_layer.cpp

Loading...
View Options

libs/ui/kis_file_layer.h

Loading...
View Options

libs/ui/kis_file_layer.cpp

Loading...
View Options

libs/ui/kis_layer_manager.cc

Loading...