betaflight-configurator icon indicating copy to clipboard operation
betaflight-configurator copied to clipboard

Defer cost of OSD tab font glyph render from tab initialization to time-of-use (cuts tab load time to roughly 1/3 of previous).

Open DavidAnson opened this issue 6 months ago • 11 comments

I don't love this, but it's a sketch of one approach that seems to work.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of font preview images by generating character images on-demand before displaying previews.
    • Ensured consistent updates of font and logo previews when the font manager modal is opened and after font changes.
  • Refactor

    • Centralized font and logo preview updates to modal creation and controlled font preset changes, enhancing performance and user experience.

DavidAnson avatar May 29 '25 06:05 DavidAnson

"""

Walkthrough

The changes modify how and when font character images are generated and previewed. The drawing of font characters is now explicitly triggered during preview rendering or modal creation, rather than automatically on every character addition or font load. This centralizes and defers image generation to specific points in the workflow.

Changes

Files/Paths Change Summary
src/js/LogoManager.js Replaced direct access to cached character image URLs with calls to this.font.draw(i) in drawPreview.
src/js/tabs/osd.js Removed FONT.draw from FONT.pushChar; added modal onCreated callback to trigger previews; introduced flag to defer initial preview on font preset change.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Modal
    participant FONT
    participant LogoManager

    User->>Modal: Open Font Manager modal
    Modal->>FONT: onCreated callback
    FONT->>FONT: preview()
    loop For each character index
        FONT->>FONT: draw(i)
    end
    FONT->>LogoManager: drawPreview()
    loop For each character index
        LogoManager->>LogoManager: font.draw(i)
    end

Possibly related issues

  • betaflight/betaflight-configurator#4494: Both the PR and the issue address when FONT.draw is called for character rendering, shifting from automatic to explicit invocation.

Suggested reviewers

  • haslinghuis """

📜 Recent review details

Configuration used: .coderabbit.yaml Review profile: CHILL Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fcda90380f08eb4fe950d3f6e699b3254f7c49c and 365295883726ead2b3dbed6e95ea9ac94f43ed3b.

📒 Files selected for processing (2)
  • src/js/LogoManager.js (1 hunks)
  • src/js/tabs/osd.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/js/LogoManager.js
  • src/js/tabs/osd.js
✨ Finishing Touches
  • [ ] 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar May 29 '25 06:05 coderabbitai[bot]

Fixes #4494

DavidAnson avatar May 29 '25 06:05 DavidAnson

Note that OSD tab load is faster, BUT the first load of the Font Manager is slower since it has to render all the glyphs then.

DavidAnson avatar May 29 '25 06:05 DavidAnson

FYI, my confidence level here is low to medium because of the lack of encapsulation around the font URL data. If people want to move forward with this PR, I would probably want to gate access behind a helper function that performs the extra draw operation. I also don't like the new delay opening font manager because it seems like nothing is happening for a little bit after clicking the button. (The current delay opening the OSD tab is at least clear that the tab is loading.)

DavidAnson avatar May 29 '25 16:05 DavidAnson

  1. perhaps add a loading message or progress bar (see 3)
  2. gating access behind / using a helper function would improve code encapsulation
  3. but IRL don't experience lag opening font manager with this PR

image

haslinghuis avatar May 29 '25 16:05 haslinghuis

What I think I just realized is that the draw function IS the abstraction and everything should use the return value of that instead of accessing the array. That should simplify this PR. Let me do that and then you can try it and maybe decide if it really needs a loading message or not.

DavidAnson avatar May 29 '25 17:05 DavidAnson

Preview URL: https://36529588.betaflight-configurator.pages.dev

github-actions[bot] avatar May 30 '25 04:05 github-actions[bot]

@haslinghuis, please have a look at the latest commit. I feel better about it mostly, though I wish there were a little less duplication. If you are happy with this, I think it makes sense to merge regardless of whether the SVG idea goes anywhere.

DavidAnson avatar May 30 '25 04:05 DavidAnson

Well, I guess I still want to know if the SVG idea has potential before committing this PR. My change right now still feels a little fragile and I'm worried I may have missed a scenario.

DavidAnson avatar May 30 '25 07:05 DavidAnson

@haslinghuis, I've posted a PR for the SVG change and I feel better about it for almost the same benefit: #4497. This change could ALSO be applied, but I'm worried there may be subtleties that haven't been uncovered yet. That change seems easier to validate.

DavidAnson avatar May 31 '25 20:05 DavidAnson