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

Show override icon in script editor gutter #65535

Merged
merged 2 commits into from
Sep 25, 2022

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Sep 8, 2022

Part of godotengine/godot-proposals#5369

Includes two new icons for the script editor gutter, one for overridden method and a second for combining with the "connected signal" icon:

image

The icon can be clicked to jump to the method declaration (or in case of a built-in class, it opens the corresponding documentation page).

I also snuck another commit in to optimize an icon from a previous PR 👀

@RedMser RedMser requested a review from a team as a code owner September 8, 2022 19:04
@YeldhamDev YeldhamDev added this to the 4.1 milestone Sep 8, 2022
@MewPurPur
Copy link
Contributor

For completeness, #64693 implements the same as a highlighting color

@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 8, 2022

Anyway, I'm looking forward to these being discernible! I think an icon along the lines of an arrow forking out would be more intuitive. Not a svg extraordinaire, so here's a pixel art version of what I have in mind:

image

Combo:

image

*Edited to reflect Yeldham's comment

@YeldhamDev
Copy link
Member

YeldhamDev commented Sep 8, 2022

The override color shouldn't be the same as the connection one, so they can clearly stand out from each other. The combo one can have the arrow keep the override color and the box the connection color.

@RedMser RedMser force-pushed the script-editor-inheritance-icon branch from 02cab21 to 573741a Compare September 9, 2022 14:32
@RedMser RedMser requested a review from a team as a code owner September 9, 2022 14:32
@RedMser
Copy link
Contributor Author

RedMser commented Sep 9, 2022

Thanks for the icon templates @MewPurPur, I went ahead and created SVG versions based on these.

@RedMser RedMser force-pushed the script-editor-inheritance-icon branch from 573741a to fb8845c Compare September 9, 2022 14:36
@YuriSizov YuriSizov modified the milestones: 4.1, 4.x Sep 9, 2022
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

This looks good!

In other IDE's clicking the "override" icon will take you to the parent method would be cool to do that here. In which case might be worth changing the metadata from a string to a dictionary?

@RedMser RedMser force-pushed the script-editor-inheritance-icon branch from fb8845c to 11d9a29 Compare September 20, 2022 14:25
@RedMser RedMser requested a review from a team as a code owner September 20, 2022 14:25
@RedMser
Copy link
Contributor Author

RedMser commented Sep 20, 2022

@Paulb23 Good point.
I added the feature, so it jumps to the method declaration when clicking on the icon. For built-in classes, it jumps to the corresponding documentation page instead. Uses dictionaries like suggested.

For the "combined" signal+override case, I think it would be nice to somehow show both icon's information (e.g. open a PopupMenu which allows choosing whether to show the dialog or the declaration). But I think it's a rare enough case that we can ignore this for now. Until more gutter icons get added and a real system needs to be in place for them.

@RedMser RedMser force-pushed the script-editor-inheritance-icon branch from 11d9a29 to b1d99b8 Compare September 20, 2022 14:30
Combines with the connection slot icon when both apply.
Can be clicked to jump to the method declaration (or documentation for
built-in classes).
@RedMser RedMser force-pushed the script-editor-inheritance-icon branch from b1d99b8 to a9b394d Compare September 20, 2022 23:50
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

LGTM.

The otherway around it would be to have ctrl+click goto the method similar to normal lookup. While, a normal click brings up the connection popup.

@akien-mga akien-mga modified the milestones: 4.x, 4.0 Sep 25, 2022
Copy link
Member

@akien-mga akien-mga 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 to me.

Initial response to this PR seems to be positive, I'm not 100% sure all users will love the change as it's quite intrusive, but this kind of UX might be best tested in production to get feedback in 4.0 beta 2.

@akien-mga akien-mga merged commit d456dce into godotengine:master Sep 25, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for snatching PR 0xFFFF!

@fracteed
Copy link

fracteed commented Oct 3, 2022

Is there any way of hiding these new icons? It seems that I can hide the whole info gutter, but I would still like to have the signal icons visible.

@RedMser
Copy link
Contributor Author

RedMser commented Oct 3, 2022

@fracteed There currently is no way, but I don't see anything that speaks against adding some more fine-grained editor settings to choose which gutter icons should show.

@fracteed
Copy link

fracteed commented Oct 4, 2022

@RedMser thanks for your reply. Yes, it would be great to have per icon toggles for the gutter in the same way that we have per item gizmo visibility in the viewport. Especially if more icons end up being added over time.

Ideally this would be a menu in the text editor itself rather than having to dive into preferences. I can certainly see their usefulness, but they are not something that I need to see all the time.

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.

7 participants