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

Two independent commits #2211

Closed
wants to merge 2 commits into from
Closed

Two independent commits #2211

wants to merge 2 commits into from

Conversation

cbhaley
Copy link
Contributor

@cbhaley cbhaley commented Mar 3, 2024

  1. Fix save-to-disk templates containing template expressions like {:'current_library_name()'} printing an exception. The exception doesn't appear to break anything, but it shouldn't be there.

  2. Use a timer to debounce book details display. This makes startup faster and scrolling smoother.

cbhaley added 2 commits March 3, 2024 14:12
…rrent_library_name()'} printing an exception. The exception doesn't appear to break anything, but it shouldn't be there.
@kovidgoyal
Copy link
Owner

The debounce should be implemented on the book details side.
current_changed() may be used in some other components and so shouldnt
have semantics needed for book details. Instead create a new slot in the
book details that debounces and connect that to current_changed.

@cbhaley
Copy link
Contributor Author

cbhaley commented Mar 4, 2024

Unfortunately that doesn't solve the entire problem. Currently, when current_change is triggered model() indirectly calls get_metadata() (not proxy_metadata). This forces the evaluation of all composite columns, which can take a lot of time for no purpose if the data won't be used.

I will look for alternatives. One that suggests itself is to debounce emitting new_bookdisplay_data. I don't think this has the same problem as debouncing current_changed.

Another companion change would be to have get_metadata() set the value of a composite to None instead of evaluating it. This makes it be evaluated on demand by get_attribute in metadata.book.base(). If I do this then I need to be sure that there aren't any backdoors where a None composite isn't evaluated, for example in get_user_metadata() and get_all_user_metadata().

You can close this PR. I'll make another if I satisfy myself that it works and is worth the trouble.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 4, 2024 via email

@kovidgoyal kovidgoyal closed this Mar 4, 2024
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