GNOME Bugzilla – Bug 622450
Add full CSS to StTooltip
Last modified: 2010-11-16 18:27:33 UTC
Created attachment 164357 [details] [review] Patch to StTooltip Currently StTooltip uses ClutterText directly internally, thus only a small subset of implemented CSS properties are recognized. In particular, tooltips get no background or border, which makes them impossible to read. My patch corrects this by rewriting StTooltip as a simple container and sort-of-layout-manager for a StLabel. (I also dropped that horrible bouncing from tooltips, but I suppose all that code should be rewritten to use the new St animation features)
Comment on attachment 164357 [details] [review] Patch to StTooltip This seems like a good plan, although looking at the patch, there is still plenty more oddness in StTooltip. Eg, it is a subclass of StBin, but its child label is not actually the StBin's child, and so StTooltip needs to reimplement all of the container-y methods itself. It seems like maybe StTooltip should just be made a subclass of StLabel? (Unless we want to allow for the possibility of non-textual tooltips, in which case it should stay as an StBin, but have an StLabel child by default, with the label as a proper child of the bin.) The current animation is just awful, yes. I'm not sure what the right default is, or how to make that stylable.
Comment on attachment 164357 [details] [review] Patch to StTooltip >+/* > static void > st_tooltip_style_changed (StWidget *self) of course all the commented-out stuff should just be removed if it's no longer needed >- clutter_actor_animate (self, CLUTTER_EASE_OUT_ELASTIC, >+ clutter_actor_animate (self, CLUTTER_EASE_OUT_SINE, I think the right fix at this point is to just throw out the old animation code; we can use the new StThemeNode transition stuff if we want an animation now. Also, as mentioned before, it should just make the label be its child, and then get_preferred_width, get_preferred_height, allocate, paint, map, unmap, and dispose can all go away.
Created attachment 174341 [details] [review] Rework to use StLabel instead of ClutterText Replaces ClutterText with StLabel inside StTooltip, so full CSS styling is supported.
Comment on attachment 174341 [details] [review] Rework to use StLabel instead of ClutterText (In reply to comment #2) > Also, as mentioned before, it should just make the label be its child, and then > get_preferred_width, get_preferred_height, allocate, paint, map, unmap, and > dispose can all go away. still not addressed.
(In reply to comment #4) > (From update of attachment 174341 [details] [review]) > (In reply to comment #2) > > Also, as mentioned before, it should just make the label be its child, and then > > get_preferred_width, get_preferred_height, allocate, paint, map, unmap, and > > dispose can all go away. > > still not addressed. This was actually intended: a tooltip is not a generic container, it does not make sense to clutter_container_add to a tooltip, when it can handle only text labels. Thinking twice, it may make sense instead (with some modifications to StWidget to handle the most common cases).
Created attachment 174540 [details] [review] StTooltip: turn into an StBin Updated patch
(In reply to comment #5) > (In reply to comment #4) > > still not addressed. > > This was actually intended So you should explain that when attaching the updated patch > a tooltip is not a generic container, it does not > make sense to clutter_container_add to a tooltip, when it can handle only text > labels. Yeah... it would be nice if we could have a way to reuse the StBin/StContainer machinery without actually exposing the container interface. At any rate, StTooltip was already an StBin, and just not using the StBin machinery, which was just weird. So it definitely needed to be fixed in one direction or the other (either making it a real StBin, or making it a subclass of StWidget or StLabel rather than StBin).
(In reply to comment #7) > At any rate, StTooltip was already an StBin ugh... or rather, struct StTooltip and StTooltipClass were built on top of StBin and StBinClass, but the type was derived from G_TYPE_OBJECT...
Comment on attachment 174540 [details] [review] StTooltip: turn into an StBin ok, started reviewing this and found some minor things, but then I got to: >+ clutter_actor_set_parent (CLUTTER_ACTOR (tooltip), clutter_actor_get_stage (CLUTTER_ACTOR (widget))); which won't work if @widget isn't on the stage yet. So you'd need to add some additional complexity for that case, to wait until @widget has been realized, and then parent its tooltip at that point. Anyway, this combined with the fact that I'd been confused before about StTooltip already being an StBin, and the fact that it would be nice to minimize divergence from Mx since we've started trying to work towards re-merging, makes me think you should just commit your previous patch (attachment 174341 [details] [review]) (probably also fixing the struct definitions in st-tooltip.h to reference StWidget rather than StBin).
Comment on attachment 174341 [details] [review] Rework to use StLabel instead of ClutterText Pushed with the struct fixes