Review Request: Limit tooltip size for klipper

[Aaron J. Seigo]

> On Oct. 13, 2012, 6:47 p.m., Aaron J. Seigo wrote:
> > klipper/tray.cpp, line 65
> > <http://git.reviewboard.kde.org/r/106829/diff/2/?file=89398#file89398line65>
> >
> >     this will not work as expected on multi-screen systems. it needs to have the geometry of the screen it will show up on.
> >     
> >     this should probably be put into the notifications UI code instead. this way it will be fixed for any user of the notifications and can be set for the given screen.
> >     
> >     good catch though!
> 
> Xuetian Weng wrote:
>     Yeah, I know it should be something like that, but the problem here is we need to strip done too long qstring, but unfortunately I don't have better idea about this. Or I'd rather put an fixed number here, maybe say about 600 or so, since the icon size in tooltip is fixed (currently), so a fixed number won't look bad I think?
>     
>     Text will wrap automatically so 600 isn't too large in this case..
> 
> Marco Martin wrote:
>     the tooltip should indeed have a maximum width, but in characters rather than pixels.
>     very high dpi screens are coming and pixel based sizes make even less sense then ever
> 
> Xuetian Weng wrote:
>     So In that case I choose to use the "primary" screen size for the width.. People usually have tray on primary screen, no?
>     
>     Pay attention, width / 2 of primary screen doesn't means it will take up half of the screen, long tooltip will be wrapped by Plasma Tooltip (break into at least two line, due to elideText effect), so it will take only about 1/4 of the screen or even smaller.
>     
>     Even in the worst case, that primary screen is things like 2560 and the tray is on an 640x480, the result is still acceptable, since usually the problem is the tooltip is too high (break into too many lines), not too wide.
>     
>     I have considered to use width, but QChar doesn't provide wcwidth equivalent api to wcwidth, so hardcoded QString length is not an optimal solution for Asian Character.

"In that case I choose to use the "primary" screen size for the width.. People usually have tray on primary screen, no?"

That is not a safe assumption. Keep in mind that this should apply to ALL tooltips as well. So it must be adapted to the screen it appears on (and ToolTipManager knows which screen that will be, so the information is there).

I looked at this code today in kdelibs/plasma/private/tooltip_p.h/cpp and it definitely needs some refactoring. The only way I can see to make it work fully in all cases is to introducing a scroll area for the subtext content and elide the title if needed. Right now all the text, both title and content, is in one QTextDocument. So this needs to be broken out. Then the resizing of the tooltip needs to be done a bit fancier: a size big enough to accommodate all text without scrolling bounding to a given proportion of the screen it is shown on. If the content is scrollable, then it will also need to not disappear if the mouse goes into it (so the content can be scrolled)

This is important because the tooltip is used by things like the clock to show timezones.

So ... this sounds like a fairly large set of changes, and it is. At one point someone was working on a QML port of the tooltips, and I think this is where the fix belongs. There is little point to addressing this in the tooltips now given the amount of work it would be.

In the meantime, I'd suggest that klipper simply does not put so much text into the tooltip. It is not possible to know what the appropriate screen size is from in klipper (as the content may be shown anywahere .. this is a visualization detail and so must be done in the visualization, NOT the data). To that end, I actually like the solution in 103482 better. It's simple, but it's all that's really needed: just show the user a prevew of the content. A few hundred characters should be enough.


- Aaron J.


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


On Oct. 13, 2012, 4:31 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106829/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2012, 4:31 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> Klipper already limits the text size in the popup menu, however, it doesn't limit size of tooltip, which can still take up whole screen in some case.
> 
> There used to be another patch have similar idea: https://git.reviewboard.kde.org/r/103482/ , but I tried to use fontMetrics based API instead of hardcoded string length, though there is no easy way to obtain plasma tooltip font size and width is not possible from here (It's not really impossbile, but it will introduce more depenency, which is wanted).
> 
> 
> Diffs
> -----
> 
>   klipper/tray.cpp 3f4fc83 
> 
> Diff: http://git.reviewboard.kde.org/r/106829/diff/
> 
> 
> Testing
> -------
> 
> No problem with an acceptable size.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20121014/24fc5549/attachment.html>