Add back magnifier to QPainter port of rectangle selection
AbandonedPublic

Authored by abalaji on May 4 2018, 7:24 AM.

Details

Summary

Reimplement magnifier for rectangle mode R166:790cb3fb2a97: Add magnifier for rectangle mode for QPainter port

Depends on D12626

Diff Detail

Repository
R166 Spectacle
Branch
qpainter-magnifier
Lint
No Linters Available
Unit
No Unit Test Coverage
abalaji requested review of this revision.May 4 2018, 7:24 AM
abalaji created this revision.

The code still needs a little cleanup, but before that, please make sure I got the logic down

rkflx added a comment.May 4 2018, 11:21 AM

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.


The code still needs a little cleanup, but before that, please make sure I got the logic down

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.

abalaji retitled this revision from Magnifier for Rectangle mode port to Implement magnifier for Rectangle mode port.May 4 2018, 4:48 PM
abalaji edited the summary of this revision. (Show Details)
abalaji retitled this revision from Implement magnifier for Rectangle mode port to Add back magnifier to QPainter port of rectangle selection.

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

abalaji updated this revision to Diff 33633.May 4 2018, 5:06 PM
  • Fix issues
abalaji updated this revision to Diff 33635.May 4 2018, 5:14 PM

Fix diff

One nip-tick: inline should be placed in function definition not in declaration, you can make other review to correct it.

rkflx added a comment.May 4 2018, 5:48 PM

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

One nip-tick: inline should be placed in function definition not in declaration, you can make other review to correct it.

Sorry my bad, I thought it should be placed in both places, I'll remove it from the declaration

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

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

abalaji updated this revision to Diff 33647.May 4 2018, 6:34 PM

Make the rectangle border one real pixel thick, and align border to crosshair

rkflx added a comment.May 4 2018, 6:36 PM

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,

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.

So, is it best to just ditch inline from everywhere and let the compiler decide?

rkflx added a comment.May 4 2018, 6:39 PM

So, is it best to just ditch inline from everywhere and let the compiler decide?

Yup, that's what I understood from the SO link.

abalaji added a comment.EditedMay 4 2018, 6:45 PM

Rectangle is now 1 "real" pixel wide even on HiDPI. Tested on 2x scaling on 3840x2160. Red line is 2 real pixels wide.


Edit: forgot to check "Include cursor" while taking the screenshot. The cursor is dragging the bottom-right corner

rkflx added a comment.May 4 2018, 6:50 PM

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

rkflx added a comment.May 4 2018, 6:54 PM

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

rkflx added a comment.May 4 2018, 7:16 PM

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.

abalaji added a comment.EditedMay 4 2018, 7:22 PM

Oh I noticed that too, just figured it out, it is aligned, but the dark mask is one pixel off haha

abalaji updated this revision to Diff 33652.May 4 2018, 8:00 PM
  • Fix all border issues
rkflx added a comment.May 4 2018, 8:09 PM
  • Fix all border issues

Selection border looks much better now ;) More comments:

  • Is the bottom and right border of the magnifier working for you with 1x scaling? Just asking, because with QML it never worked for me, but for QPainter it should probably work?
  • The borders of the size label and the bottom help text are still fuzzy.
  • Make sure to add your fixes to the correct Diff (this one is only about the magnifier, IIRC ;)
abalaji updated this revision to Diff 33654.EditedMay 4 2018, 8:18 PM
  • Rebase
  • Fix all border issues

Selection border looks much better now ;) More comments:

  • Is the bottom and right border of the magnifier working for you with 1x scaling? Just asking, because with QML it never worked for me, but for QPainter it should probably work?
  • The borders of the size label and the bottom help text are still fuzzy.
  • Make sure to add your fixes to the correct Diff (this one is only about the magnifier, IIRC ;)
  • Rn the border rendering for the magnifier is quite naive, so I suspect that 1x scaling would break it. Please read http://doc.qt.io/qt-5/qrectf.html#rendering for more information on what's happening on line 499 and how it "works" with 2x scaling. On 2x, only half of 1 logical pixel (2 real pixels) of the border ends up showing up, the other half is overlapped by the PixmapFragment from line 500. The fix is to not use drawRect but use fillRect with x, y, height, width adjusted by 1 pixel outwards.
  • I'll fix the fuzzed borders
  • Just moved unrelated changes to back to the main diff, so that's sorted out.
abalaji updated this revision to Diff 33656.May 4 2018, 8:39 PM
  • Fix magnifier border
abalaji updated this revision to Diff 33659.May 4 2018, 8:57 PM
  • Rebase
abalaji updated this revision to Diff 33661.May 4 2018, 9:08 PM
  • Rebase

@rkflx fixed every issue you pointed out back in D12626

rkflx added a comment.May 4 2018, 9:26 PM

@rkflx fixed every issue you pointed out back in D12626

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.

@rkflx fixed every issue you pointed out back in D12626

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.

Alright gotcha

guotao added a subscriber: guotao.May 18 2018, 2:36 AM
anthonyfieroni resigned from this revision.Jul 30 2018, 4:46 AM
rkflx resigned from this revision.Aug 25 2018, 6:41 AM
abalaji updated this revision to Diff 46343.Nov 27 2018, 7:40 PM

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.

abalaji updated this revision to Diff 46345.Nov 27 2018, 7:44 PM
  • Rebase
abalaji abandoned this revision.Nov 27 2018, 9:55 PM

Now merged into D12626