Review Request: Fix vertical positionning of "click" message in klineedit

[Aaron Seigo]

> On 2010-05-20 22:01:19, Thomas Lübking wrote:
> > /trunk/KDE/kdelibs/kdeui/widgets/klineedit.cpp, line 1766
> > <http://reviewboard.kde.org/r/4042/diff/1/?file=27060#file27060line1766>
> >
> >     have you tried to just 
> >     cr.setTop(cr.top() + (cr.height() - fm.height()) % 2)
> >     here (or merge it into the above adjustment) and
> >     p.drawText(cr, Qt::AlignLeft|(alignment()&Qt::AlignVertical_Mask) , d->clickMessage);
> 
> Hugo Pereira Da Costa wrote:
>     this "empirically" does not work with even font sizes the same way as "(cr.height() - fm.height() + 1) / 2;", which is used in QLineEdit, hence reintroducing inconsistencies (right ?).
>     
>     All in all, I agree that the code could be made much more compact. but the idea was rather to stick as close as possible to the QLineEdit::paintEvent code, so that future differences will be easier to track down. 
>     
>     On the long run: QLineEdit has its own clickMessage (called placeholderText) since Qt4.7.
>     What we should do is probably wrap KLineEdit::clickMessage around it, without breaking the API. 
>     
>     My worry is that doing so breaks BC. Plus I think the current Qt's placeHolderText is not drawn in italics. 
>     
>     What do you think ?
> 
> Christoph Feck wrote:
>     About the vertical alignment:
>     
>     Please stick to the Qt implementation, and don't even try to be smart simplifying it.
>     
>     I spent two days on Skulpture getting vertical alignment pixel perfect in case of even/odd widget sizes, even/odd font sizes, and user-adjustable vertical alignment in half-pixel steps (I hope you test that?), and had to change most of the code again when Nokia decided to change the font metrics algorithms for Qt 4.6. It was not funny.
>     
>     
>     Regarding placeHolderText:
>     
>     It would not break BC, if you just call setPlaceHolderText() from within setClickMessage().
>     
>     Unless Qt introduces the same feature for QTextEdit (KTextEdit got it with KDE SC 4.4), I would not want to change it, though. So nothing to worry before Qt 4.8. Consistency rulez.
>     
>     And yes, having the text italics, and that even depending on the locale, is something I don't want to give up ;)

"My worry is that doing so breaks BC."

it's fine as long as setClickMesage/clickMesage remain; the can call QLineEdit::[setP|p]laceholderText() internally and the datamemebers in the private class can be removed. shouldn't be a problem.

"Plus I think the current Qt's placeHolderText is not drawn in italics."

probably not worth worrying over. and who knows, maybe we can convince them that italics for this looks better ;) .. or better yet, make sure its styleable.


- Aaron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4042/#review5741
-----------------------------------------------------------


On 2010-05-20 22:34:59, Hugo Pereira Da Costa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4042/
> -----------------------------------------------------------
> 
> (Updated 2010-05-20 22:34:59)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch makes a better job at painting klineEdit 'click' message (e.g. in systemsettings 'search' bar) in a way that is similar to what qlineedit::paintEvent does for normal text. 
> This notably fixes the vertical alignment of the "click" message for some styles, including oxygen (see attached screenshot)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/widgets/klineedit.cpp 1128996 
> 
> Diff: http://reviewboard.kde.org/r/4042/diff
> 
> 
> Testing
> -------
> 
> Tested with oxygen, plastique, skulpture, and bespin. It was checked that the "click message" is vertically positioned the same as normal text (from user input) is, with all styles 
> 
> 
> Screenshots
> -----------
> 
> vertically centered "click" message in systemsettings
>   http://reviewboard.kde.org/r/4042/s/399/
> 
> 
> Thanks,
> 
> Hugo
> 
>