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

Allow paths in the installations page to be clickable #228

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

MaxIsJoe
Copy link
Contributor

A small QoL change that I've wanted for a while.

private string GetManagerProtocl()
{
string protocol = "Explorer.exe";
if (OperatingSystem.IsWindows())
Copy link
Contributor

@CorruptComputer CorruptComputer Mar 11, 2024

Choose a reason for hiding this comment

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

We have the EnvironmentService to do platform detection already, if you move this from the View to the ViewModel using a Command instead of a PointerClicked event it should be as easy as dependency injecting it in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think you need to detect the platform at all, you can just do it the same way we open log directory after a scan fail, it should open in whatever the user has specified as their default handler for opening directories.

https://github.com/unitystation/stationhub/blob/develop/UnitystationLauncher/Services/InstallationService.cs#L550

ProcessStartInfo psi = new()
{
    FileName = path,
    UseShellExecute = true,
    Verb = "open"
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work.

Unhandled exception. System.ComponentModel.Win32Exception (5): An error occurred trying to start process 'C:\Users\Maximus\AppData\Roaming\StationHub\Installations\UnityStationDevelop' with working directory 'F:\projects\stationhub\UnitystationLauncher\bin\Debug\net8.0'. Access is denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the EnvironmentService to do platform detection already, if you move this from the View to the ViewModel using a Command instead of a PointerClicked event it should be as easy as dependency injecting it in.

what difference does this make?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work.

Unhandled exception. System.ComponentModel.Win32Exception (5): An error occurred trying to start process 'C:\Users\Maximus\AppData\Roaming\StationHub\Installations\UnityStationDevelop' with working directory 'F:\projects\stationhub\UnitystationLauncher\bin\Debug\net8.0'. Access is denied.

Hmm, thats odd, I checked it out locally and that worked fine for me. Does it work when opening the log folder for a failed scan? I tested that in a Windows VM when I was working on that before and I remember it working fine, but maybe something regarding that changed with the upgrade to .NET 8.

We have the EnvironmentService to do platform detection already, if you move this from the View to the ViewModel using a Command instead of a PointerClicked event it should be as easy as dependency injecting it in.

what difference does this make?

Having standard ways of doing things makes the code a lot more maintainable, and code in the ViewModel can be effectively unit tested, while the code in the View cannot be in most cases. https://docs.avaloniaui.net/docs/concepts/the-mvvm-pattern

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

Successfully merging this pull request may close these issues.

2 participants