Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Posts overflow their card #756

Closed
daenney opened this issue Jan 26, 2024 · 14 comments · Fixed by #762
Closed

[Bug]: Posts overflow their card #756

daenney opened this issue Jan 26, 2024 · 14 comments · Fixed by #762
Labels
bug Something isn't working

Comments

@daenney
Copy link

daenney commented Jan 26, 2024

Describe the bug

For some reasons a number of posts seem to "escape" from the card they're rendered in.

I tried disabling and enabling the increased font size and line height options, but that doesn't change it.

Steps To Reproduce

I don't know exactly how to reproduce it, it just started happening. It's in this thread that it happens: https://en.osm.town/@opencage/111823092381561642. It seems to happen at random and so far it seems to only happen to @opencage@en.osm.town.

Logs and/or Screenshots

Screenshot from 2024-01-26 18-25-43
Screenshot from 2024-01-26 18-24-31
Screenshot from 2024-01-26 18-23-45
Screenshot from 2024-01-26 18-19-23

Instance Backend

GoToSocial

Operating System

Arch Linux

Package

Flatpak

Troubleshooting information

os: GNOME 45 (Flatpak runtime)
prefix: /app
flatpak: true
version: 0.6.2 (production)
gtk: 4.12.4 (4.12.4)
libadwaita: 1.4.2 (1.4.2)
libsoup: 3.4.4 (3.4.4)
libgtksourceview: 5.10.0 (5.10.0)

Additional Context

No response

@daenney daenney added the bug Something isn't working label Jan 26, 2024
@daenney
Copy link
Author

daenney commented Jan 26, 2024

It does render correctly in Feditext, and it seems to render fine in their Masto instance UI too, which prompted me to file the bug.

@GeopJr
Copy link
Owner

GeopJr commented Jan 26, 2024

This is a regression due to #644 (I'll include more info for future reference, but the solution will be to partially revert one of the hacks)

While most other labels have a bound width of ~532, the overflowing one has more than double
Screenshot from 2024-01-26 20-26-59

default:

Screenshot from 2024-01-26 20-24-44

ellipsize + lines 100 hack:

Screenshot from 2024-01-26 20-25-18

default + use-markup false:

Screenshot from 2024-01-26 20-24-54

I can't override most of the functions responsible for calculating the width and I don't want to re-create gtklabel in Vala https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtklabel.c

This is probably the culprit of #719 #632 #626

cc: @bugaevc in case you have any insights

@daenney
Copy link
Author

daenney commented Jan 26, 2024

That was a lot faster and more info than I was expecting, thank you! Seems that's going to be annoying to fix.

I'm still curious how it's only posts in that thread that manage to trigger it, but ah well.

@GeopJr
Copy link
Owner

GeopJr commented Jan 26, 2024

Seems that's going to be annoying to fix.

I'm still curious how it's only posts in that thread that manage to trigger it, but ah well.

It's very much out of my expertise. It has to do with at least GTK's basic label widget and probably other circumstances that cause the label dimension (bounds) being incorrect.

