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

remove normalizing of splits #2548

Closed
wants to merge 4 commits into from
Closed

remove normalizing of splits #2548

wants to merge 4 commits into from

Conversation

gempir
Copy link
Contributor

@gempir gempir commented Mar 20, 2021

Could not find any benefit of normalization, but splits got resized to bigger than they should.

Fixes #2362

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

I'm testing this on my machine for now, but I can't see any regressions with this and I have a pretty big split setup with like 3 columns and each having 3-4 rows

gempir added 2 commits March 20, 2021 14:32
Could not find any benefit of normalization, but we lost information
and splits got resized to bigger than they should
Copy link
Collaborator

@leon-richardt leon-richardt left a comment

Choose a reason for hiding this comment

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

I only have input on the changelog entry unfortunately 🤷‍♂️

CHANGELOG.md Outdated Show resolved Hide resolved
gempir and others added 2 commits March 20, 2021 14:45
Co-authored-by: Leon Richardt <leon.richardt@gmail.com>
@pajlada
Copy link
Member

pajlada commented Mar 21, 2021

Without this fix

Before reopening chatterino
2021-03-21-174810_640x480_scrot

After reopening it once
2021-03-21-174828_640x480_scrot

After reopening it twice
2021-03-21-174848_640x480_scrot

With this fix

Before reopening chatterino
2021-03-21-175132_640x480_scrot

After reopening it once
2021-03-21-175142_640x480_scrot

After reopening it twice
2021-03-21-175150_640x480_scrot

There was a single change in the "width flex" here once after fixing but I can't reproduce it so that might've been me fatfingering the split size

TL;DR: Looks good to me!

@pajlada pajlada requested a review from fourtf March 21, 2021 16:56
@fourtf
Copy link
Member

fourtf commented Mar 21, 2021

3x

I recommend this fix instead: #2554

@pajlada
Copy link
Member

pajlada commented Mar 21, 2021

3x

I recommend this fix instead: #2554

I will try it out in the same way I tried this build
image

@gempir
Copy link
Contributor Author

gempir commented Apr 14, 2021

What do I need to do to get this merged? This PR seems to have less regressions than fourtf's and personally less code is more IMO.

@pajlada
Copy link
Member

pajlada commented Apr 17, 2021

What do I need to do to get this merged? This PR seems to have less regressions than fourtf's and personally less code is more IMO.

fourtf's PR #2554 has been updated and fixes all issues now, so I'll go ahead and close this issue.

Thanks for bringing the issue to our attention!

@pajlada pajlada closed this Apr 17, 2021
@gempir gempir deleted the splits branch April 17, 2021 11:53
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.

Height of splits is not accurately loaded
4 participants