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

[Feature request]: Status widget needs to be refactored #57

Closed
bleakgrey opened this issue Jan 13, 2023 · 5 comments · Fixed by #424
Closed

[Feature request]: Status widget needs to be refactored #57

bleakgrey opened this issue Jan 13, 2023 · 5 comments · Fixed by #424
Labels
bug Something isn't working performance Performance related issue status Status widget related issue

Comments

@bleakgrey
Copy link
Contributor

Describe the bug

The more I think about Widgets.Status, the more I agree that its architecture has become questionable.

Currently, each Widgets.Status has a pre-instanced Widgets.VoteBox, Widgets.AttachmentBox, and emoji Gtk.FlowBox. These widgets stay even if their status entity doesn't really need them, so they just hang there being invisible. There really is no need to create so many useless widgets.

Ideally, the entire content box should be generated dynamically on demand (when bound to a API.Status) to avoid creating a larger memory footprint. Some widgets can be skipped altogether if the account backend doesn't support some features (like emoji reactions).

To make things worse, Notification widgets derive from Widget.Status, so they all succumb to this behavior as well (even though it makes no sense for a follow request to have a VoteBox at its disposal).

Steps To Reproduce

Inspect https://github.com/GeopJr/Tooth/blob/main/data/ui/widgets/status.ui#L220-L226 and https://github.com/GeopJr/Tooth/blob/main/data/ui/widgets/status.ui#L257-L264

Logs and/or Screenshots

No response

Instance Backend

Mastodon

Operating System

Pop!_OS 22.04 LTS

Package

Flatpak

Troubleshooting information

flatpak: false
version: main-15b8f4a (development)
gtk: 4.8.4 (4.8.4)
libadwaita: 1.2.1 (1.2.1)
libsoup: 2.74.2 (2.74.2)

Additional Context

No response

@bleakgrey bleakgrey added the bug Something isn't working label Jan 13, 2023
@bleakgrey bleakgrey changed the title [Bug]: Status widget needs to be refactored [Feature request]: Status widget needs to be refactored Jan 13, 2023
@GeopJr
Copy link
Owner

GeopJr commented Jan 14, 2023

Definitely agree!

I do have one question for Status widgets: Why do we even bind all their properties? Do they (most of them at least) actually change?

Constructing them once (with only the children they need) is obviously the desired solution but if e.g. they can get a spoiler at some point then I doubt the memory saving is worth the cost of destroying the content label -> creating a Stack, pages, spoiler button, spoiler label, icons, content label...

@bleakgrey
Copy link
Contributor Author

Only a handful of properties need to be reactive, like reblogged, favourited, and bookmarked. For instance, if you bookmark a status in a Conversation, it becomes bookmarked in other Timelines as well for consistency.

If Mastodon supported editing statuses, it would make sense to make content reactive too. I'm not sure about other backends though.

@bleakgrey
Copy link
Contributor Author

bleakgrey commented Jan 14, 2023

Here's a full list of properties that definitely should be reactive:

  • reblogged
  • favourited
  • bookmarked
  • replies_count
  • reblogs_count
  • favourites_count

Should be reactive, but the functionality they represent is currently NYI:

  • muted
  • pinned

Questionable (depends on the backend, needs research):

  • content

@bleakgrey
Copy link
Contributor Author

I was today years old when I learned Mastodon actually supports editing statuses. What.
https://docs.joinmastodon.org/methods/statuses/#edit

@GeopJr
Copy link
Owner

GeopJr commented Jan 15, 2023

Thanks for the info!

Yeah... statuses are now editable - I've only added an "edited indicator" so far.

Not sure what the best plan forward is, binding them and creating them if they don't exist?

Widgets.RichLabel? spoiler_label = { get; set; }

// ...

self_bindings.bind_property ("spoiler-text", spoiler_label, "label", BindingFlags.SYNC_CREATE, (b, src, ref target) => {
	if (spoiler_label != null && src == null) {
		spoiler_label.destroy();
		return true;
	}

	if (spoiler_label == null) {
		spoiler_label = new Widgets.RichLabel();
		content_column.append(spoiler_label);
	}
	target.set_string (accounts.active.visibility[src.get_string ()].icon_name);
	return true;
});

(pseudo untested code)

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

Successfully merging a pull request may close this issue.

2 participants