Changed The Properties Dialog for Generator (Fill) Layers to be Non-Modal

Authored by eoinoneill on Wed, Jan 23, 12:55 AM.


Group Reviewers

This change moves toward consistency with other properties windows allowing you to move the properties window of a fill layer. This is the first step in trying to make fill layers more interactive. I will continue to work on this so that you can see the color or pattern of the fill layer change as you change its properties, for example.

The new window can be called either as modal or non modal depending on whether or not it has existing layer data passed into it. This means that creating a new fill layer uses a modal window while editing allows you to move the window to the side to compare and contrast values.

Realtime updates to the fill layer would be nice and I will work on that next. I would also like to make this change to filter layers and others to try to make the properties windows feel more interactive.

Also, I'm curious to know how people feel about moving the properties dialogs so that they spawn closer to wherever your layer manager dock is (when it is available)? I'd like to streamline some of the properties panels to make them work more fluently with a stylus.

Test Plan
  • Make a fill layer with any color settings. This window should be modal.
  • Right click on the layer and go to properties for the newly created fill layer.
  • This window should be non-modal. Cancel and Apply should work for both pattern and color.

Diff Detail

R37 Krita
Lint Skipped
Unit Tests Skipped
eoinoneill created this revision.Wed, Jan 23, 12:55 AM
Restricted Application added a reviewer: Krita. · View Herald TranscriptWed, Jan 23, 12:55 AM
Restricted Application edited projects, added Krita; removed Krita: Abyss. · View Herald Transcript
eoinoneill requested review of this revision.Wed, Jan 23, 12:55 AM
eoinoneill retitled this revision from Changed The Properties for Generator (Fill) Layers to be Non-Modal to Changed The Properties Dialog for Generator (Fill) Layers to be Non-Modal.
rempt added a comment.Wed, Jan 23, 9:20 AM

A short test didn't show up any problems, not even when painting on another layer. I think that in non-modal mode there should also be an apply and close button, not Ok and Cancel, so people can try out different things without closing the dialog.

The main thing that should be checked in such cases is the presence of deadlocks when trying to do some actions, when the dialog is open. E.g. opening this dialog again, or starting a filter, or changing the current layer or removing the current layer.

Except double redo() of the command the patch looks kind of okay. I don't feel good about making the dialog non-modal, because it allows the user to open it multiple times. But the layer properties dialog is already affected by this bug, so it will not be too big regression.

That is, the only real blocker here is double-redo, the rest is fine.


cmd->redo() is not needed. It is done by the adapter. Extra redo may break undo event.


Why removing a reference from defaultLayerName? I guess it was just a mistype :)

eoinoneill added a comment.EditedWed, Jan 23, 10:17 PM

Alright, I'll go ahead and push this up then. I've got rid of the extra undo (though that also exists in the adjustment layer code from the previous implementation, which I will work on next weekend or so.) Also, nice catch on the reference, I've added that back to the code.

As for opening multiple properties windows, I'll look into fixing that for the properties windows as I continue to work on this part of the code. I think that getting the behavior to be uniform between the dialog windows would be a nice start to coming to a solution with the design though, since the lack of uniformity makes it difficult to come to a solution that works for all of them.

As for apply/close, I think that I would instead like to make it so that changing the color or generator type (pattern/fill) simply updates the the generator layer as a preview with the change only taking affect when you hit Ok. I think all I need to do with that is create a slot that that updates the layer and either revert the changes to the previous config on cancel, or actually update the layer permanently on ok.

eoinoneill added a comment.EditedWed, Jan 23, 10:36 PM

@rempt Hey, I was just trying to merge my commit but noticed that my changes were already in the code. It looks like it happened on commit cf5346cb2233dc05e7c621544ec6a5f074111e9e , you may have accidentally merged my code when you were testing this patch. I was thinking about just fixing it since it is already there, but I think it may be cleaner if you revert that commit and remove the original test patch so that I can keep these changes in one single commit (so that cherry picking or reversion will be easier in the future.)

eoinoneill abandoned this revision.Tue, Jan 29, 11:07 PM

I've gone ahead and fixed the above. I will push this up after working on a few other minor changes to the way fill layers preview.

Thanks for taking the time for a code review.