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.

Sign up for GitHub

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

Jump to bottom

feat: reposition darwin traffic-lights in title bar #6102

Merged
merged 8 commits into from
Feb 5, 2025
Merged

feat: reposition darwin traffic-lights in title bar #6102

merged 8 commits into from
Feb 5, 2025

Conversation

ndom91
Copy link
Contributor

@ndom91 ndom91 commented Jan 28, 2025 •
edited
Loading

🧢 Changes

☕️ Reasoning

@vercel Vercel
Copy link

vercel bot commented Jan 28, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 3:53pm
gitbutler-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 3:53pm

@github-actions github-actions bot added the rust Pull requests that update Rust code label Jan 28, 2025
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from fc8dc4c to 224c84b Compare January 28, 2025 16:19
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from 224c84b to af3f98a Compare January 28, 2025 16:24
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from af3f98a to f36fcb6 Compare January 28, 2025 16:30
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from f36fcb6 to af51129 Compare January 28, 2025 19:15
@Byron
Copy link
Collaborator

Byron commented Jan 29, 2025

With the fix I pushed it compiles and does seem to have an effect.

Screenshot 2025-01-29 at 09 17 39

@Byron
Copy link
Collaborator

Byron commented Jan 29, 2025

CI still fails with terrible build errors on Linux it seems, an issue that isn't reproducible on MacOS.

Does the following reproduce the issue locally on Linux?

cargo check --workspace --all-targets

@ndom91
Copy link
Contributor Author

ndom91 commented Jan 29, 2025

CI still fails with terrible build errors on Linux it seems, an issue that isn't reproducible on MacOS.

Does the following reproduce the issue locally on Linux?

cargo check --workspace --all-targets

Yeah so locally here on my linux box I get the same error messages as in CI 😭

image

@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from 01f08fd to e186ffb Compare January 29, 2025 12:24
@PavelLaptev PavelLaptev force-pushed the feat-traffic-light-positioner branch from e186ffb to 448b022 Compare January 29, 2025 12:41
@PavelLaptev
Copy link
Contributor

@Byron can we merge this?

Byron
Byron approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good to me and I re-validated it after making some final adjustments.

@Byron Byron force-pushed the feat-traffic-light-positioner branch from d1b1fa4 to 21666af Compare February 5, 2025 12:47
@Byron Byron force-pushed the feat-traffic-light-positioner branch from 21666af to 5cc94a7 Compare February 5, 2025 12:53
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from 5cc94a7 to 2140366 Compare February 5, 2025 15:50
@ndom91 ndom91 force-pushed the feat-traffic-light-positioner branch from 2140366 to 8b9aec6 Compare February 5, 2025 15:52
@ndom91 ndom91 enabled auto-merge (rebase) February 5, 2025 15:52
@ndom91 ndom91 merged commit 9d8cd34 into master Feb 5, 2025
23 checks passed
@ndom91 ndom91 deleted the feat-traffic-light-positioner branch February 5, 2025 15:57
mtsgrd
mtsgrd reviewed Feb 7, 2025
View reviewed changes
crates/gitbutler-tauri/src/main.rs
Comment on lines +141 to +149
// NOTE: Make sure you only call this ONCE per window.
{
if let Some(window) = tauri_app.get_window("main") {
#[cfg(target_os = "macos")]
// NOTE: Make sure you only call this ONCE per window.
window
.setup_traffic_lights_inset(LogicalPosition::new(16.0, 25.0))?;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only called once for the app atm, and doesn't reposition new lights in new windows. Also, it applies the same positioning to V2. Let's fix before we need to make a next release!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Byron looks like there was a reason for the 2nd call

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the issues @mtsgrd is describing would be true in multi-window mode and when V3 and V2 is switched while the app is running?
Indeed it's made to only work if the app launches in V3 mode and it's assumed to only have one Window - multi-window support was never made official, and from what I remember it's currently hanging/deadlocking on Windows even. When V3 is switched to V2, the app needs a restart for the buttons to appear in the right spot.

Should there be a follow-up that makes this multi-window compatible?

@Byron looks like there was a reason for the 2nd call

Maybe, but if it wasn't added by you or me, who would know about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Byron Byron Byron approved these changes

@mtsgrd mtsgrd mtsgrd left review comments

Assignees
No one assigned
Labels
@gitbutler/desktop rust Pull requests that update Rust code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndom91 @Byron @PavelLaptev @mtsgrd