Clean up content rect in TerminalDisplay
Needs ReviewPublic

Authored by sandsmark on Dec 22 2018, 8:12 PM.


Group Reviewers

Instead of having to make sure we check both content rects
everywhere, we can just use the normal QWidget content
rect. Also some tiny cleanup where I was touching anyways.

Test Plan

Tested with various styles, turning scrollbars on/off,
various margins, primary and secondary screen.

Diff Detail

R319 Konsole
Lint Skipped
Unit Tests Skipped
sandsmark created this revision.Dec 22 2018, 8:12 PM
Restricted Application added a subscriber: konsole-devel. · View Herald TranscriptDec 22 2018, 8:12 PM
sandsmark requested review of this revision.Dec 22 2018, 8:12 PM
hindenburg edited the summary of this revision. (Show Details)Dec 29 2018, 1:57 AM
hindenburg edited the test plan for this revision. (Show Details)

This would be better if this diff only changed _contentRect; the other changes should be done w/ a later patch.

sandsmark updated this revision to Diff 51201.Feb 8 2019, 5:36 PM

Removed the unrelated cleanup/moving of the code in hotSpotRegion()

Thanks still working on reading these changes.


Did you mean to change contentsRect() to rect()? rect() doesn't appear to take into account the scrollbar


same here


Was the old code having 2 contentsRect().left() a mistake?

sandsmark added inline comments.Mar 2 2019, 7:54 PM

hmm, the only difference between rect() and contentsRect() should be whether it takes into account the margins, I think? Not sure why it would impact the scrollbar.


yeah, I think it worked mostly by accident (I don't think many people use explicit extra margin), and I can't see why you would want to remove it twice.

mglb added a subscriber: mglb.Mar 2 2019, 8:49 PM
mglb added inline comments.

rect() returns whole widget's area, including scrollbar (it is drawn inside rect()). _contentRect (now contentsRect()) represents the actual terminal contents (characters), so scrollbar width is added to one of the margins (see line 2248).
Both contentsRect() and rect() (and even nothing but pe->region()) will work, but limiting the region to contentsRect() cuts out not important rects where characters could not exist.


It was a mistake. _contentRect's margins were including possible contentsMargins().

Did you want to change to using contentsRect() then?