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 bottom panel sizing with multiple options #99594

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

Conversation

Giganzo
Copy link
Contributor

@Giganzo Giganzo commented Nov 23, 2024

Fixes: #95681

Did see this issue a while ago and thought it looked interesting, as I needed to create something similar in gdscript and if a version could be made for the editor.

I did now see there is PR for this already. Haven't looked how it is implemented or tested it.
If not used in its entirety maybe something here can be useful or combined in to a PR.

Features:

I first made a version with < prev and next > buttons. But decided to remove them to save space in the bottom panel, as the popup menu gives you a good overview of all the options.

Before:
Screenshot_20241123_085708

After (looks the same):
Screenshot_20241123_085542

Compact

Default:
Screenshot_20241123_090050

Light:
Screenshot_20241123_090116

Oled:
Screenshot_20241123_090141

Resizing and scrolling between options:

You can speed up scroll by holding alt key.

Screencast_20241123_091418.webm

Scrolls to clicked option:

Screencast_20241123_091707.webm

Scrolls to popup menu selection:

Popup only shows buttons visible in the bottom panel.

Screencast_20241123_092159.webm

Scrolls to the editor opened by events such as clicking on a node in the tree:

Screencast_20241123_092408.webm

@Giganzo Giganzo requested review from a team as code owners November 23, 2024 18:58
@AThousandShips AThousandShips added bug topic:editor cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 23, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 23, 2024
@@ -125,6 +143,65 @@ bool EditorBottomPanel::_button_drag_hover(const Vector2 &, const Variant &, But
return false;
}

void EditorBottomPanel::_focus_pressed_button(Button *p_btn) {
if (!Variant(p_btn).get_validated_object()) {
Copy link
Member

@AThousandShips AThousandShips Nov 23, 2024

Choose a reason for hiding this comment

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

This won't work, you should use an ObjectID instead if you need to handle freed objects

get_validated_object only makes sense for a Variant created from a valid pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, how would I do that?

Copy link
Member

Choose a reason for hiding this comment

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

You would get the ObjectID using get_instance_id and then fetch the object with ObjectDB::get_instance

@Giganzo Giganzo force-pushed the bottom-panel-bar branch 3 times, most recently from 8015f70 to 2bcb8a0 Compare November 23, 2024 19:54
@tetrapod00
Copy link
Contributor

For testing, I re-uploaded the two extreme MRPs from the issue for convenience:

This PR handles both of them just fine:
godot windows editor x86_64_RXN3MlIZaH
godot windows editor x86_64_ZLIEwlNIoM

I'm not sure why, but in the Many Small MRP, this PR can't open any of the custom bottom panel tabs besides Test0. I'm not sure if that's a problem with this PR or with the MRP. These MRPs are not necessarily robust implementations of bottom panel plugins; they mostly exist as proof of concept of the problem existing.

godot.windows.editor.x86_64_CxssMxSWXO.mp4

So the really bad cases seem solved by this (once the issue with Many Small is solved). I also tested casually with a more normal workload using only built-in bottom panels, and it works as expected.

Is it possible to have the popup appear so the base is aligned with the top of the three dot button, rather than covering it?
godot windows editor x86_64_ukTKiBNi89

Other three dot buttons spawn their popups directly below the button, they don't cover the button:
godot windows editor x86_64_96oBLgxudu
godot windows editor x86_64_3v163izSS0

@AThousandShips AThousandShips dismissed their stale review November 24, 2024 09:51

Resolved (will do a review later)

@Giganzo
Copy link
Contributor Author

Giganzo commented Nov 24, 2024

Is it possible to have the popup appear so the base is aligned with the top of the three dot button, rather than covering it?

Should open like this now
Screenshot_20241124_054116

@tetrapod00
Copy link
Contributor

tetrapod00 commented Nov 24, 2024

The last editor artifact is broken for me on Windows 10:

godot windows editor x86_64_w6dUCWTsD0

Edit: If I spam click on it, it works correctly. This might be an unrelated bug, it seems familiar.

godot windows editor x86_64_qdm8BwasHT

Yep, it's #99581 or something similar.

@Giganzo
Copy link
Contributor Author

Giganzo commented Nov 25, 2024

The last editor artifact is broken for me on Windows 10:
Yep, it's #99581 or something similar.

This didn't happen before?

Tried another approach with the button. Does this also trigger the above issue?

@tetrapod00
Copy link
Contributor

godot.windows.editor.x86_64_MKuKmqzoKP.mp4

With this one, it's easier to get a correctly spawned popup, but it's still possible to get the bad version of the popup by spamming left click. I really suspect that this problem is unrelated to your PR, and is something similar to #99581. But I haven't reviewed your actual code and I don't know enough to, all I can do is test the functionality.

@Giganzo Giganzo force-pushed the bottom-panel-bar branch 2 times, most recently from b4e3256 to 7a548e3 Compare November 25, 2024 04:24
@Giganzo
Copy link
Contributor Author

Giganzo commented Nov 25, 2024

Thanks for testing! About the spam clicking, missed a bit of code in your latest test. Added it in now, hopefully it helps was told it prevented the issues of empty popup from one tester.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 19, 2024

Both this PR and #97878 use ScrollContainer, but the other PR uses side arrows instead of popup:
image
They make it more obvious that the buttons are scrollable. Also the popup isn't very useful, unless you have lots of buttons 🤔

@WhalesState
Copy link
Contributor

WhalesState commented Jan 13, 2025

We can have the menu button before the left arrow button, so why don't we combine both ? Also we can allow hiding the menu button in editor settings because i also don't like it, but others may see it useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:editor
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
5 participants