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

Refactor use of ListView #1314

Draft
wants to merge 5 commits into
base: feat/listbox-listview
Choose a base branch
from

Conversation

farao
Copy link
Contributor

@farao farao commented Jan 30, 2025

Attention: Currently not again in a working state. I tried to get this to run but currently it does not.

Two major changes:

  1. Use ListView only for Status containers (Timelines and Threads).
    Reason for this is mostly to not need to adapt all the other Widgets).
  2. In order to use the performance benefit of ListView: differente for
    Status between the setup of the widget and the (re)initialisation of
    the widget with content.

Point 2 could performancewise be (probably significantly) improved by moving more of the initialisation that's not content specific out of init() and into the empty Status constructor.

@GeopJr GeopJr marked this pull request as draft January 31, 2025 07:37
GeopJr and others added 3 commits January 31, 2025 12:54
Currently not again in a working state.

Two major changes:
1) Use ListView only for Status containers (Timelines and Threads).
   Reason for this is mostly to not need to adapt all the other Widgets).
2) In order to use the performance benefit of ListView: differente for
   Status between the setup of the widget and the (re)initialisation of
   the widget with content.

Point 2 could performancewise be (probably significantly) improved by
moving more of the initialisation that's not content specific out of
init() and into the empty Status constructor.
@farao farao force-pushed the feat/listbox-listview branch from 450145b to 6658094 Compare February 2, 2025 21:15
@farao farao marked this pull request as ready for review February 2, 2025 21:16
@farao
Copy link
Contributor Author

farao commented Feb 2, 2025

@GeopJr The CI failure does not really seem to be caused by my change. But anyways this is a nonworking state, so it does not need to go through CI at the moment.

@GeopJr
Copy link
Owner

GeopJr commented Feb 3, 2025

It was fixed on main, just pulled.

On #1301 I have a GTK patch that lowers the recycler's threshold. As it stands, ListView will only start recycling when it hits 200 items, so it's a bit hard to test for Tuba and to target down performance issues. edit: the 200 item threshold is hard coded, the only way to lower it is by compiling GTK. I wanted to make my own recycler for that and other reasons but ListView uses way too many private classes and widgets

One other thing I noticed on #1301, while most accessibility issues I had have been fixed, the scrolled position will jump around due to what I'm assuming is binding and different heights. So say there's a big post, 5k words, right at the start of the recycling view, you scroll a bit, the first post gets unbound so the whole view's height changes dramatically, causing the recycler to load it again since it scrolled up a bit and ends in a loop of binding and unbinding :////

edit: also please keep it as a draft, else I might accidentally merge it

@GeopJr GeopJr marked this pull request as draft February 3, 2025 14:12
@farao
Copy link
Contributor Author

farao commented Feb 5, 2025

Ah, good thanks! I'll try it out in the following days :)

The bug with the chaotic jumps is unfortunately an underlying GTK bug. Fractal has it too (and is therefore near unusable on mobile). This is the gitlab issue: https://gitlab.gnome.org/GNOME/gtk/-/issues/6344. There is some initial investigation in the ticket but unfortunately no progress over many months (even though apparently all applications that use ListView or GridView are affected) :-/

@farao
Copy link
Contributor Author

farao commented Feb 8, 2025

Have you tried out the state in this PR? It still (also with your added commits) doesn't load the timeline for me...

@GeopJr
Copy link
Owner

GeopJr commented Feb 9, 2025

Sorry I havent had the time to take a deep look yet. The main problem is on_create_model_widget (i.item);

4 signals, the two we care about are setup and bind. setup gets called for generating blank widgets which we later bind on bind.

The thing is that we don't have access to the model item on setup, only on bind. So it's null on setup.


Think of it like this, a listview of labels and a model with random names. On setup you just create labels new Gtk.Label (""). On bind you bind the names child.label = item.

The listview generates a bunch of empty widgets (setup) and then recycles them as you scroll (bind).


The only way I can see this working without building the widget on bind is by having the contentbase know in advance what widgets it's going to build and do so in setup, but that won't work when there are different types of widgets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants