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 set_slot_custom_icon and get_slot_custom_icon to GraphNode #82669

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

4d49
Copy link
Contributor

@4d49 4d49 commented Oct 2, 2023

Add set_slot_custom_icon & get_slot_custom_icon to GraphNode.

@4d49 4d49 requested review from a team as code owners October 2, 2023 09:37
@KoBeWi KoBeWi added this to the 4.x milestone Oct 2, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2023

Is there a proposal?

@ze2j
Copy link
Contributor

ze2j commented Oct 2, 2023

I was about to make a very similar PR too :)

I didn't find a proposal but, IMHO, the justification is obvious: these icons are the only parameters of the set_slot method which cannot be modified individually. This PR fixes this.

void set_slot(int p_slot_index, 
              bool p_enable_left,
              int p_type_left,
              const Color &p_color_left, 
              bool p_enable_right,
              int p_type_right, 
              const Color &p_color_right, 
              const Ref<Texture2D> &p_custom_left = Ref<Texture2D>(), 
              const Ref<Texture2D> &p_custom_right = Ref<Texture2D>(), 
              bool p_draw_stylebox = true);

bool is_slot_enabled_left(int p_slot_index) const;
void set_slot_enabled_left(int p_slot_index, bool p_enable);

void set_slot_type_left(int p_slot_index, int p_type);
int get_slot_type_left(int p_slot_index) const;

void set_slot_color_left(int p_slot_index, const Color &p_color);
Color get_slot_color_left(int p_slot_index) const;

// New
void set_slot_custom_icon_left(int p_slot_index, const Ref<Texture2D> &p_custom_icon);
Ref<Texture2D> get_slot_custom_icon_left(int p_slot_index) const;

bool is_slot_enabled_right(int p_slot_index) const;
void set_slot_enabled_right(int p_slot_index, bool p_enable);

void set_slot_type_right(int p_slot_index, int p_type);
int get_slot_type_right(int p_slot_index) const;

void set_slot_color_right(int p_slot_index, const Color &p_color);
Color get_slot_color_right(int p_slot_index) const;

// New
void set_slot_custom_icon_right(int p_slot_index, const Ref<Texture2D> &p_custom_icon);
Ref<Texture2D> get_slot_custom_icon_right(int p_slot_index) const;

bool is_slot_draw_stylebox(int p_slot_index) const;
void set_slot_draw_stylebox(int p_slot_index, bool p_enable);

@ze2j
Copy link
Contributor

ze2j commented Oct 2, 2023

I have somewhere a working implementation for 4.1 and 4.0 if you want to backport this. Let me known if you are interested in.

@4d49
Copy link
Contributor Author

4d49 commented Oct 3, 2023

Is there a proposal?

Is it really necessary to create a new proposal here? I actually only created methods for the existing functionality. It is obvious that these methods were simply forgotten to be added before. The comment of the person above confirms it.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2023

No, I think the justification in that comment makes sense. I'm not that familiar with GraphNode API.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Makes sense and looks good :)

@AThousandShips AThousandShips changed the title Add set_slot_custom_icon & get_slot_custom_icon Add set_slot_custom_icon & get_slot_custom_icon to GraphNode Jan 2, 2024
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.

Looks good, please update your commit message to be more clear, the title of the PR would be sufficient (it wasn't clear from a glance what thing these were added to)

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 2, 2024
@4d49
Copy link
Contributor Author

4d49 commented Jan 4, 2024

Looks good, please update your commit message to be more clear, the title of the PR would be sufficient (it wasn't clear from a glance what thing these were added to)

@AThousandShips did I do the right thing?

@AThousandShips
Copy link
Member

The commit message hasn't been changed, you need to git commit --amend and then git push -f

@4d49 4d49 force-pushed the graph-node-slot-custom-icon branch from 64befa9 to 1b10076 Compare January 4, 2024 12:06
@4d49
Copy link
Contributor Author

4d49 commented Jan 4, 2024

The commit message hasn't been changed, you need to git commit --amend and then git push -f

@AThousandShips i'm sorry. Now it's right?

@AThousandShips
Copy link
Member

No it should be Add set_slot_custom_icon & get_slot_custom_icon to GraphNode 🙂

@4d49 4d49 force-pushed the graph-node-slot-custom-icon branch from 1b10076 to 2881219 Compare January 8, 2024 06:23
@4d49
Copy link
Contributor Author

4d49 commented Jan 8, 2024

No it should be Add set_slot_custom_icon & get_slot_custom_icon to GraphNode 🙂

@AThousandShips sorry. And now?

@akien-mga
Copy link
Member

It seems like it's still a bit off, you modified the description of the commit, instead of its title (the first line if in a plain text editor), so it looks like this:

image

I'll fix it up for you :)

@akien-mga akien-mga changed the title Add set_slot_custom_icon & get_slot_custom_icon to GraphNode Add set_slot_custom_icon and get_slot_custom_icon to GraphNode Jan 8, 2024
@akien-mga akien-mga force-pushed the graph-node-slot-custom-icon branch from 2881219 to 631d722 Compare January 8, 2024 07:30
@4d49
Copy link
Contributor Author

4d49 commented Jan 8, 2024

I'll fix it up for you :)

@akien-mga thank you😭

@akien-mga akien-mga merged commit df29fc9 into godotengine:master Jan 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@4d49 4d49 deleted the graph-node-slot-custom-icon branch January 8, 2024 14:02
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.

6 participants