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

Add inner_item_margin_* Theme constants to the Tree control #75460

Conversation

joao-pedro-braz
Copy link
Contributor

@joao-pedro-braz joao-pedro-braz commented Mar 29, 2023

Description

This PR adds the "inner_item_margin" Theme constant to the Tree Control.
It behaves like a horizontal padding (in CSS), but only in the active
writing direction (So on LTR it'll apply a left padding and on RTL right
padding).

This PR also makes use of this newly added feature to adjust the Editor Theme.
Previously, icons, like the ones in the FileSystemDock or even in the SceneTree, didn't have proper spacing between borders and would therefore appear too close to them, which was specially noticeable with large Corner Radius values, such as 6.

Comparisons

Currently (pre-PR)

image
File System Dock

image
File System Dock in RTL

With PR

image
File System Dock

image
File System Dock in RTL

Note 2: This superseeds my first PR and also my second PR

@joao-pedro-braz joao-pedro-braz requested review from a team as code owners March 29, 2023 13:32
@joao-pedro-braz
Copy link
Contributor Author

joao-pedro-braz commented Mar 29, 2023

Basically #74321, but with different branches

@KoBeWi KoBeWi added this to the 4.x milestone Mar 29, 2023
@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch 2 times, most recently from 3574dd7 to 0b4bad9 Compare April 2, 2023 00:41
@KoBeWi
Copy link
Member

KoBeWi commented Apr 25, 2023

The StyleBox does not work well if it draws anything:

godot.windows.editor.dev.x86_64_rzRLAYLYKz.mp4

It overrides any other style.

If your aim is only to increase spacing, maybe it should be a theme constant instead?

@joao-pedro-braz
Copy link
Contributor Author

@KoBeWi

Interesting... In a way that's kinda expected, given that the "item" StyleBox is only for the item itself, so everything in red doesn't really "belong" to the item and is more akin to an outer padding:
image

But I do see how that might be confusing and counter-intuitive!
I'll have a look at implementing it as a theme constant.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 27, 2023

What I meant is that selected and focused style no longer shows, because item overrides it, which is wrong.
But yeah, constant would be better for this use-case anyway.

@joao-pedro-braz
Copy link
Contributor Author

@KoBeWi

Do you rather I force-push to this branch or create another PR altogether?

@KoBeWi
Copy link
Member

KoBeWi commented Apr 27, 2023

Force-push this one, but you'll need to update commit message and title.

@joao-pedro-braz
Copy link
Contributor Author

Sure thing!

@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 0b4bad9 to 20d8892 Compare April 27, 2023 18:24
@joao-pedro-braz joao-pedro-braz changed the title Add new "item" StyleBox to the Tree Control Add the "inner_item_margin" Theme constant to the Tree control Apr 27, 2023
@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 20d8892 to 7178c71 Compare April 27, 2023 18:35
@joao-pedro-braz
Copy link
Contributor Author

@KoBeWi

Done!

Thank you for the suggestion, this new approach is not only simpler, but also stabler when on RTL, given that the text no longer "moves" around when resizing the item.

@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 7178c71 to ecbde52 Compare April 27, 2023 18:41
@joao-pedro-braz
Copy link
Contributor Author

joao-pedro-braz commented Apr 27, 2023

Force-pushed again to update the default value for the editor theme.
Seems to be good to go.

@joao-pedro-braz joao-pedro-braz changed the title Add the "inner_item_margin" Theme constant to the Tree control Add a new "inner_item_margin" Theme constant to the Tree control Apr 27, 2023
@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from ecbde52 to 6590d94 Compare April 27, 2023 19:03
@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 6590d94 to a8453cb Compare May 16, 2023 21:22
@joao-pedro-braz
Copy link
Contributor Author

Github apparently had a stroke and force-closed my issue :/
Reopened and fixed a conflict in tree.cpp

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.

The feature itself works correctly and implementation looks ok.
There is no linked proposal though.

@joao-pedro-braz
Copy link
Contributor Author

There is now: godotengine/godot-proposals#6902
😄

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

The StyleBox does not work well if it draws anything:

I think we should start accepting cases where only StyleBoxEmpty makes sense. We have pleeeeenty of controls that need 4 numeric definitions for each side. Using 4 constants or arbitrary using only one constant doesn't seem practical. Like, why we only add one constant here? And with such vague name too.

If we don't want to go the StyleBoxEmpty route, then this PR needs to be updated to introduce 4 constants, like we did in #70901.

  • item_margin_top
  • item_margin_bottom
  • item_margin_left
  • item_margin_right

@joao-pedro-braz
Copy link
Contributor Author

Interesting, if we were to add the StyleBoxEmpty back I guess it could just act as a container for the margin values (ie, without actually being drawn)?
Also, about the name, I got the inspiration from the "item_margin", which does virtually the same thing, but applied to the outside of the item (From the docs: The horizontal margin at the start of an item. This is used when folding is enabled for the item.).

@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 0d94d51 to ac7807e Compare May 19, 2023 09:36
@joao-pedro-braz
Copy link
Contributor Author

@YuriSizov Went with the 4 constants route, as per #70901.

@YuriSizov
Copy link
Contributor

Also, about the name, I got the inspiration from the "item_margin"

Ah, right. That existing property is some weird hack, but I guess we have to work around it now.

scene/gui/tree.cpp Outdated Show resolved Hide resolved
@kleonc
Copy link
Member

kleonc commented May 20, 2023

Currently (pre-PR)

image

With PR

image

Not sure if it's just me but I do like the "before" look more. The "after" seems like wasting horizontal space for no reason/gain.
What I'm saying is maybe the default editor Theme should be made to look like before (or have a smaller left mergin)? 🤔
(given nobody mentioned it yet it's likely just me 😄)

@joao-pedro-braz
Copy link
Contributor Author

I think the "wasted space" is an illusion because of that icon in particular (the teardrop rainbow).
With other (specially squared) icons, the issue seems more prominent, not to mention with rounded themes, which was originally my inspiration for this.

@YuriSizov
Copy link
Contributor

What I'm saying is maybe the default editor Theme should be made to look like before (or have a smaller left mergin)?

I definitely don't like how it looks now, and it bothered me for years at this point, but I agree that we can use a smaller extra margin by default (and we can perhaps respect the additional spacing editor setting here for a bit of extra space for those who need it).

@joao-pedro-braz
Copy link
Contributor Author

Great idea!
I'll work on it rn

@joao-pedro-braz
Copy link
Contributor Author

Done!

Without additional spacing:
image

With additional spacing (value of 3):
image

@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch 2 times, most recently from b7d8f34 to 6938797 Compare May 20, 2023 12:32
doc/classes/Tree.xml Outdated Show resolved Hide resolved
editor/editor_themes.cpp Outdated Show resolved Hide resolved
This PR adds the "inner_item_margin" Theme constant to the Tree Control.
It behaves like a horizontal padding (in CSS), but only in the active
writing direction (So on LTR it'll apply a left padding and on RTL right
padding).

The Editor Theme has been updated to make use of this and a result items
in Trees and ItemLists no longer "hugs" their border, expressing a proper
spacing instead.
@joao-pedro-braz joao-pedro-braz force-pushed the add_new_item_stylebox_to_the_tree_control branch from 6938797 to 670b7be Compare June 2, 2023 09:33
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jun 2, 2023
@YuriSizov YuriSizov merged commit b4a1129 into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov changed the title Add a new "inner_item_margin" Theme constant to the Tree control Add inner_item_margin_* Theme constants to the Tree control Jul 19, 2023
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.

4 participants