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

Editor property selector does not clamp description #83495

Closed
KoBeWi opened this issue Oct 17, 2023 · 13 comments · Fixed by #83745
Closed

Editor property selector does not clamp description #83495

KoBeWi opened this issue Oct 17, 2023 · 13 comments · Fixed by #83745

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Oct 17, 2023

Godot version

4.2 beta1

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1060 (NVIDIA; 30.0.15.1403) - Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz (8 Threads)

Issue description

ZUGV8CMUXg.mp4

The description should be scrollable probably.

Steps to reproduce

  1. Add AnimationPlayer and animation
  2. Click Add Track
  3. Select some node and click a property with a long description

Minimal reproduction project

N/A

@namedtoaster
Copy link

Idk if I'll be able to work on this, but I started looking into it. I found where the description is created and it uses a MarginContainer subclass. I think the solution is to just add it to a ScrollContainer, but when I tried that, I couldn't expand the width. Would that be a good way to fix this? If so, how do you set the width of the container so it expands like the margincontainer seems to already do?

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 20, 2023

Set horizontal flags to EXPAND_FILL.

@quirkylemon
Copy link
Contributor

I might have fixed it yesterday but then forgot to post about it.

The issue was solved by disabling set_fit_content in the rich text label in help_bit
and setting a custom minimum size to it so it isn't to short.

but some questions are

  • is it OK to add more dependencies? this adds core/math/vector2.h to editor/property_selector.cpp
  • should the height of the description be a set value or is it worth adding it to settings?

please have patience I am new to Github and c++ :)

Screenshot from 2023-10-20 10-27-12

@AThousandShips
Copy link
Member

It already has a reference to it so no need to add the include, all Control derived types do have access to it

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 20, 2023

It would be nice if the box wasn't a fixed size. It should use minimum size and only grow when necessary, up to a certain height. Not sure how to achieve it though 🤔

@quirkylemon
Copy link
Contributor

I am working on that now.

@namedtoaster
Copy link

namedtoaster commented Oct 21, 2023

Screen.Recording.2023-10-20.at.6.12.17.PM.mov

So NOT this, right? It's a fixed height clearly

@Aman7s
Copy link

Aman7s commented Oct 21, 2023

@namedtoaster

Screen.Recording.2023-10-20.at.6.12.17.PM.mov
So NOT this, right? It's fixed clearly

@namedtoaster I think you have solved the issue but not push it to repo and show us source code what you have done ?

@quirkylemon
Copy link
Contributor

what should the height be clamped to?

@quirkylemon
Copy link
Contributor

quirkylemon commented Oct 21, 2023

I think 135 is a good minimum height it cleanly holds 8 lines
also I won't be adding scaling based on the amount of text.
image

@namedtoaster
Copy link

@quirkylemon just curious does your code change have the same effect as the one in my video? Like I said I probably won't push a PR but wanted to see if that's what the goal was or not

@quirkylemon
Copy link
Contributor

yeah
should I go ahead and do a PR?

@namedtoaster
Copy link

Go for it

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

Successfully merging a pull request may close this issue.

6 participants