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

Move AutoUpdate.CanUpdate check to resolve VisualStudio Designer Error #2407

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Move AutoUpdate.CanUpdate check to resolve VisualStudio Designer Error #2407

merged 1 commit into from
Apr 12, 2018

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Apr 11, 2018

Problem:

VisualStudio is very touchy to manual changes in the Designer, especially in InitializeComponent().
At the moment, it throws an error every time you want to open the Design View of the Settings Form, because the AutoUpdateGroupBox and its members depend on AutoUpdate.CanUpdate via an if-statement, which checks if the exe is rewriteable.

My Proposal:

Move the AutoUpdate.CanUpdate-check to InstallUpdateButton_Click instead of blocking some GUI labels and buttons. Throw an error (should it throw a kraken?), if it's false.
This would also make the SettingsWindow don't look as holey in this case.

Show full `AutoUpdateCheckbox`, even if AutoUpdate isn't avaiable to solve VisualStudio Designer Error.
Instead, check for `AutoUpdate.CanUpdate` on `InstallUpdateButton_Click` and raise error, if false.
@DasSkelett
Copy link
Member Author

DasSkelett commented Apr 11, 2018

AutoUpdate.CanUpdate was added in #2187 (#2187 (comment) / bd00633#diff-77661e48afbbc3ba9cde9b81106f683e).

@Olympic1
Copy link
Member

Wil test tonight after work but looks good at first glance.

@politas
Copy link
Member

politas commented Apr 12, 2018

Oooh, I just made my executable read-only and yes, that is rather ugly. Thanks for this PR, DasSkelett!

}
else
{
GUI.user.RaiseError("Can't autoupdate. Please check https://github.com/KSP-CKAN/CKAN/ for help!");
Copy link
Member

Choose a reason for hiding this comment

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

Would someone opening this URL be able to figure out what was going on and/or what to do about it?

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, that's a good point. Just merged this, unfortunately. Pointing to the latest release would probably be better, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oups, you were a bit faster than me.
Actually, I wanted to ask if this would work first, before detailing the error message...
Well, should I open a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

Make it point to https://github.com/KSP-CKAN/CKAN/releases/latest I think. @HebaruSan , does that seem like a useful link to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, maybe something like:
"Error during update.
Can't update automatically, because ckan.exe is read-only or we are not allowed to overwrite it. Please update manually via https://github.com/KSP-CKAN/CKAN/releases/latest ." ?

Copy link
Member

Choose a reason for hiding this comment

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

That message sounds good to me!

@politas politas merged commit c9bf1f8 into KSP-CKAN:master Apr 12, 2018
politas added a commit that referenced this pull request Apr 12, 2018
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.

5 participants