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 Height for Resolve Conflicts Dialog #7815

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexknop
Copy link

@alexknop alexknop commented Jan 31, 2025

Problem:

-With multiple conflicts to fix, sometimes the dialog box would expand beyond the screen in height and could not be resized to a smaller height.

Solution:

-Adjusted height based on content
-Set more reasonable minimum height

Fixes #7785 #6228

Screenshot From 2025-01-31 12-04-14
Screenshot From 2025-01-31 12-12-53

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I have one comment

src/gui/ResolveConflictsDialog.qml Outdated Show resolved Hide resolved
@alexknop alexknop force-pushed the conflictdialogfix branch 2 times, most recently from 3a082e0 to 328338c Compare February 3, 2025 14:22
@alexknop
Copy link
Author

alexknop commented Feb 3, 2025

@claucambra Done. Thank you

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@nilsding nilsding left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

I like the idea of the window adapting its height to how many conflicts are present, however the current approach doesn't seem to work well; when you try to resize the window the height keeps changing until 80% of the screen height is reached.

on macOS the window will just be dragged around when I try to resize it, but on my Plasma setup it's more visible:

Screencast_20250206_103808.webm

@nilsding
Copy link
Member

nilsding commented Feb 6, 2025

also please use rebases instead of merges when updating your branch

@nilsding nilsding linked an issue Feb 6, 2025 that may be closed by this pull request
8 tasks
@alexknop
Copy link
Author

alexknop commented Feb 6, 2025

@nilsding
I think the height glitching like this is due to the Math.min calculation. It's probably recalculating as you resize the window.
I will probably just reduce our minimum height style parameter to something like 400. With my 1366x768 screen, this was going off the screen.
Otherwise maybe I will try to automatically size it based on content list, but not make the window resizeable? Does this window need to be resizeable?

@alexknop
Copy link
Author

alexknop commented Feb 6, 2025

@nilsding I went with the quicker and easier route. Just shrunk the minimum height requirement.
I also reduced some padding to make more use of the space.

@nilsding
Copy link
Member

nilsding commented Feb 7, 2025

Otherwise maybe I will try to automatically size it based on content list, but not make the window resizeable? Does this window need to be resizeable?

I prefer windows to be resizable myself, it's especially useful when using screens with different resolutions.

surely there's got to be some way to only set the height initially, but for now having a smaller minimum size is fine too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants