Dinamically load icons if cache is being rebuilt (issue #2209) #2238

edwood-grant · 2025-01-02T01:15:14Z

Short summary

This aims to fix #2209

What I've found is that are some methods that will temporarily erase the cached icons folder in order to repopulate. This happens:

On the first time the app is loaded and there is no cache
When the cache is old enough that it warrants another download. Right now is set up to one hour, but the problem is easy to trigger every-time you open if you set this to zero. This is why "sometimes" the icons are bad, because the metadata pre-process only happens when the cache is old enough to trigger this process again.

The method in question is preprocess_metadata. This, for every user and system remote, delete s its cache folder, and checks for the integrity of everything before re-creating the folder icons symbolics links again.

Given some discussion, the conclusion was that it was best to add a signal to detect as soon as possible that each remote manages to update itself and then update the corresponding icon accordingly. The aforementioned icons now use the default icon if it fails to

This PR also addresses the icon load procedure in Package.vala, in order to have priority on the recommended icons to load according to the AppStream API. The order in question is:

Stock icon
Cached Icon
Local Icon
Remote Icon (not being loaded/used right now)

Right now, we are not loading remote icons because of a blocking event when querying the URI. I think it's perfectly possible to dynamically load these, similarly to when the cache is being rebuilt. Worthy of a future PR. This change should be a basic preparation for that.

Tests

Here are some videos of the results. Note that I'm using for some of these fake cache times, just to show stuff.

This video tests loading icons for the first time. Then it reloads to show that, if cache is good, won't load them up again.

2025-01-02-01.13.16.723112574.mp4

This one shows that if you quickly go to the AppInfoView, the loading icon event can still be seen.

2025-01-02-01.13.59.877832494.mp4

Additional info

Sorry about the commit history being a bit polluted, I was doing and undoing stuff. If you need me to squash some commits, I can do that.

…ded.

…ith a borken icon. This also attmepts to load all available icons, including flatpak emote icons via priority according to freedekstop specs.

…rify cache existence). Also removing a not needed critical message

…d icon. Then, change it to the correct icon when you are sure its correspondent cahce remote has been updated properly. This happens aof Banner icons, ListPackageRowGrid icons, and AppInfoView Icons

…esIcon works!

blocking method. Also, ensure no spinner shows when an icon is not available (instead of failing to load icon)

alainm23 · 2025-01-03T10:31:18Z

I think it's a great solution, but I'm not sure if we should use Gtk.Spinner. Maybe just use the default icon until the actual one loads and leverage Gtk.Stack to make the transition smoother.

edwood-grant · 2025-01-03T23:14:17Z

So I implemented the Gtk.Stack way. Wanted some input on the result though.

I suppose the CROSSFADE transition would be the best, smooth but not too much (but maybe other transitions might be better? There are slides and 3d box rotations, but not sure about that).

I also decided to blur and desaturate the default icon a bit via CSS, to avoid confusion if an actual icon cannot be found, so those use the normal default icons (at the moment some flatpaks that use remote icons only have no icon on the AppCenter because we are not dynamically downloading remote icons yet.)

Here is the result:

2025-01-03.18-06-47.mp4

Let me know what you think 👍

Edit: I made some change to removed using named Gtk.Stack children to ref widgets, for performance reasons 👍

…ce reasons

teamcons · 2025-01-06T16:19:54Z

I feel stupid for asking this, but how would it be best to test this out ?

when testing this branch, deleting the appstream or flatpak, or both, folders in cache,
only makes appcenter window take up ~17s (if killed before) or 5s (if wasnt) to appear

it appears fully loaded, no temporary icon or anything.

deleting io.elementary.appcenter only causes #2246

edwood-grant · 2025-01-07T01:01:00Z

Hey @teamcons.

So yeah, it's a bit annoying to test this. The "real test" is to, literally, wait for one hour after opening the AppCenter and closing it. The cache gets invalidated after one hour, so that's how you should test it "for real".

But if you want to do the quick check, you can modify the MAX_APPSTREAM_AGE constant value, located in src/Core/FlatpakBackend.vala, which currently is at 3600 seconds (e.g. one hour).

You can put a small value, like 30, so the cache gets invalidated every 30 seconds or something like that (so you can test that a reload of the AppCenter should not trigger this behavior in between the invalidation timer because the cache is OK). You can also put 0, and it will always invalidate.

In theory, both tests should yield the same results, as the only difference is the AppCenter Flatpak backend defined time for the cache to be invalid.