The hack previously used would fix that by setting the label to ellipsize (adds ... at the end without displaying the whole content) and setting the desired lines it should use to 100 (so it doesn't actually ellipsize and show the whole content). That seemed to force the label to have a somewhat static size and would avoid the above issue. On #644 Sergey was fixing some low level GTK issues discovered through Tuba and the hacks were removed since they would conflict with it.

This issue was first brought to my attention on the comments of one of the early Tuba releases 😅 Before removing the hack I did check against those posts that caused it then and didn't seem to be an issue anymore but here we are again!

To report it to GTK I need to create at least a simple minimal reproducer but since the circumstances are so narrow (use-markup, larger font and line height, listbox with complicated nested widgets, label-with-widgets etc) it's going to take some time. That's why I tagged Sergey since he is familiar with both Tuba's and GTK's codebases in case he has any insights!

@bugaevc
Copy link
Contributor

bugaevc commented Jan 26, 2024

So FWIW (and I haven't read the details of this issue, yet): we know that GtkLabel is currently pretty broken, and I'm going to work on reworking how it does things wrt wrapping and ellipsizing and measurements.

So if you can make this into a reproducible test case, that'd help.

@GeopJr
Copy link
Owner

GeopJr commented Jan 26, 2024

Done! Probably can be simplified further but this did it. FWIW, the stack was the last piece of the puzzle

public class ExampleApp : Adw.Application {
	public ExampleApp () {
		Object (
			application_id: "com.example.App",
			flags: ApplicationFlags.DEFAULT_FLAGS
		);
	}
  
	public override void activate () {
		var win = new Adw.ApplicationWindow (this);
		win.set_default_size (500, 300);

		var provider = new Gtk.CssProvider ();
		provider.load_from_string (".larger{font-size:larger;line-height:1.4;}.padding{padding:2px;}");
		Gtk.StyleContext.add_provider_for_display (Gdk.Display.get_default (), provider, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);

		var listbox = new Gtk.ListBox () {
			selection_mode = Gtk.SelectionMode.NONE,
			css_classes = {"background"}
		};

		var content_box = new Gtk.Box (Gtk.Orientation.VERTICAL, 0) {
			css_classes = {"card", "padding"},
		};
		content_box.append (new Gtk.Label ("") {
			width_request = 610,
			height_request = 34
		});

		var stack = new Gtk.Stack () {
			margin_top = 15,
			margin_start = 18,
			margin_end = 18,
			margin_bottom = 18
		};
		stack.add_child (new Gtk.Label ("and he <a href='https://en.osm.town/@SK53'>@SK53</a> keeps the momentum with the rarely seen combination of Rwanda 🇷🇼 Romania 🇷🇴 <a href='https://en.osm.town/tags/fridaygeotrivia'>#fridaygeotrivia</a> 👏") {
			use_markup = true,
			wrap = true,
			xalign = 0.0f,
			wrap_mode = Pango.WrapMode.WORD_CHAR,
			valign = Gtk.Align.START,
			ellipsize = Pango.EllipsizeMode.NONE,
			css_classes = {"larger"}
		});
		content_box.append (stack);
		listbox.append (content_box);
  
		win.content = new Gtk.ScrolledWindow () {
			vexpand = true,
			hscrollbar_policy = Gtk.PolicyType.NEVER,
			child = new Adw.Clamp () {
					child = listbox,
					tightening_threshold = 300,
					maximum_size = 670,
					vexpand = true
			}
		};
		win.present ();
	}
  
	public static int main (string[] args) {
		var app = new ExampleApp ();
		return app.run (args);
	}
}

valac --target-glib=2.74 --pkg gtk4 --pkg libadwaita-1 reproducer.vala && ./reproducer

image

@bugaevc
Copy link
Contributor

bugaevc commented Apr 18, 2024

I can't seem to reproduce this, I get a normal-looking layout from your example:

Screenshot from 2024-04-18 19-02-04

(I only had to remove the emoji, as there seems to be some bug on F40 that causes emoji to get HUGE. Please tell me if you can still reproduce on your end without emoji, or if they are important here.)

@GeopJr
Copy link
Owner

GeopJr commented Apr 18, 2024

Yep, the emojis are important. In fact, everything is important, including the exact characters https://gitlab.gnome.org/GNOME/gtk/-/issues/6637

edit:
image

@bugaevc
Copy link
Contributor

bugaevc commented Apr 18, 2024

Screenshot from 2024-04-18 19-36-12
Looks fine with emoji too, save for the -- well, emoji.

Can you try with a fresh GTK from master?

@GeopJr
Copy link
Owner

GeopJr commented Apr 18, 2024

Sorry for the wait, using gnome-nightly flatpak, which is on 4.15.0-c37e88d,

image

@bugaevc
Copy link
Contributor

bugaevc commented Apr 18, 2024

  1. I can't seem to reproduce on a recent commit (latest main, a few commits past c37e88d) either. Hmm.

  2. Partly off-topic, but also very related: could you please test this experimental branch? With this example, and with Tuba overall. You should:

    • set wrap
    • probably also set ellipsize and nat-lines -- but not to work around anything, but because we don't want overly long posts taking over the timeline

    The branch basically reworks label layout in presense of ellipsize / wrap / a line limit, to hopefully make it behave consistently and not break everything.

    Would be super cool if you could test it together with https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/6569 too.

@GeopJr
Copy link
Owner

GeopJr commented Apr 18, 2024

I'll flatpak these just so we are on the same environment. Since these are also pango related, make sure you use gnome defaults (font, size) without fractional scaling etc...

image

I finally have some logs:

(reproducer:2): Gtk-CRITICAL **: 22:52:24.602: Allocation width too small. Tried to allocate 652x68, but GtkListBoxRow 0x55bc27efbbf0 needs at least 889x68.

(reproducer:2): Gtk-CRITICAL **: 22:52:24.602: Allocation width too small. Tried to allocate 648x64, but GtkBox 0x55bc27e4c820 needs at least 885x64.

(reproducer:2): Gtk-CRITICAL **: 22:52:24.602: Allocation width too small. Tried to allocate 648x45, but GtkStack 0x55bc27f11600 needs at least 885x45.

And making the window larger doesn't seem to fix wrapping now (making it smaller does though)

https://github.com/GeopJr/Tuba/tree/experiment/reproducer (flatpak-builder --user --force-clean --install .flatpak-buildder ./reproducer.json)

As for nat-lines + ellipsize + wrap:

Setting it to 20, seems to ellipsize regardless
image

(disabling ellipsize) One of the measuring issues seems to be fixed https://labyrinth.zone/notice/AgzGZoBtX749xlQGeG
image
but not another one https://meta.masto.host/@deckverse/111867070033544002
image

the one from this issue seems to be fixed but nat-lines doesn't work like lines did (ellipsizes always on 1 line) so I also disabled ellipsize here

image

https://github.com/GeopJr/Tuba/tree/experiment/nat-lines

@bugaevc
Copy link
Contributor

bugaevc commented Apr 19, 2024

So, two things:

  1. Gtk.ListBox seems to be always giving its children their minimum height, ignoring the natural height. The way the old lines property worked, this ended up doing what you wanted, i.e. it reported either the actual number of lines or the set limit as the minimum height, and only ellipsized after that. The new min-lines / nat-lines are designed to not work like that; the minimum height of an ellipsizable label is a single line (unless you set min-lines).

    As a workaround/hack, you can make a widget that wraps the label report its natural height as its minimum width, like I've done here. The proper way to solve this would be to change Gtk.ListBox to not do that, and allocate natural size when it makes sense to.

  2. Pango.Layout:height (which backs nat-lines) does this weird thing where it limits the number of lines per paragraph, instead of per layout overall. This is of course rather useless, since people tend to insert line breaks into long texts anyway, and this basically means the text can continue forever, as long as it keeps being split into paragraphs. It does say

    This behavior may be changed in the future to act per layout instead of per paragraph. File a bug against pango at https://gitlab.gnome.org/gnome/pango if your code relies on this behavior.

    so maybe we could go ahead and change that in Pango.

Other than that, this seems to be working great for me actually. No criticals from the box optimization, and the labels seem to behave as intended.

nat-lines = 20:

Screenshot from 2024-04-19 10-27-42

nat-lines = 3:

Screenshot from 2024-04-19 10-28-19

Deckverse:

Screenshot from 2024-04-19 10-29-28

Patting Luna:

Screenshot from 2024-04-19 10-31-47

Rwanda:

Screenshot from 2024-04-19 10-34-32

@GeopJr
Copy link
Owner

GeopJr commented Apr 20, 2024

LGTM, overall, nice work!

To be honest, I don't think ellipsizing at all is appropriate for Tuba however. It's not just labels, so it can end up as:

  • label ellipsized on 20 lines
  • code block widget
  • another label ellipsized on 20 lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants