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

Reduce ghost emoji flash in title bar #4804

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

liby
Copy link
Contributor

@liby liby commented Jan 8, 2025

Fixes #4799

This PR attempts to reduce the flash caused by the ghost emoji in the title bar when opening new windows.

Changes:

  • Initialize SurfaceView.title with empty string instead of ghost emoji

  • Simplify title computation logic in TerminalView

  • Adding a 500ms fallback timer for "👻"

    • Canceling timer if title is set

Current Status:

While these changes reduce the initial ghost emoji flash, there's still a brief moment where a folder emoji appears alone in the title bar when opening a new window. This suggests there might be a race condition or timing issue with how the title is being set and updated.

demo.mov

Would appreciate feedback on the remaining flash issue and suggestions for further improvements.

@liby liby requested a review from jparise January 8, 2025 13:02
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me! I'm not sure if we should also implement the delay that was proposed in #4799:

When a new window is created, use a blank title and delay the ghost emoji in the titlebar for something like 500ms, and only set it if another title hasn't been set. That should give the shell enough time to startup and set a title if there is one.

... but this change looks like a step in the right direction improvement-wise.

@liby
Copy link
Contributor Author

liby commented Jan 8, 2025

Code looks good to me! I'm not sure if we should also implement the delay that was proposed in #4799:

When a new window is created, use a blank title and delay the ghost emoji in the titlebar for something like 500ms, and only set it if another title hasn't been set. That should give the shell enough time to startup and set a title if there is one.

... but this change looks like a step in the right direction improvement-wise.

Thank you for the review!

I noticed that the flickering issue is more complex than initially thought. The current flickering isn't just about the ghost emoji - it's also related to how the title (folder emoji + path) gets updated.

Even with the 500ms delay for the ghost emoji, I still see a brief flash where the folder emoji appears before the path. This suggests that the issue might be more related to the title update mechanism itself rather than just the ghost emoji timing.

I believe this current implementation is a good first step as it addresses the original issue about the ghost emoji's white flash. For the broader title update flickering, we might want to:

  1. First validate if this improvement helps with the original issue
  2. Create a separate issue to track and investigate the title update mechanism if needed

What do you think?

@jparise
Copy link
Collaborator

jparise commented Jan 8, 2025

I believe this current implementation is a good first step as it addresses the original issue about the ghost emoji's white flash. For the broader title update flickering, we might want to:

1. First validate if this improvement helps with the original issue
2. Create a separate issue to track and investigate the title update mechanism if needed

What do you think?

I think that sounds right. Maybe do (1) as part of this PR's scope before merging it?

@liby
Copy link
Contributor Author

liby commented Jan 8, 2025

I think that sounds right. Maybe do (1) as part of this PR's scope before merging it?

Yes, but if there are better suggestions or directions, I don’t mind improving this PR further.

@mitchellh
Copy link
Contributor

The direction looks good. I think we should continue improving this PR though. For one, I don't like that if nothing ever sets a title then the window is just blank. To be fair, MOST shells (esp with our shell integration) should set the title, but if you run a command or something it will not. That's why I suggested the timer to fallback to a default eventually.

@liby liby force-pushed the fix/title-bar-ghost-emoji-flash branch from 4aa4a73 to 5bfb392 Compare January 8, 2025 22:30
@liby
Copy link
Contributor Author

liby commented Jan 8, 2025

but if you run a command or something it will not.

Interesting case, I hadn't considered a similar scenario before.

I've made a commit to improve this scenario by:

  • Adding a 500ms fallback timer for "👻"
  • Canceling timer if title is set

While I haven't been able to observe this edge case in my testing environment, the improvement should provide a more robust solution for such scenarios.

Copy link
Contributor

@AlexJuca AlexJuca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liby This looks good. Do we have an updated video to visualize how things appear after adding the fallback timer?

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.

Delay "👻" in title bar to prevent FOUC
4 participants