do not set a hardcoded minimum size
ClosedPublic

Authored by sitter on Jun 14 2019, 10:32 AM.

Details

Summary

setting the minimum size manually like this is overriding the actual
calculated minimum size (as per child widgets) which ultimately means that
when a child's effective minimum is larger than the manually set one you
can resize the widget such that the child no longer fits in.
this is most notably observed with the page title widget. it doesn't word
wrap, so it has a specific minimum size: the amount of space it requires to
render the text in one line if its minimum width then exceeds the
600 pixels we had manually set, the widget would get cut off.

setting the minimum size should be entirely unnecessary. if all widgets
have a suitable sizing policy and sizehint the dialog will calculate a
suitable overall sizehint and minimum. getting rid of the manually set
minimum means the dialog can be shrunk exactly as far as the page content
allows it to shrink and no further.

I chased the the minimum size back to kde-runtime in git but it even seems
to predate that, so I have no clue what it was meant to achieve, but I am
almost certain that it was a hacky attempt at hiding bad size policies
or lack of size adjustments elsewhere in the dialog stack.

CHANGELOG: The bug report dialog can no longer be resized to cut off text
FIXED-IN: 5.16.3
BUG: 403408

Test Plan
  • set general font size to 16pt
  • start drkonqi with LANGUAGE=pt_BR
  • report bug
  • observe all pages suffering from being cut off, up until the backtrace page which has manual adjustment logic as of a couple of commits ago (due to its actual sizing changing)

Diff Detail

Repository
R871 DrKonqi
Branch
minimums
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12931
Build 12949: arc lint + arc unit
sitter created this revision.Jun 14 2019, 10:32 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 14 2019, 10:32 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sitter requested review of this revision.Jun 14 2019, 10:32 AM
cfeck added a subscriber: cfeck.Jun 14 2019, 1:11 PM

What happens if you (instead/also) remove the next line?

Practically right now nothing, technically I think (?) Qt goes for the sizeHint() by default, so in theory the resize may have an effect. That said, I am not opposed to dropping that call as well if we can agree on that.

cfeck added a comment.Jun 14 2019, 2:09 PM

I think I now see why we messed with sizes. There are wrapped QLabels in the dialog, and these break minimum size hints.

What we generally do to resolve this issue is to use setMinimumSize(sizeHint()).

cfeck added a comment.Jun 14 2019, 2:12 PM

I guess sizeHint() from a paged dialog doesn't pick all pages, so the correct way to resolve this issue is to iterate over all pages, compute their sizeHint(), and use the combined size as the minium size, see QSize::expandedTo().

Pre-computation is not going to work 100% because the backtracewidget is actually changing its content based on state (a whole other can of worms that :S).

What's the specific problem with wrapped qlabels though? Shouldn't they still correctly hint? I mean, the thing is, the dialog having a minimum size explicitly or not should not matter, each individual page should have a (correct) minimum size based on layout negotiation. If that doesn't work I would suspect that either the layout of that page is bugged or stretchyness/policy is off, no?

cfeck added a comment.Jun 14 2019, 3:47 PM

If the backtrace is filled later, and that would cause the minimumSize to change, then it will automatically get propagated to the window.

A wordwrapped label is supposed to adapt to any with _or_ any height, so its minimum size is its minimum with and its minimum height, but that of course cannot accomodate the complete text.

so its minimum size is its minimum with and its minimum height

Sure, unless the Policy is Minimum/MinimumExpanding, which I think it would need to be in our case when wrapping is enabled. It still has a hint, it just doesn't have a useful minimum, by making the hint minimal-sufficient the labels have a minimum and we can have a useful hint on the dialog at large.

I guess I'll go over all possible pages and see if I can break them and adjust the policies so they no longer break, then we should no longer need the hardcoded minimum.

sitter updated this revision to Diff 59989.Jun 17 2019, 1:00 PM

expand diff based on additional pondering

  • both dialogs now throw away their minimum size
  • the resize calls are gone too
  • instead we now calculate a 16:9 size based off of the sizeHint and resize to that (keeps the appearance much the same while not needing the hardcoding)
  • maindialog and backtracwidget UI files had their qlabels switched to Minimum policies to prevent their text getting cut off (Preferred is free to shrink, and shrinking a QLabel means cutting it off for the most part)
  • dropped window resizing hack in backtracewidget as now the window must resize to fit the changing qlabels in that widget, making the explicit resizing unnecessary

I've tested every single widget/page we have on 16pt and 10pt and they do no longer allow for cutting off labels

Oh, FWIW, I am not sure if defaulting to 16:9 on the report dialog is actually all that awesome, it seems to cause an exhaustive amount of empty space for the most part. Opinions welcome.

apol accepted this revision.Jun 27 2019, 10:24 AM
apol added a subscriber: apol.

Looks good.

This revision is now accepted and ready to land.Jun 27 2019, 10:24 AM
sitter edited the summary of this revision. (Show Details)Jun 27 2019, 11:41 AM
This revision was automatically updated to reflect the committed changes.