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

Defer icon lookup until formatting #1926

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Defer icon lookup until formatting #1926

merged 1 commit into from
Jul 31, 2023

Conversation

bim9262
Copy link
Collaborator

@bim9262 bim9262 commented Jul 28, 2023

See #1774

Let me know if you think this is a good approach to only set the required values needed based on the format that's being used.

@MaxVerevkin
Copy link
Collaborator

I think this kind of optimization has potential. In particular if we could actually skip expensive computation of values that are not used. For example, in net block, we may entirely skip IP address fetch if it is not present in the format.

However I am not sure if doing so for backlight or apt makes much sense. brightness and count are most certainly present - why would anyone use this block but not use it's only feature?

As for icons, instead of resolving them immediately (as in Value::icon(api.get_icon("gpu")?)), we can do so inside a formatter (so this becomes Value::icon("gpu")). This way unused icons are never actually fetched, which takes care of #1774.

@bim9262
Copy link
Collaborator Author

bim9262 commented Jul 30, 2023

As for icons, instead of resolving them immediately (as in Value::icon(api.get_icon("gpu")?)), we can do so inside a formatter (so this becomes Value::icon("gpu")). This way unused icons are never actually fetched, which takes care of #1774.

I like that! I'll give that a go. We'll need to handle the icon progressions in the formatter too...

@bim9262 bim9262 changed the title WIP: Conditional map Defer icon lookup until formatting Jul 30, 2023
@bim9262 bim9262 marked this pull request as ready for review July 30, 2023 19:28
@MaxVerevkin
Copy link
Collaborator

Looks great!

@bim9262 bim9262 merged commit 2720972 into greshake:master Jul 31, 2023
11 checks passed
@bim9262 bim9262 deleted the 1774 branch July 31, 2023 17:02
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