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.

Sign up for GitHub

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

Jump to bottom

Update the RiderPathLocator to support the JetBrains Toolbox 2.0 #78832

Merged
merged 1 commit into from
Jul 8, 2023
Merged

Update the RiderPathLocator to support the JetBrains Toolbox 2.0 #78832

merged 1 commit into from
Jul 8, 2023

Conversation

van800
Copy link
Contributor

@van800 van800 commented Jun 29, 2023 •
edited by akien-mga
Loading

@van800 van800 requested a review from a team as a code owner June 29, 2023 11:39
@akien-mga akien-mga added enhancement topic:editor topic:dotnet cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jun 29, 2023
@akien-mga akien-mga added this to the 4.2 milestone Jun 29, 2023
raulsntos
raulsntos requested changes Jun 29, 2023
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for updating the JetBrains Rider support for Godot. In general, I think these changes look good.

These changes seem to add a lot of abstraction that doesn't seem necessary, I assume this is to reuse code outside of Godot. Have you considered releasing a library in NuGet that could be reused instead? It seems like it would be less work than copying code over, then the code could live in a JetBrains repository and wouldn't be subject to our code style.

modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathLocator.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs Outdated Show resolved Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs Outdated Show resolved Hide resolved
@van800
Copy link
Contributor Author

van800 commented Jun 30, 2023 •
edited
Loading

I will try it with a nuget package. Thanks!

@van800 van800 force-pushed the path_locator_4x branch 3 times, most recently from 363c23a to 3e933d9 Compare June 30, 2023 14:23
@van800 van800 requested a review from raulsntos June 30, 2023 14:28
@van800 van800 force-pushed the path_locator_4x branch from 3e933d9 to ed2734e Compare July 2, 2023 07:04
@van800 van800 force-pushed the path_locator_4x branch from ed2734e to bf3af9f Compare July 2, 2023 18:55
@van800 van800 requested a review from raulsntos July 2, 2023 19:35
raulsntos
raulsntos approved these changes Jul 2, 2023
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

@van800 van800 mentioned this pull request Jul 6, 2023
Closed
@akien-mga akien-mga merged commit 4a3c662 into godotengine:master Jul 8, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 10, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

@geowarin
Copy link
Contributor

@van800 This is selecting Fleet over Rider (EAP) for me.
On linux, toolbox is installed via AUR (https://aur.archlinux.org/packages/jetbrains-toolbox).

I had to uninstall fleet to solve the problem

@van800
Copy link
Contributor Author

van800 commented Jul 13, 2023

I will look into it, thanks.

@van800
Copy link
Contributor Author

van800 commented Jul 18, 2023

@geowarin I have another change with also fix for that case, which you had. But I want to test it on MacOSX first, it may take a while.

van800 added a commit to van800/godot that referenced this pull request Jul 27, 2023
@van800
support Rider and Fleet open file args, fix godotengine#78832 (comment)
6ec0e03
@van800 van800 mentioned this pull request Jul 27, 2023
Merged
van800 added a commit to van800/godot that referenced this pull request Aug 4, 2023
@van800
Delegate opening files for Rider to the RiderPathLocator NuGet package,
7f8e3ab 
fix godotengine#78832 (comment)
IntangibleMatter pushed a commit to IntangibleMatter/godot that referenced this pull request Aug 13, 2023
@van800 @IntangibleMatter
Delegate opening files for Rider to the RiderPathLocator NuGet package,
39c3292 
fix godotengine#78832 (comment)
mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
@van800 @mandryskowski
Delegate opening files for Rider to the RiderPathLocator NuGet package,
1e14631 
fix godotengine#78832 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raulsntos raulsntos raulsntos approved these changes

Assignees
No one assigned
Labels
enhancement topic:dotnet topic:editor
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@van800 @akien-mga @YuriSizov @geowarin @raulsntos