D21797: do not set a hardcoded minimum size

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R871:fdff5bd4bc20: do not set a hardcoded minimum size 
(authored by sitter).

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21797?vs=59989&id=60721

REVISION DETAIL
  https://phabricator.kde.org/D21797

AFFECTED FILES
  src/backtracewidget.cpp
  src/bugzillaintegration/reportassistantdialog.cpp
  src/drkonqidialog.cpp
  src/ui/backtracewidget.ui
  src/ui/maindialog.ui

To: sitter, #plasma, apol
Cc: apol, cfeck, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D21797: do not set a hardcoded minimum size

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  Looks good.

REPOSITORY
  R871 DrKonqi

BRANCH
  minimums

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma, apol
Cc: apol, cfeck, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter added a comment.


  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.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter updated this revision to Diff 59989.
sitter added a comment.


  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

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21797?vs=59782&id=59989

BRANCH
  minimums

REVISION DETAIL
  https://phabricator.kde.org/D21797

AFFECTED FILES
  src/backtracewidget.cpp
  src/bugzillaintegration/reportassistantdialog.cpp
  src/drkonqidialog.cpp
  src/ui/backtracewidget.ui
  src/ui/maindialog.ui

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter added a comment.


  In D21797#479670 , @cfeck wrote:
  
  > 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.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D21797: do not set a hardcoded minimum size

[Christoph Feck]

cfeck added a comment.


  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.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter added a comment.


  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?

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D21797: do not set a hardcoded minimum size

[Christoph Feck]

cfeck added a comment.


  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()`.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D21797: do not set a hardcoded minimum size

[Christoph Feck]

cfeck added a comment.


  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())`.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter added a comment.


  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.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D21797: do not set a hardcoded minimum size

[Christoph Feck]

cfeck added a comment.


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

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D21797

To: sitter, #plasma
Cc: cfeck, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D21797: do not set a hardcoded minimum size

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
sitter requested review of this revision.

REVISION 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.1
  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)

REPOSITORY
  R871 DrKonqi

BRANCH
  Plasma/5.16

REVISION DETAIL
  https://phabricator.kde.org/D21797

AFFECTED FILES
  src/bugzillaintegration/reportassistantdialog.cpp

To: sitter, #plasma
Cc: plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart