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

Saving Settings and Reload Script #130

Merged
merged 11 commits into from
Apr 19, 2018
Merged

Conversation

Zonz
Copy link
Contributor

@Zonz Zonz commented Apr 8, 2018

No description provided.


if (!string.IsNullOrEmpty(UserSettings.LastScript))
{
MenuReloadScript.Header = "Reload " + Path.GetFileName(UserSettings.LastScript) + "\tCtrl+R";
Copy link
Owner

Choose a reason for hiding this comment

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

You can probably use InputGestureText="Ctrl+R" in the XAML instead of adding the string manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I didn't know about that.


namespace PROBot
{
public static class UserSettings
Copy link
Owner

Choose a reason for hiding this comment

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

The class doesn't need to be static, you can instantiate it in BotClient or in the view.
That way we can instantiate multiple clients with different settings if we want to.

string fileText = File.ReadAllText("Settings.json");
JObject json = JsonConvert.DeserializeObject(fileText) as JObject;
_settings = JsonConvert.DeserializeObject<SettingsCache>(json.ToString());
return;
Copy link
Owner

Choose a reason for hiding this comment

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

You can remove the return here and move _settings = new SettingsCache(); inside the catch {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to create a new setting if the file does not exist, that's why I did it that way.

Copy link
Owner

Choose a reason for hiding this comment

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

If you move the new settings creation inside the catch, the behaviour stays the same:

// ...
catch
{
    _settings = new SettingsCache();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you delete the Settings file and try it again, you'll see that _settings will be null if you do that.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, indeed, I forgot about the if. We can keep it like this then.

@Silv3rPRO
Copy link
Owner

I added some comments, but it looks good overall. 👍

@Silv3rPRO Silv3rPRO merged commit 730b97f into Silv3rPRO:master Apr 19, 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.

2 participants