Change ruler subdivision based on zoom level (Bug 388247)
ClosedPublic

Authored by mzhou on Jan 20 2018, 9:49 PM.

Details

Summary

This is a fix for Bug 388247 (https://bugs.kde.org/show_bug.cgi?id=388247)
This revision changes how the markers and numbers on the ruler (the ruler on the peripheral of the canvas, not the assistant ruler) are drawn so that the ruler changes its precision based on the zoom level of the canvas.

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.
mzhou created this revision.Jan 20 2018, 9:49 PM
Restricted Application added a subscriber: woltherav. · View Herald TranscriptJan 20 2018, 9:49 PM
mzhou requested review of this revision.Jan 20 2018, 9:49 PM
alvinhochun retitled this revision from Fix for Bug 388247 to Change ruler subdivision based on zoom level (Bug 388247).Jan 21 2018, 6:28 AM

Please refrain from stating only the bug number in the title, because it makes the title unmeaningful to anybody.

mzhou added a comment.Jan 21 2018, 6:35 AM

Please refrain from stating only the bug number in the title, because it makes the title unmeaningful to anybody.

All right, I'll make the title more descriptive next time.

mzhou updated this revision to Diff 25717.Jan 21 2018, 3:13 PM

To draw the marks when the ruler is set to be right to left.

mzhou updated this revision to Diff 25780.Jan 22 2018, 7:52 PM

...I forgot to commit my last change before doing the commit.

rempt accepted this revision.Jan 23 2018, 8:00 AM
rempt added a subscriber: rempt.

Nice! This works perfectly. Can you push it yourself or don't you've got commit access yet?

This revision is now accepted and ready to land.Jan 23 2018, 8:00 AM

I do have some very minor nitpicks. Check my inline comments.

libs/widgets/KoRuler.cpp
281

Minor nitpick: I would prefer to put the numberSeparation = minimalNumberSeparation assignment under this conditional block rather than having an empty block here.

480

Ditto. Also, this looks like to be the same code as my other inline comment at my first glance. Can they be refactored into a function to reduce duplicate code?

mzhou added a comment.Jan 23 2018, 9:05 AM
In D9999#194599, @rempt wrote:

Nice! This works perfectly. Can you push it yourself or don't you've got commit access yet?

I don't have commit access yet. I probably can't push it myself.

mzhou added a comment.Jan 23 2018, 9:14 AM

I do have some very minor nitpicks. Check my inline comments.

On the first matter, I'm doing that because I don't like declaration without initialization. I'd say it's just a matter of style: some people like it, others don't, but it doesn't really matter.

On the second matter, actually a ton of refactoring can be done to this class. If I'm going to refactor it, I'd rather do it to the whole class. Considering this class generally is working well and it's not likely to be changed frequently, I'd rather use the time on other bugs. I'll be glad to accept the task of refactoring this class when the next phase for large-scale refactoring comes.

This revision was automatically updated to reflect the committed changes.

Just a comment:

I have a feeling that there is no need to show subdivisions for half- and quarter- pixels. The point is a bit disputable, but it might be a nice topic for future fixes.

libs/widgets/KoRuler.cpp
281

I would just add a // noop comment :)

Anyway, it doesn't matter since the patch is already pushed :)