I don't know the code to say anything qualified, I just added a few random thoughts glancing over the change. I do think it's an improvement over the old state.

Note that there is different, but similar commit ready for the 3.1 branch.

INLINE COMMENTS

> kis_image.cc:285
> + rhs.colorSpace(),
> + undoStore ? undoStore : new KisDumbUndoStore()))
> +{

Is the thing really called Dumb? I guess it should be Dummy, assuming that it is an undo store that doesn't really allow undoing anything. Unrelated to this patch though, so discard this comment.

Yes -- it's really called that. Dummy probably would be a better name, since it's indeed an undo store that doesn't save commands.

> kis_node_facade.h:140
> struct Private;
> - Private * const m_d;
> + QScopedPointer<Private> m_d;
> };

Nice :) Using scoped pointer makes a lot of sense here. To make review easier, this could be a separate patch since it's not directly related to the rest of the change.

We probably should review all our d-pointers at one point and make them into QScopedPointers. We don't _need_ d-pointers -- we're not providing a stable abi, but it saves some rebuild time when adding member variables. That's why it was initially introduced in 2014 or so...

> KisDocument.cpp:439
> class SafeSavingLocker;
> };

I guess this forward declaration can also be removed?

Yes.

> kis_brush_export.cpp:87
> + buf.close();
> + }
>

Is this block disabled intentionally? I don't know what it did before, so I cannot judge it. Maybe it should have a FIXME: reenable? Of if it's dead code it could be removed.

There is a comment in the KisBrushExport::convert class about this block of code.

> psd_resource_block.h:78
>
> + KisAnnotation* clone() const Q_DECL_OVERRIDE {
> + // HACK ALERT: we are evil! use notmal copying instead!

I'd prefer just c++ 11 override, unless you want to support c++ 98 compilers.o

Okay.

In D3258#60774, @rempt wrote:

Note that there is different, but similar commit ready for the 3.1 branch.

INLINE COMMENTS

> kis_image.cc:285
> + rhs.colorSpace(),
> + undoStore ? undoStore : new KisDumbUndoStore()))
> +{

Is the thing really called Dumb? I guess it should be Dummy, assuming that it is an undo store that doesn't really allow undoing anything. Unrelated to this patch though, so discard this comment.

Yes -- it's really called that. Dummy probably would be a better name, since it's indeed an undo store that doesn't save commands.

The reason why it was not called Dummy is that there are two variants of "dummy" undo adapter: KisDumbUndoStore and KisSurrogateUndoStore. And the latter one is much more "dummy" is a sense that is used in programming world (mostly in unittesting). That is it does its job, can undo/redo, but has no dependencies on KisDocument and stuff. Therefore I explicitly decided not to use that well-known name "dummy" to not mess up things even more. If you have better ideas for both the classes, I'm ok with renaming them.