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 BottomPanel excessive width. #97878

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

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Oct 6, 2024

@tetrapod00
Copy link
Contributor

tetrapod00 commented Oct 6, 2024

Tested locally.

With a typical use case, this allows the bottom bar to stay as narrow as it currently does in 4.3. However, this PR does truncate names unnecessarily by forcing each button to have an equal-length name. Subjectively, forcing equal spacing also looks slightly worse at wide widths.
4.3, minimum width:
Godot_v4 3-stable_win64_7GtDjThK0s
This PR, minimum width:
godot windows editor x86_64_tU3XRykGFX
This PR, maximum width:
godot windows editor x86_64_yKkjVncwZf

With the second MRP in the issue (#95681 (comment)). On a 1080 screen (I use windows with the taskbar on the side, so the width is a bit less than 1080), that MRP still produces a broken UI with this PR. The right side of the inspector is cut off, and the left and right panels still can't be dragged to rescale.
godot windows editor x86_64_4Q6WsY5rl7
To make the problem more clear, this is the MRP with this PR in a window ~1000px wide. The right side of the screen is completely cut off!
godot windows editor x86_64_hoWVGkncPa

I suspect one of the other proposed solutions mentioned in the issue, using a TabBar or HFlowContainer, is a more robust solution.

@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 6, 2024

TabBar is not ideal, unless we find a way to change one tab's text color without affecting the others.

HFlowContainer will just move this from a width issue to a height issue with the same amount of tabs that you already have drawn.

Maybe ScrollContainer is the only way to fix this, but it will require some more work, to allow focus mode for buttons and to focus the button after using Button->set_button_pressed(true) to allow the ScrollContainer to use follow_focus and to scroll to the selected button.

@kitbdev
Copy link
Contributor

kitbdev commented Oct 6, 2024

We could add text customization to TabBar (see godotengine/godot-proposals#9161 and #88709). We could also add the ability to change tabs when dragging, as it would be useful for regular docks (added in #86765).

But the real problem with changing it to a TabBar is the method add_control_to_bottom_panel returns a Button, and we need to keep compatibility.

@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 6, 2024

We can consider creating a custom Container for the bottom panel, similar to TabBar, to handle button layout and behavior, we don't have to tweak the TabBar to solve an issue that doesn't belong to it and also it can break compatibility.

@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 7, 2024

Another try.
The Left and Right buttons are a replacment for the horizontal scroll bar, and to allow scrolling in android editor if they don't have a mouse connected.

BottomPanel2.mp4

@WhalesState WhalesState force-pushed the bottom-panel branch 3 times, most recently from 5e9657d to 5951ddc Compare October 7, 2024 16:32
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Reviewing functionality, not code:
Tested the new version with the arrows. It works great for both typical editor usage and the pathological MRP. I like this solution, it feels very natural.

The arrows should be either disabled (grayed out and non-interactive) or completely hidden when all the buttons fit horizontally (which is most of the time, in typical editor usage). I would personally prefer being hidden entirely, since in typical editor usage you should never need to use these arrows. But you can find a precedent for either hiding or disabling within the BottomPanel itself (toast notifications are disabled/enabled, but the expand vertically button hides and unhides).

@WhalesState
Copy link
Contributor Author

The arrows should be either disabled (grayed out and non-interactive) or completely hidden when all the buttons fit horizontally (which is most of the time, in typical editor usage).

Thank You!

Disabling buttons is safer than hiding them to prevent accidental selection. Additionally, connecting the value_changed signal is not necessary since pressing the button when no room to scroll does nothing.

I'm just laying the groundwork, and it's open to future changes and improvements.

@arkology
Copy link
Contributor

arkology commented Oct 7, 2024

I would prefer these buttons to be hidden, which is typical editor usage 99.999999% of the time.

@WhalesState
Copy link
Contributor Author

To avoid accidental selection we can only hide them when they aren't needed, and then we can show both and disable each one separately when there are many editor plugins.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 7, 2024

The buttons should definitely be disabled when not needed.

Also if hiding is a problem, you could "hide" them by setting modulate alpha to 0. Not sure if such margin out of nowhere is acceptable though.
image

@KoBeWi
Copy link
Member

KoBeWi commented Oct 7, 2024

  • image
    Ctrl is usually used for scroll to the end, so Ctrl and Shift should be swapped here IMO.
  • Scrolling to last element when panel is opened does not work correctly, because of the expand button becoming visible.
kkS2NXlekm.mp4

Making ensure_control_visible() deferred can probably fix it.

@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 7, 2024

The buttons should definitely be disabled when not needed.

Also if hiding is a problem, you could "hide" them by setting modulate alpha to 0. Not sure if such margin out of nowhere is acceptable though.

Hiding was so easy, the issue was disabling the buttons which have required a lot of code and to be deferred to work properly, hiding the buttons when not needed and disabling each one when it reaches it's own limit makes it more usable.

Scrolling to last element when panel is opened does not work correctly, because of the expand button becoming visible.
Making ensure_control_visible() deferred can probably fix it.

I have tried to make it deferred and also have tried to adjust the HScrollBar value or defer the adjustment, but nothing has worked. Also, I have some concerns that if we wait two frames until the scroll updates correctly, the button may change or be freed, editor plugins like Plugin Refresher may cause that.

I will make a final test before pushing the changes.

Should i also check for Key::META beside Key::CTRL for macOS ?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 8, 2024

For updating disable status, maybe connecting value_changed of scrollbar with CONNECT_DEFERRED would suffice? 🤔

@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 8, 2024

For updating disable status, maybe connecting value_changed of scrollbar with CONNECT_DEFERRED would suffice? 🤔

It's enough for the left button, but for the right button it should update when resized or when the button_hbox size changes (ie. adding/removing buttons) and after expand_button becomes visible or hidden. so maybe HScrollBar changed and value_changed can be enough.

@WhalesState WhalesState force-pushed the bottom-panel branch 2 times, most recently from 7b459d6 to 7d7247d Compare October 8, 2024 13:42
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

There are some inefficient calls (see comments), but functionally it's ok.

@AThousandShips
Copy link
Member

Please rebase to fix a CI problem

@WhalesState WhalesState marked this pull request as draft October 18, 2024 20:21
@WhalesState WhalesState deleted the bottom-panel branch October 25, 2024 12:03
@Zireael07
Copy link
Contributor

Why close?

@WhalesState
Copy link
Contributor Author

I want to start by expressing my sincere respect for all Godot developers. I have learned so much from you all, and I am genuinely grateful for the many times you've helped me correct my mistakes and deepen my understanding of Godot. Thank you for your support and guidance.

I apologize if my decision to close my PRs seemed abrupt. I know many developers are currently occupied with GodotCon and the holiday season.

One reason for closing my PRs is that I am working on another Godot fork, and I need to consolidate everything under my main GitHub account. I also felt that my PRs weren’t receiving attention, even those that were already approved and had been pending for over 20 days.

However, there was a deeper issue: several of my merged PRs in the Blazium fork were cherry-picked by the Redot project before these changes were integrated into Godot 4.x. I had applied these changes specifically to the Blazium engine, yet Redot copied my commits, divided them into multiple PRs, and presented them as if they were original contributions without any acknowledgment. This experience was frustrating and discouraging.

Another concern I have is related to the approach toward blocking on GitHub. I'm concerned about the possibility of being blocked if my personal opinions or beliefs don’t align with certain community politics or principles. This has made me cautious, as I deeply value the freedom that open-source projects stand for.

@AThousandShips AThousandShips removed this from the 4.4 milestone Oct 25, 2024
@hpvb
Copy link
Member

hpvb commented Oct 25, 2024

@WhalesState I am sorry to hear that. However the only people that were blocked from Github were people opening spam issues on our bug tracker. The people who claimed they were blocked because of something they said on Twitter were lying, they had posted spam to the Github.

@WhalesState
Copy link
Contributor Author

Godot didn't say anything about this, they didn't correct it and thank you for making it clear, we are left to hear from one side and i didn't care about being blocked anywhere except on github, I always try to stay away from politics and to focus on learning and writing code.
I just need to delete the branch from main and to fork godot from another account, I think I'm doing it wrong and maybe i should have transfered the ownership to another account instead of closing the PRs. It's the first time to deal with such issue and sorry about that.

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.

Too many plugins on the bottom panel make it impossible to resize side panels
9 participants