If you have more questions, let me know 👍

teamcons · 2025-01-07T14:23:01Z

I mean i cam retry, but that is also how ive proceeded a few times, waiting some time before reopening appcenter, and the behaviour is similar - probably it loads too fast ?

edwood-grant · 2025-01-11T14:11:24Z

What you mention can definitely happen, since the loading metadata happens on a separate async process there is a chance that it might happen faster and thus no need to wait to load icons.

I suppose it depends on the machine in that case, but in any case, setting the MAX_APPSTREAM_AGE constant value to 0 should always trigger the load icon behavior without any waiting period. But if the async process happens first, then you might not be able to see it happen.

teamcons · 2025-01-14T18:27:04Z

works as intended. No regression noticed. Does indeed fix 2209

danirabbit

Hey thanks for your patience on reviews!

It seems like there's a number of unrelated changes in this branch. I left some comments.

Do you think we can also break out the icon loading order changes in Package.vala to its own branch? That seems like a change that is worthy of being reviewed on its own and I'm noticing that it has introduced a bit of blurriness with some icons

danirabbit · 2025-02-18T17:52:55Z

src/Views/Homepage.vala

@@ -428,6 +439,18 @@ public class AppCenter.Homepage : Adw.NavigationPage {
        });
    }

+    private int get_app_scale_factor () {


every Gtk.Widget has access to scale_factor: https://valadoc.org/gtk4/Gtk.Widget.scale_factor.html

So this function should be unnecessary

Ah! Noted, I will remove this in Homepage.vala
I noted, however, that in Banner.vala if I don't do a certain type check for this.scale_factor, it will trigger this assertion.

(io.elementary.appcenter:153094): Gtk-CRITICAL **: 09:26:50.675: gtk_widget_get_scale_factor: assertion 'GTK_IS_WIDGET (widget)' failed

This won't happen in Homepage.vala, using this.scale_factor is enough. But on Banner.vala, the assertion will trigger.

My solution for banner would be to do something like this:

var scale = (Gtk.Widget) this != null ? this.scale_factor : 1; var pkg_icon = package.get_icon (128, scale);

But maybe I'm doing something wrong and should do something else. I could also just use this.scale_factor and ignore the assertion. Not sure what you think about this.

danirabbit · 2025-02-18T17:53:57Z

src/Widgets/BackButton.vala

@@ -24,7 +24,9 @@ public class AppCenter.BackButton : Gtk.Button {
            var current_page = (Adw.NavigationPage) get_ancestor (typeof (Adw.NavigationPage));
            var navigation_view = (Adw.NavigationView) get_ancestor (typeof (Adw.NavigationView));
            var previous_page = navigation_view.get_previous_page (current_page);
-            label_widget.label = previous_page.title;
+            if (previous_page != null) {


This seems unrelated to the icon loading, can this be in a separate branch?

Ah, yes, this is indeed a commit I made because the Critical assertions were annoying me and wanted a clean log. But it is indeed not related to the actual PR. I will remove it to keep things clean and set a new PR focused on cleaning Critical assertions only.

danirabbit · 2025-02-18T17:56:34Z

src/Widgets/Banner.vala

        );
    }


    construct {
        var name_label = new Gtk.Label (app_name) {
-            max_width_chars = 50,


Is there a reason why you removed the maximum label width? Seems unrelated to icon loading

Ah! These Banner.vala changes was me experimenting something else when using the spinner widget was a thing I believe. I might have been disorganized myself and let something that shouldn't.

Either way, they don't seem relevant at all. I will remove these since they don't have anything to do with the actual PR. Sorry about that.

danirabbit · 2025-02-18T17:57:05Z

src/Widgets/Banner.vala

@@ -98,10 +116,10 @@ public class AppCenter.Widgets.Banner : Gtk.Button {
        inner_box.append (summary_label);
        inner_box.append (description_label);

-        var outer_box = new Gtk.Box (HORIZONTAL, 0) {
+        var outer_box = new Gtk.Box (HORIZONTAL, 24) {


What's the reason for adding spacing here?

Same as the previous comment about Banner.vala. I will remove these unnecessary changes.

italo-capasso added 14 commits December 31, 2024 07:54

Only add banner and recently updated apps when the cache is fully loa…

dce9a58

…ded.

Ensure packge get_icon method gets a proper icon and does ot end up w…

efcfb0a

…ith a borken icon. This also attmepts to load all available icons, including flatpak emote icons via priority according to freedekstop specs.

Load with FileIcon instead of BytesIcon (should be ok since we now ve…

3b2e072

…rify cache existence). Also removing a not needed critical message

Added css to latest_apps_message to have a slightly bigger font

e912881

Instead of message dialogue in banner. Have a spinner in each unloade…

aaeab44

…d icon. Then, change it to the correct icon when you are sure its correspondent cahce remote has been updated properly. This happens aof Banner icons, ListPackageRowGrid icons, and AppInfoView Icons

Make some flatpak backends unowned

93e5cf9

Eliminate a bunch of critical assertions and unused variables.

4564a8e

Better warnign message for remote icon url fetch

3372135

Remove unused CSS

a69dccb

Restoring some unused code back to original state

ac670cb

Use a better name for generic icon in package

dd81748

Fix for a crash when using FileIcon to load certain remote icons. Byt…

4c0048a

…esIcon works!

Remove remote icons load as it makes get_icon a

aafd3ad

blocking method. Also, ensure no spinner shows when an icon is not available (instead of failing to load icon)

Fixing bad linter errros

687cea9

italo-capasso added 2 commits January 3, 2025 18:16

Implemented a dimmed icon update via Gtk.Stack instead of Gtk.Spinner

17f4940

Changed named Gtk.Stack named icons to ref widget icons for performan…

7b27f52

…ce reasons

danirabbit requested changes Feb 18, 2025

View reviewed changes

italo-capasso and others added 5 commits February 21, 2025 09:43

Removed unnecesary get_app_scale_factor method

7ab887f

Removed unnecesary changes related to PR 2209

978a71e

Merge branch 'main' into italocapasso/issue-2209

bb0916b

Merge branch 'main' into italocapasso/issue-2209

e5510d0

Fixing linting errors

c3aedeb

edwood-grant requested a review from danirabbit February 21, 2025 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dinamically load icons if cache is being rebuilt (issue #2209) #2238

Dinamically load icons if cache is being rebuilt (issue #2209) #2238

edwood-grant commented Jan 2, 2025 •

edited

Loading

alainm23 commented Jan 3, 2025 •

edited

Loading

edwood-grant commented Jan 3, 2025 •

edited

Loading

teamcons commented Jan 6, 2025

edwood-grant commented Jan 7, 2025 •

edited

Loading

teamcons commented Jan 7, 2025

edwood-grant commented Jan 11, 2025 •

edited

Loading

teamcons commented Jan 14, 2025

danirabbit left a comment

danirabbit Feb 18, 2025

edwood-grant Feb 21, 2025

danirabbit Feb 18, 2025

edwood-grant Feb 21, 2025 •

edited

Loading

danirabbit Feb 18, 2025

edwood-grant Feb 21, 2025

danirabbit Feb 18, 2025

edwood-grant Feb 21, 2025

Dinamically load icons if cache is being rebuilt (issue #2209) #2238

Are you sure you want to change the base?

Dinamically load icons if cache is being rebuilt (issue #2209) #2238

Conversation

edwood-grant commented Jan 2, 2025 • edited Loading

Short summary

Tests

Additional info

alainm23 commented Jan 3, 2025 • edited Loading

edwood-grant commented Jan 3, 2025 • edited Loading

teamcons commented Jan 6, 2025

edwood-grant commented Jan 7, 2025 • edited Loading

teamcons commented Jan 7, 2025

edwood-grant commented Jan 11, 2025 • edited Loading

teamcons commented Jan 14, 2025

danirabbit left a comment

Choose a reason for hiding this comment

danirabbit Feb 18, 2025

Choose a reason for hiding this comment

edwood-grant Feb 21, 2025

Choose a reason for hiding this comment

danirabbit Feb 18, 2025

Choose a reason for hiding this comment

edwood-grant Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

danirabbit Feb 18, 2025

Choose a reason for hiding this comment

edwood-grant Feb 21, 2025

Choose a reason for hiding this comment

danirabbit Feb 18, 2025

Choose a reason for hiding this comment

edwood-grant Feb 21, 2025

Choose a reason for hiding this comment

edwood-grant commented Jan 2, 2025 •

edited

Loading

alainm23 commented Jan 3, 2025 •

edited

Loading

edwood-grant commented Jan 3, 2025 •

edited

Loading

edwood-grant commented Jan 7, 2025 •

edited

Loading

edwood-grant commented Jan 11, 2025 •

edited

Loading

edwood-grant Feb 21, 2025 •

edited

Loading