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

Make ScrollBar account for its margins #80013

Closed

Conversation

atlas-wheatear
Copy link

@atlas-wheatear atlas-wheatear commented Jul 29, 2023

This is a bug fix for #79994 .

There are two main changes:

  1. adjust the various components of the scrollbar and its background, so that they take into account the margins.
  2. remove the redundant method get_area_offset

Fixed view:

new_after.mov

The old version, off of the current master branch:

old_master.mov

@atlas-wheatear atlas-wheatear requested a review from a team as a code owner July 29, 2023 12:22
@AThousandShips
Copy link
Member

Can you provide a capture of before and after to make it easier to confirm the fix?

@atlas-wheatear

This comment was marked as outdated.

@AThousandShips

This comment was marked as outdated.

@atlas-wheatear
Copy link
Author

atlas-wheatear commented Jul 29, 2023

Apologies, it's not very clear!

If you know how to change the colour of the "scroll-bar background", for ease of visual discernment, that would be much appreciated!

@AThousandShips
Copy link
Member

Can you please squash your commits, per our guidelines

@AThousandShips
Copy link
Member

Please name your commit something more descriptive, the title of this PR is good, use git commit --amend

@atlas-wheatear
Copy link
Author

Lots of git push -f noise, sorry 😅 I accidentally added a merge commit to the source branch, on my fork, and had to try again.

@kleonc kleonc requested a review from YuriSizov October 1, 2023 10:22
@kleonc
Copy link
Member

kleonc commented Oct 1, 2023

cc @YuriSizov #79994 (comment)

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Oct 2, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Oct 23, 2023

This fix isn't really correct. Content margin should not affect drawing, it's there to define padding for the child content (e.g. child nodes of a PanelContainer or anything that draws over stylebox). In that regard the current behavior is correct and the fix should be done in the theme.

With your changes, the stylebox is smaller when it has margins:
image
vs new:
image

Also, since you only changed the drawing code, it doesn't correctly detect hover:

godot.windows.editor.dev.x86_64_61V8dkNfe0.mp4

@atlas-wheatear
Copy link
Author

I don't have the free time to look at this any longer. There's an awful lot of interacting pieces!

Hopefully the gathered information (breaking commit version, etc) is useful to somebody else.

@atlas-wheatear atlas-wheatear deleted the bugfix-slider-margin branch October 23, 2023 16:02
@AThousandShips AThousandShips removed this from the 4.2 milestone Oct 23, 2023
@YuriSizov
Copy link
Contributor

Thanks for your efforts anyway!

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

Successfully merging this pull request may close these issues.

Editor scroll bar's gutter don't reach its ends
6 participants