sway_text_node: Fix issues with large dimensions #8586
Conversation
| @@ -87,7 +87,7 @@ static void render_backing_buffer(struct text_buffer *buffer) { | |||
| } | |||
|
|
|||
| float scale = buffer->scale; | |||
| int width = ceil(buffer->props.width * scale); | |||
| int width = ceil(get_text_width(&buffer->props) * scale); | |||
There was a problem hiding this comment.
The whole text was rendered to avoid rendering again when resizing a window.
I believe this change breaks sway_text_node_set_max_width().
There was a problem hiding this comment.
Oh, but we call render_backing_buffer() in sway_text_node_set_max_width(). According to git log it seems like it was there since the beginning.
This begs the question: do we need update_source_box() for anything? Isn't the source box always the full size of the buffer?
There was a problem hiding this comment.
Yeah I also thought that the source box was to optimize resize, but as you mention it's rendered ineffective.
It makes sense to do, but the question is if we want to fix that now or just whack it for now and try again later. From the perspective of fixing the crash I'm leaning towards the latter.
There was a problem hiding this comment.
Yeah, makes sense to fix now and leave optimizations for later.
There was a problem hiding this comment.
Removed the source box.
sway/sway_text_node.c
Outdated
| @@ -60,6 +60,8 @@ static int get_text_width(struct sway_text_node *props) { | |||
| int width = props->width; | |||
| if (props->max_width >= 0) { | |||
| width = MIN(width, props->max_width); | |||
| } else { | |||
| width = MIN(width, 3840); | |||
There was a problem hiding this comment.
8K monitors are 7680px large, so this seems a bit low?
There was a problem hiding this comment.
I mean, it's the boundary for the title text before the max_width has been set by the container code.
We should always be setting the max_width from what I can tell, so I don't think it really makes sense for us to render anything before the max width is determined. I think the code has been written as too generalized for our needs...
There was a problem hiding this comment.
Hmm, I guess we already skip rendering if max_width == 0, so the else branch doens't matter at all. I'll remove it.
There was a problem hiding this comment.
Ah yeah. I suppose max_width isn't a great name here.
kennylevinsen
force-pushed
the
textnode-dimensions
branch
from
8023b06 to
aa15391
Compare
max_width was applied to the source box, but not to the cairo surface. The cairo surface would therefore take on arbitrarily large dimensions according to the required dimensions to fit the text input, which if large enough would cause failures during output rendering and leave a black hole in the titlebar.
The source box is always set to the full buffer dimensions, making it ineffective. Remove it.
kennylevinsen
force-pushed
the
textnode-dimensions
branch
from
aa15391 to
746f225
Compare
There was a problem hiding this comment.
LGTM! Maybe we should wait for a bit in case @Nefsen402 has something else to add :)
|
I have nothing to add. Nice and simple, should have done this since day 1. |
sway_text_node was not respecting max_width during rendering, which could lead to very large cairo surfaces and failures during rendering.
Minimal reproduction: https://gitlab.freedesktop.org/emersion/wleird/-/merge_requests/36