Clean up content rect in TerminalDisplay
Needs ReviewPublic

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

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Summary

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

Repository
R319 Konsole
Lint
Lint Skipped
Unit
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.

src/TerminalDisplay.cpp
1614

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

1633

same here

2035

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

sandsmark added inline comments.Mar 2 2019, 7:54 PM
src/TerminalDisplay.cpp
1614

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.

2035

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.
src/TerminalDisplay.cpp
1614

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.

2035

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

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

It looks like this is still open - do you want to move this to an invent MR?