DevHeads.net

Review Request: kfileplaceeditdialog too small

Review request for kdelibs.

Description
Hi, this patch sets a minimum size for the widget.

This addresses bug 266435.
<a href="http://bugs.kde.org/show_bug.cgi?id=266435" title="http://bugs.kde.org/show_bug.cgi?id=266435">http://bugs.kde.org/show_bug.cgi?id=266435</a>

Diffs
kfile/kfileplaceeditdialog.cpp d798b4d

Diff: <a href="http://git.reviewboard.kde.org/r/102751/diff/diff" title="http://git.reviewboard.kde.org/r/102751/diff/diff">http://git.reviewboard.kde.org/r/102751/diff/diff</a>

Testing
it's working

Thanks,

Greg T

Comments

Re: Review Request: kfileplaceeditdialog too small

By Greg T at 10/04/2011 - 14:27

(Updated Oct. 4, 2011, 7:27 p.m.)

Review request for kdelibs.

Changes
improved. I set a max length

Description
Hi, this patch sets a minimum size for the widget.

This addresses bug 266435.
<a href="http://bugs.kde.org/show_bug.cgi?id=266435" title="http://bugs.kde.org/show_bug.cgi?id=266435">http://bugs.kde.org/show_bug.cgi?id=266435</a>

Diffs (updated)
kfile/kfileplaceeditdialog.cpp d798b4d

Diff: <a href="http://git.reviewboard.kde.org/r/102751/diff/diff" title="http://git.reviewboard.kde.org/r/102751/diff/diff">http://git.reviewboard.kde.org/r/102751/diff/diff</a>

Testing
it's working

Thanks,

Greg T

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/04/2011 - 14:28

(Updated Oct. 4, 2011, 7:28 p.m.)

Review request for kdelibs.

Summary (updated)
kfileplaceeditdialog lineedit too small

Description
Hi, this patch sets a minimum size for the widget.

This addresses bug 266435.
<a href="http://bugs.kde.org/show_bug.cgi?id=266435" title="http://bugs.kde.org/show_bug.cgi?id=266435">http://bugs.kde.org/show_bug.cgi?id=266435</a>

Diffs
kfile/kfileplaceeditdialog.cpp d798b4d

Diff: <a href="http://git.reviewboard.kde.org/r/102751/diff/diff" title="http://git.reviewboard.kde.org/r/102751/diff/diff">http://git.reviewboard.kde.org/r/102751/diff/diff</a>

Testing
it's working

Thanks,

Greg T

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/04/2011 - 15:02

(Updated Oct. 4, 2011, 8:02 p.m.)

Review request for Dolphin and kdelibs.

Changes
Probably I should add dolphin, too ?!

Description
Hi, this patch sets a minimum size for the widget.

This addresses bug 266435.
<a href="http://bugs.kde.org/show_bug.cgi?id=266435" title="http://bugs.kde.org/show_bug.cgi?id=266435">http://bugs.kde.org/show_bug.cgi?id=266435</a>

Diffs
kfile/kfileplaceeditdialog.cpp d798b4d

Diff: <a href="http://git.reviewboard.kde.org/r/102751/diff/diff" title="http://git.reviewboard.kde.org/r/102751/diff/diff">http://git.reviewboard.kde.org/r/102751/diff/diff</a>

Testing
it's working

Thanks,

Greg T

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/05/2011 - 13:22

(Updated Oct. 5, 2011, 6:22 p.m.)

Review request for Dolphin and kdelibs.

Description
Hi, this patch sets a minimum size for the widget.

This addresses bug 266435.
<a href="http://bugs.kde.org/show_bug.cgi?id=266435" title="http://bugs.kde.org/show_bug.cgi?id=266435">http://bugs.kde.org/show_bug.cgi?id=266435</a>

Diffs (updated)
kfile/kfileplaceeditdialog.cpp d798b4d

Diff: <a href="http://git.reviewboard.kde.org/r/102751/diff/diff" title="http://git.reviewboard.kde.org/r/102751/diff/diff">http://git.reviewboard.kde.org/r/102751/diff/diff</a>

Testing
it's working

Thanks,

Greg T

Re: Review Request: kfileplaceeditdialog lineedit too small

By Christoph Feck at 10/05/2011 - 07:16

Setting the width to "30+needed" looks wrong, too. This means depending on the current URL, it could grow very big, or be too small.

What I do in those cases is decide about a "good average" text length (for bookmarked Places URLs this could be 40 chars), then use "fontMetrics.height() * chars / 2" as its width hint, because the average height of a font is twice it's width. This also works good for CJK fonts, where each character is usally as wide as it is tall, but where you need less characters to write the same.

- Christoph Feck

On Oct. 4, 2011, 8:02 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/05/2011 - 13:23

Thank you for your kind explanation. I hope I have understood you right.

- Greg

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By David Faure at 10/05/2011 - 06:30

Why the setMaxLength?? What if one wants to type in a long URL?

Also, I can't reproduce the bug here (kde-4.7), but maybe only because the big icon button makes the dialog quite large?

- David Faure

On Oct. 4, 2011, 8:02 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/28/2011 - 05:22

Hey is your patch already submitted?

- Greg

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Christoph Feck at 10/28/2011 - 06:58

Yes, see the bug report. But appearantly, the commit hook did not close the review request. Please mark it as submitted.

- Christoph

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Greg T at 10/05/2011 - 13:23

indeed, the setmaxLength was stupid

it's quite small on 4.6.5 <a href="http://wstaw.org/m/2011/10/05/plasma-desktopF27745.jpg" title="http://wstaw.org/m/2011/10/05/plasma-desktopF27745.jpg">http://wstaw.org/m/2011/10/05/plasma-desktopF27745.jpg</a>

- Greg

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Christoph Feck at 10/06/2011 - 06:07

First, averageCharWidth() is slow. It goes through every character to compute the average. Second, that average might be skewed when there are broken characters in the font.

I'll add a comment why the lineedits width depends on the fonts height.

- Christoph

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By Peter Penz at 10/06/2011 - 03:15

Using QFontMetrics::averageCharWidth() might be an alternative in comparison to use the height as reference for calculating the width (but probably this has some drawbacks I'm not aware of).

- Peter

On Oct. 5, 2011, 6:22 p.m., Greg T wrote:

Re: Review Request: kfileplaceeditdialog lineedit too small

By David Faure at 10/06/2011 - 02:52

Ah I see, it's the german translation which makes the label longer ("Choose an icon" -> "Wählen Sie ein Symbol aus"), leaving less room for the lineedits.

Patch 3 looks good, although I can't help but think there's a bug if a set*Width() call takes a height() as value :-) But I'll trust Christoph on this one.

- David

On Oct. 5, 2011, 6:22 p.m., Greg T wrote: