-
Notifications
You must be signed in to change notification settings - Fork 7
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
Wrong domain key logging and writing config to disk #254
base: dev
Are you sure you want to change the base?
Conversation
The config written on disk is located in an Editor folder, which means it wont appear in builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only comments/thoughts: DiskLootLockerConfig.txt is a strange name imo. I would have gone with something like PersistedLootLockerConfig
or similar personally.
Nothing that needs addressing, just a thought :D
Ah yeah, I like that, it seems that the pr isnt good so I'll fix the name too :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need a walkthrough of how this will work and look before I can approve. Also left a few comments on things I think we can improve.
#if UNITY_EDITOR | ||
if (string.IsNullOrEmpty(settingsInstance.apiKey)) | ||
{ | ||
string filePath = "Assets/LootLockerSDK/Resources/Config/Editor/PersistedLootLockerConfig.txt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're writing to disk, then why not use something like .ini that is a standard for configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that, or simply write LootLockerConfig as a serizialized json and then we can deserialize the object straight into an instance when reading?
#if UNITY_EDITOR | ||
if (string.IsNullOrEmpty(settingsInstance.apiKey)) | ||
{ | ||
string filePath = "Assets/LootLockerSDK/Resources/Config/Editor/PersistedLootLockerConfig.txt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, here you hard code the file path but below you use Application.dataPath. These risk not being the same. Please settle for one.
static void WriteConfigToDisk() | ||
{ | ||
//Check for an already existing persistent config file | ||
string fileDirectory = "Assets/LootLockerSDK/Resources/Config/Editor/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really should create these once statically I feel like and then just reuse the paths instead of typing it out like this every time.
if(!Directory.Exists(fileDirectory)) | ||
{ | ||
Directory.CreateDirectory(fileDirectory); | ||
File.Create(filePath).Close(); | ||
} | ||
else if(!File.Exists(filePath)) | ||
{ | ||
File.Create(filePath).Close(); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!Directory.Exists(fileDirectory)) | |
{ | |
Directory.CreateDirectory(fileDirectory); | |
File.Create(filePath).Close(); | |
} | |
else if(!File.Exists(filePath)) | |
{ | |
File.Create(filePath).Close(); | |
} | |
if(!Directory.Exists(fileDirectory)) | |
{ | |
Directory.CreateDirectory(fileDirectory); | |
} | |
if(!File.Exists(filePath)) | |
{ | |
File.Create(filePath).Close(); | |
} |
@@ -1,37 +1,70 @@ | |||
using System; | |||
using System.IO; | |||
using System.Linq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you really using Linq?
#if UNITY_EDITOR | ||
private void OnEnable() | ||
{ | ||
ProjectSettings.APIKeyEnteredEvent += WriteConfigToDisk; | ||
ProjectSettings.DomainKeyEnteredEvent += WriteConfigToDisk; | ||
} | ||
|
||
private void OnDisable() | ||
{ | ||
ProjectSettings.APIKeyEnteredEvent -= WriteConfigToDisk; | ||
ProjectSettings.DomainKeyEnteredEvent -= WriteConfigToDisk; | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to be taught how all this works. We have more settings than api key and domain key. Game version for example is a game breaking setting that needs to be persisted.
@@ -91,6 +95,8 @@ private void DrawGameSettings() | |||
if (EditorGUI.EndChangeCheck()) | |||
{ | |||
gameSettings.domainKey = m_CustomSettings.FindProperty("domainKey").stringValue; | |||
DomainKeyEnteredEvent?.Invoke(); | |||
m_CustomSettings.FindProperty("domainKey").stringValue = gameSettings.domainKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand m_CustomSettings will be recreated from gameSettings next frame right? So why set this?
if (match.Success) | ||
{ | ||
string domainkey = match.Value; | ||
Debug.LogWarning("You accidentally used the domain url instead of the domain key,\nWe took the domain key from the url.: " + domainkey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to be explicit about this. They should figure it out when the input changes.
The config written on disk is located in an Editor folder, which means it wont appear in builds.