Reimplement magnifier for rectangle mode R166:790cb3fb2a97: Add magnifier for rectangle mode for QPainter port
Depends on D12626
ngraham | |
rkflx | |
anthonyfieroni |
Reimplement magnifier for rectangle mode R166:790cb3fb2a97: Add magnifier for rectangle mode for QPainter port
Depends on D12626
No Linters Available |
No Unit Test Coverage |
The code still needs a little cleanup, but before that, please make sure I got the logic down
Sorry to be pendantic, but for large projects like KDE it's important to get the commit message right:
Magnifier for Rectangle mode port
I already linked to https://chris.beams.io/posts/git-commit/#imperative multiple times, but here is how I'd word it: "Add back magnifier to QPainter port of rectangle selection"
Implement magnifier for rectangle mode
I'd also reference the commit here where it was originally implemented, i.e. 790cb3fb2a97.
Goes on top of D12626
There's a special syntax for this, so Phabricator recognizes this as a proper depedency (see https://community.kde.org/Infrastructure/Phabricator#Marking_patches_as_dependent_on_other_patches):
Depends on D12626
Please change this. You may also have to Edit Related Revisions (box on the top right) due to not specifying this during arc diff time.
Could not test in detail yet, but in general behaves okay. Visually, it's missing the border and the top part of the crosshair is off to the left a bit.
If by "logic" you meant the code, I did not look at that yet.
Oh so sorry @rkflx, I keep forgetting that the "title" is the commit message, my bad! And the top of the crosshair was correct at one point, I pushed the wrong thing out whoops, I'll fix that right away. By logic, I guess I meant more about the behaviour, but a code review would be great too :D. I'll work on the border stuff next
One nip-tick: inline should be placed in function definition not in declaration, you can make other review to correct it.
I'm skeptical about whether we should use inline at all: https://stackoverflow.com/questions/29796264/is-there-still-a-use-for-inline
Sorry my bad, I thought it should be placed in both places, I'll remove it from the declaration
Yeah, inline is not much use with modern compilers, but it can serve for documentation purposes as "a chunk of code that is a runs like a sub portion of another function" if that makes any sense, since all functions I've declared as inline are only called once, and are as good as subsections of the functions they get called in
No, it does not make sense: What you are describing is a function call which was inlined by the compiler. However, that's for the compiler to decide, as mentioned before.
since all functions I've declared as inline are only called once, and are as good as subsections of the functions they get called in
inline won't at all prevent them from being called again, making your "documentation purposes" very questionable.
If you have as simple helper function, put it in an anonymous namespace outside of the class. If you have to access member variables, use a private function.
Rectangle is now 1 "real" pixel wide even on HiDPI. Tested on 2x scaling on 3840x2160. Red line is 2 real pixels wide.
Ah, sorry. I believe there was some miscommunication: I meant for 1x scaling, the line should be aligned to the grid, and thus render as a 1px line. However, for HiDPI scaling, Qt should still scale up the line width for you, of course. The problem was merely that you positioned the line in-between two pixels, so it got rendered as two lines. For example for 2x scaling, the line should be 2px wide etc.
Ahh, gotcha, and I know why: anti-aliasing, I'll disable it and take a screenshot again
Oh no, don't disable it. You just have to make sure to align correctly on the pixel grid. If you disable antialiasing, it will look bad for fractional scaling (e.g. 1.7x).
Yeah, you're right, anti-aliasing wasn't the culprit, here's a screenshot with anti-aliasing back on for confirmation
If you look closely at the left border, you'll see the border is rendered with two different colours. If I'm not mistaken, this means that the border is still not aligned to the pixel grid, so Qt uses antialiasing to make it appear as if it was rendered at the in-between position.
I'd expect a solid (single colour) 2px line for 2x scaling, a 1px line for 1x scaling, and an antialiased in-between line for 1.5x scaling.
Oh I noticed that too, just figured it out, it is aligned, but the dark mask is one pixel off haha
Selection border looks much better now ;) More comments:
Thanks, I'll do more testing later, and in case no issues are found I'll start with the code review. However, will probably not get done this weekend, busy with other things.
FWIW, since the magnifier feature has landed in the current QML-based version, I think it would be appropriate to fold this change back into D12626 now.