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

GUI: HSeparator and VSeparator names don't match their behavior #58469

Open
philippedcote opened this issue Feb 23, 2022 · 18 comments
Open

GUI: HSeparator and VSeparator names don't match their behavior #58469

philippedcote opened this issue Feb 23, 2022 · 18 comments

Comments

@philippedcote
Copy link

philippedcote commented Feb 23, 2022

Godot version

3.4.2 Stable

System information

macOS 12.1 (21C52)

Issue description

It’s pretty simple. Look at the image to understand. Both icons have been swapped. Just need to be swapped to fix.

EDIT: To clear up confusion about this ticket, what was meant here is that VSeparator was expected to be a Divider that is used in a Vertical list layout. Same for HSeparator. Confusing that VSeparator is used in Horizontal list layout.

image

Steps to reproduce

  1. Insert a VSeparator
  2. Insert a HSeparator
  3. Notice the icons have been swapped

Minimal reproduction project

No response

@Calinou
Copy link
Member

Calinou commented Feb 23, 2022

It’s pretty simple. Look at the image to understand. Both icons have been swapped. Just need to be swapped to fix.

Are you saying that the icons' orientation used to be different in an older Godot version? If so, which version?

The icons weren't recently changed:

master:

3.x:

@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2022

I'm pretty sure a PR changed this recently... but I can't find it. Maybe it was another H/V couple.

Edit: Might have been #58323, not directly related.

@akien-mga
Copy link
Member

So I checked in master, and it seems to be that the icons are correct. The problem is likely more the name of the nodes than their icon:
image

The description highlights the confusion:

Vertical version of [Separator]. Even though it looks vertical, it is used to separate objects horizontally.

@akien-mga akien-mga changed the title GUI: HSeparator and VSeparator icons have been swapped GUI: HSeparator and VSeparator names don't match their behavior Feb 23, 2022
@philippedcote
Copy link
Author

philippedcote commented Feb 23, 2022 via email

@Calinou
Copy link
Member

Calinou commented Feb 23, 2022

My opinion is that the separator's prefix should match the one you'd typically be using in a container.

For instance, in a VBoxContainer, you should be using VSeparators. In a HBoxContainer, you should be using HSeparators. This is not the case currently, so I would recommend swapping the separators around. This is a compatibility-breaking change, so it can only be considered for 4.0.

Alternatively, we can consolidate all those H/V variants into a single node with an enum property that decides the orientation. This approach makes run-time UI layout changes easier too.

@akien-mga
Copy link
Member

This was mentioned in #54161 (comment)

VSeparator and HSeparator seem to be semantically flipped: I understand that VSeparator stretches visually up and down, but it separates things on the horizontal axis and serves as a child of a horizontal stack. I feel like the visual, in-editor dimensions of the VSeparator are less important for meaningful/consistent naming than the actual purpose it serves---that of being a horizontal separator. Same goes for HSeparator.

Not gonna lie, the current naming makes more sense to me. It's a separator that's vertical. Very self-evident.

With five 👍 on the answer, showing that it's not as obvious as it sounds.

Alternatively, we can godotengine/godot-proposals#3558 with an enum property that decides the orientation. This approach makes run-time UI layout changes easier too.

That could make sense.

@YuriSizov
Copy link
Contributor

YuriSizov commented Feb 23, 2022

My opinion is that the separator's prefix should match the one you'd typically be using in a container.

I don't think it makes sense. The prefix indicates the quality of the node, not of the container where it might be used. This seems tempting to change for superficial consistency and uniformity, but then the meaning of the node would be lost. Hence why I am one of those five 👍 .

@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2022

Checking a couple UI toolkits, they use "vertical divider" the same way we do "VSeparator": the element itself is drawn as a vertical shape, and separates two columns:
https://semantic-ui.com/elements/divider.html
https://getuikit.com/docs/divider

@KoBeWi
Copy link
Member

KoBeWi commented Feb 23, 2022

That node was always intuitive to me. HSeparator is a horizontal line, it makes perfect sense. I wouldn't change it.

@Zireael07
Copy link
Contributor

Maybe mention "vertical divider" in the description then?

@comods
Copy link

comods commented Feb 23, 2022

VChildSeparator for inside VBoxContainer,
HChildSeparator for inside HBoxContainer?

It is sad that it is more letters, but adding Child, clears up which noun the vertical and horizon adjectives are referring to.

@comods
Copy link

comods commented Feb 23, 2022

VListSeparator for inside VBoxContainer,
HListSeparator for inside HBoxContainer?

@YuriSizov
Copy link
Contributor

That is really not clearing up anything, if I'm honest.

@philippedcote
Copy link
Author

Alternatively, we can consolidate all those H/V variants into a single node with an enum property that decides the orientation. This approach makes run-time UI layout changes easier too.

I like this proposal best among all. That would clear up confusion.

In fact, why choose the orientation at all? Can’t the Separator be smart enough to know which orientation it should go depending on context? I would make "Auto" the default behaviour and let H or V be selectable for compatibility purposes.

@comods
Copy link

comods commented Feb 23, 2022

I agree that merging them together with an Auto as default would be best 👍.

Since that would take coding work, here is a rename idea in the meantime:

RowSeparator for inside VBoxContainer,
ColumnSeparator for inside HBoxContainer?

@pycbouh Could you 👍 this if it does, or 👎 if it doesn't, help clarify between a "vertically drawn line" verses a 'line separating a vertical list'?

@Calinou
Copy link
Member

Calinou commented Feb 23, 2022

In fact, why choose the orientation at all? Can’t the Separator be smart enough to know which orientation it should go depending on context? I would make "Auto" the default behaviour and let H or V be selectable for compatibility purposes.

The orientation can't always be automatically determined, especially in cases where the container's width is equal to its height.

@philippedcote
Copy link
Author

philippedcote commented Feb 23, 2022

The orientation can't always be automatically determined, especially in cases where the container's width is equal to its height.

Indeed. In that case, we should ask the user what to do.

Or… is a Square separator a good option for those cases?

@YuriSizov
Copy link
Contributor

@pycbouh Could you 👍 this if it does, or 👎 if it doesn't, help clarify between a "vertically drawn line" verses a 'line separating a vertical list'?

Like I've said on RC, these names could work, but I don't believe there is a problem to begin with. We would be better off updating the documentation as suggested above. There is always going to be somebody who misunderstands something or is confused by something. We should strive to help all, but can only help most, so if the current name work for most, I'd leave it at that.

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

No branches or pull requests

7 participants