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

Tweak property order in the inspector for OptionButton #88145

Merged

Conversation

Ratamacue9112
Copy link
Contributor

Fixes #88141. Moves the items property to below other properties.

@Ratamacue9112 Ratamacue9112 requested a review from a team as a code owner February 9, 2024 17:00
@Ratamacue9112
Copy link
Contributor Author

There was a comment that said:
// "selected" property must come after "item_count", otherwise GH-10213 occurs.
However when I reordered it, no errors were thrown. I'm guessing this was fixed at some point and the comment was never removed.

@AThousandShips

This comment was marked as outdated.

@KoBeWi

This comment was marked as outdated.

@AThousandShips

This comment was marked as outdated.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 9, 2024

I think ADD_ARRAY_COUNT does the declaration.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 9, 2024

That's true! Got it confused because of the slightly backwards named comment, would be clearer if it said must come before "selected"

Note though @Ratamacue9112 that this isn't an error being thrown, so make sure to check the issue report to make sure it doesn't reappear:

Because this was solved by moving this exact thing to where it is

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This does indeed break the class, and selected breaks with this change, it works fine without this change

@Ratamacue9112
Copy link
Contributor Author

Ah sorry, didn't notice that. I'm trying to work out a fix now, any suggestions?

@AThousandShips
Copy link
Member

No, it'd require quite a bit of reorganization I think, you can't change any of the names of the properties, and we can't delay initializing this property, so I'm not sure how it would be possible to fix this without reworking quite a bit

@KoBeWi
Copy link
Member

KoBeWi commented Feb 10, 2024

The initializing can be delayed. You can add a new boolean e.g. initialized and if selected is out of index and initialized is false, store it into e.g. a new queued_selected variable. Then in NOTIFICATION_POSTINTIALIZE check if queued_selected was changed and if it was, assign it to selected. Also set initialized to true.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 10, 2024

It's true we can delay the rest of the processing, what I meant was that we can't directly delay it with some property flag, hence the rework need

@Ratamacue9112
Copy link
Contributor Author

Ratamacue9112 commented Feb 11, 2024

@KoBeWi Where should I put the check for selected being out of index? I've tried it in a few places, which don't seem to work, but I might just be doing it wrong.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 15, 2024

It should be in the setter.

@Ratamacue9112 Ratamacue9112 force-pushed the tweak-option-button-property-order branch from 4e26c81 to e49f941 Compare February 17, 2024 10:32
@Ratamacue9112
Copy link
Contributor Author

I've fixed it now. It only worked when I put check if queued_current was changed in the set_item_count method, instead of in NOTIFICATION_POSTINITIALIZE. I don't know if that affects anything else.

@Ratamacue9112 Ratamacue9112 force-pushed the tweak-option-button-property-order branch from e49f941 to 1fb5bbf Compare February 17, 2024 10:49
@Ratamacue9112 Ratamacue9112 force-pushed the tweak-option-button-property-order branch from 1fb5bbf to 45b9cbd Compare February 17, 2024 11:04
@AThousandShips
Copy link
Member

I will test this out later today :)

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Tested and works correctly, thank you!

@akien-mga akien-mga merged commit 02f4a66 into godotengine:master Feb 17, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Feb 17, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

@Ratamacue9112 Ratamacue9112 deleted the tweak-option-button-property-order branch February 17, 2024 17:43
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.

OptionButton has a foldable Items section above properties that do not belong to it
4 participants