godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix `BottomPanel` excessive width.

Open WhalesState opened this issue 1 year ago • 14 comments

  • Fixes: https://github.com/godotengine/godot/issues/95681

WhalesState avatar Oct 06 '24 11:10 WhalesState

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 (https://github.com/godotengine/godot/issues/95681#issuecomment-2379133094). 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.

tetrapod00 avatar Oct 06 '24 16:10 tetrapod00

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.

WhalesState avatar Oct 06 '24 17:10 WhalesState

We could add text customization to TabBar (see https://github.com/godotengine/godot-proposals/issues/9161 and https://github.com/godotengine/godot/pull/88709). We could also add the ability to change tabs when dragging, as it would be useful for regular docks (added in https://github.com/godotengine/godot/pull/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.

kitbdev avatar Oct 06 '24 18:10 kitbdev

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 avatar Oct 06 '24 21:10 WhalesState

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.

https://github.com/user-attachments/assets/021e1404-d4d8-4d41-9476-72d38b0c9d6a

WhalesState avatar Oct 07 '24 13:10 WhalesState

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.

WhalesState avatar Oct 07 '24 18:10 WhalesState

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

arkology avatar Oct 07 '24 18:10 arkology

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.

WhalesState avatar Oct 07 '24 18:10 WhalesState

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 avatar Oct 07 '24 20:10 KoBeWi

  • [ ] 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.

https://github.com/user-attachments/assets/4c090e74-1352-45a4-8351-b7ce11bb208f

Making ensure_control_visible() deferred can probably fix it.

KoBeWi avatar Oct 07 '24 20:10 KoBeWi

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 ?

WhalesState avatar Oct 07 '24 23:10 WhalesState

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

KoBeWi avatar Oct 08 '24 01:10 KoBeWi

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 avatar Oct 08 '24 01:10 WhalesState

Please rebase to fix a CI problem

AThousandShips avatar Oct 08 '24 16:10 AThousandShips

Why close?

Zireael07 avatar Oct 25 '24 12:10 Zireael07

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.

WhalesState avatar Oct 25 '24 12:10 WhalesState

@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.

hpvb avatar Oct 25 '24 22:10 hpvb

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.

WhalesState avatar Oct 26 '24 21:10 WhalesState

Needs rebase to fix conflicts.

Sometimes it doesn't scroll to the button that was just selected:

https://github.com/user-attachments/assets/26cd30e4-9eb3-419f-94a7-0c1d5b300ed0

but otherwise it looks good.

kitbdev avatar Jan 18 '25 20:01 kitbdev

Needs rebase to fix conflicts.

Sometimes it doesn't scroll to the button that was just selected:

Deferred the call for ScrollContainer::insure_control_visible, it didn't work because the control was already visible then the bottom panel was resized down.

WhalesState avatar Jan 19 '25 12:01 WhalesState

Thanks!

Repiteo avatar Jan 21 '25 17:01 Repiteo

Finally trying this out in 4.4, thank you so so much @WhalesState for fixing this!

TranquilMarmot avatar Jan 31 '25 07:01 TranquilMarmot