List of changes that were done to the Assistants code
ClosedPublic

Authored by scottpetrovic on Nov 30 2017, 3:47 PM.

Details

Reviewers
dkazakov
woltherav
Group Reviewers
Krita
Summary

I originally started wanting to add a feature but it quickly started getting into refactoring mode with other issues that I saw. This is a high level list of what was changed

  1. Renamed references of "rulerassistant tool" to just "assistant tool"
  2. added ability to change opacity and color of all assistants
  3. moved the painting logic to all be under the decoration. This makes it easier to organize when UI elements are displayed
  4. assistant control widget is now displayed on top of the assistants
  5. Changed the default assistant color
  6. Made the assistant control widget active and delete buttons larger so they are easier to click.
  7. The previews do not show when in the assistant tool...only when painting
  8. fixed a bit of code with how the widgets mapped to the canvas (eg. vanishing point X when zooming)
  9. Some spline tool fixes with how it is displayed and things were shown when they should have been hidden)
  10. A lot of refactoring with moving code from the assistant tool to the decoration

There could be more refactoring...but I thought this might be a good place to stop for now.

You can apply the diff...but it might be easier to just check out my branch..

petrovic/assistants-opacity

Test Plan
  1. Add different assistants to the canvas
  2. change the color
  3. change the opacity
  4. toggle between the painting tool and the assistant tool
  5. turn on and off the preview
  6. turn the assistants on/off
  7. delete the assistants
  8. load/save assistants (even though I didn't touch this)

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
scottpetrovic requested review of this revision.Nov 30 2017, 3:47 PM
scottpetrovic created this revision.
scottpetrovic edited the summary of this revision. (Show Details)

I am having trouble applying this patch. Could you be so kind to merge master to your branch?

scottpetrovic updated this revision to Diff 23597.EditedDec 7 2017, 2:01 AM

thanks for looking at this @woltherav

I pulled the latest from master into my branch. Doing a clean build from the branch and it seems to work for me.

The branch is petrovic/assistants-opacity

That might be the easiest way to test this out since there are so many changes

woltherav added a comment.EditedDec 7 2017, 2:49 PM

Code wise: everything looks okay.

Use wise:

  1. Please make te default color gray or something, there's something to be said for sensible defaults, and cyan is not it.
  2. When drawing a concentric ellipse or a spline, there's no preview line for the first two dots, while virtually all other assistants do. Please fix this.
  3. Color and opacity are not saved, is this intentional?

Last note:

When you merge this, add "CCMAIL:kimageshop@kde.org" to the end, and ensure that the commit specifies that clean install is necessary. Otherwise there will be a crash when using the assistant tool due to old code lying around.

woltherav accepted this revision.Dec 7 2017, 2:53 PM
woltherav added a project: Krita.
This revision is now accepted and ready to land.Dec 7 2017, 2:53 PM
scottpetrovic closed this revision.Dec 7 2017, 7:08 PM

fixed those two points and pushed out. Thanks for the review @woltherav