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

Synchronized Sync (Mode 2026) #576

Merged
merged 10 commits into from
Sep 13, 2024
Merged

Conversation

ThatOneCalculator
Copy link
Contributor

Deprecates the Alacritty synchronization option for Mode 2026 (https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036)

@ThatOneCalculator ThatOneCalculator marked this pull request as draft July 18, 2024 15:59
@ThatOneCalculator ThatOneCalculator marked this pull request as ready for review July 18, 2024 17:05
@ThatOneCalculator
Copy link
Contributor Author

@karlstav Tested in ghostty, alacritty, kitty, and gnome console. This is a naive implementation but it does seem to work.

@karlstav
Copy link
Owner

karlstav commented Aug 4, 2024

hi @ThatOneCalculator,

sorry for the late feedback. There is one thing I don't understand. Why is there both a begin and end update before and after the draw function? Shouldn't there be BSU (2026h) before (line 1101) and ESU (2026l) after (line 1152)?

@ThatOneCalculator
Copy link
Contributor Author

To be honest, this was a very naive implementation -- I did it to the point where it worked, but I think that I probably made a mistake or two along the way. I am not a C programmer nor a terminal UI programmer, but this fixed works for me, although I doubt it's the right way to do things. Something about having the update go through even though it immediately gets ended makes ghostty not tear. I think it could probably work as you described having the BSU before and the ESU after. Feel free to change my implementation!

@karlstav
Copy link
Owner

If it works, it works. A couple of things though: if the old alacritty sync thing has not been deprecated by alacritty i think we should keep it along side this one. Also I think this sync method also must be a config option and that it must be default off. Never know if it can cause issues.

@ThatOneCalculator
Copy link
Contributor Author

Afaik alacritty now uses the new standard. Tested this on kitty/ghostty/alactitty/foot and all worked

@karlstav
Copy link
Owner

noted, alacritty/alacritty#7365

just re apply the config option (I still believe it should be optional) and squash the commits and then I can merge

@ThatOneCalculator
Copy link
Contributor Author

Should hopefully be able to do soon :D

@ThatOneCalculator
Copy link
Contributor Author

Oh shit I kinda forgot about this

@ThatOneCalculator
Copy link
Contributor Author

@karlstav there we go! Sorry about the very long delay

@ThatOneCalculator
Copy link
Contributor Author

Tested properly now. Works completely as expected!

@karlstav karlstav merged commit f515f93 into karlstav:master Sep 13, 2024
3 checks passed
@karlstav
Copy link
Owner

no problem, hmm looks like I can squash the commits when i merge, i did not know that. seemed excessive with 10 commits for the 5 lines of code change.

@ThatOneCalculator ThatOneCalculator deleted the mode-2026 branch September 13, 2024 20:07
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