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

Fix broken macOS check #690

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Fix broken macOS check #690

merged 2 commits into from
Sep 30, 2024

Conversation

luk1337
Copy link
Contributor

@luk1337 luk1337 commented Jul 15, 2024

No description provided.

@luk1337
Copy link
Contributor Author

luk1337 commented Jul 15, 2024

Before:
Screenshot_20240716_005940

After:
Screenshot_20240716_005931

@luk1337 luk1337 changed the title Fix minimujm heights for split labels Fix minimum heights for split labels Jul 15, 2024
@selurvedu
Copy link

@luk1337 you posted the same screenshot twice.

@luk1337
Copy link
Contributor Author

luk1337 commented Aug 1, 2024

@luk1337 you posted the same screenshot twice.

no, look at the highlighted part:
image

@selurvedu
Copy link

Oooh, riiight. That's so hard to notice if you don't know where to look. Sorry for the noise. 😅

@luk1337
Copy link
Contributor Author

luk1337 commented Aug 1, 2024

Oooh, riiight. That's so hard to notice if you don't know where to look. Sorry for the noise. 😅

np, you can also notice that SSL icon has different style ^^

@Et0h
Copy link
Contributor

Et0h commented Sep 28, 2024

The fix looks good on Windows and Linux, but we have not tested it on macOS and the first change seems to be targeted at that OS. Have you tested your first change on macOS to confirm it fixes an issue rather than introducing one?

@luk1337
Copy link
Contributor Author

luk1337 commented Sep 28, 2024

The fix looks good on Windows and Linux, but we have not tested it on macOS and the first change seems to be targeted at that OS. Have you tested your first change on macOS to confirm it fixes an issue rather than introducing one?

ok, I tested it on macOS and this was breaking it, so I just restored it to how it was before.

if isMacOS():
    window.outputlabel.setMinimumHeight(21)
else:
    window.outputlabel.setMinimumHeight(27)

And with just fixing broken macOS check, this change now only affects non-macOS.

@luk1337 luk1337 changed the title Fix minimum heights for split labels Fix broken macOS check Sep 28, 2024
@luk1337
Copy link
Contributor Author

luk1337 commented Sep 28, 2024

The fix looks good on Windows and Linux, but we have not tested it on macOS and the first change seems to be targeted at that OS. Have you tested your first change on macOS to confirm it fixes an issue rather than introducing one?

ok, I tested it on macOS and this was breaking it, so I just restored it to how it was before.

if isMacOS():
    window.outputlabel.setMinimumHeight(21)
else:
    window.outputlabel.setMinimumHeight(27)

And with just fixing broken macOS check, this change now only affects non-macOS.

image

@Et0h
Copy link
Contributor

Et0h commented Sep 30, 2024

Thanks for the additional testing and fixes. The updated version seems good to go! 👍

@Et0h Et0h merged commit 4f1fe8d into Syncplay:master Sep 30, 2024
2 of 3 checks passed
@luk1337 luk1337 deleted the luk/split branch September 30, 2024 18:52
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.

3 participants