yasirkula / UnityIngameDebugConsole

A uGUI based console to see debug messages and execute commands during gameplay in Unity
MIT License
2.05k stars 217 forks source link

Added console width resize option, and updated resize icon and to reflect the feature #53

Closed EricBatlle closed 3 years ago

EricBatlle commented 3 years ago

Following https://github.com/yasirkula/UnityIngameDebugConsole/pull/52 I've done the specified modifications, hope everything is okey this time 👍

EricBatlle commented 3 years ago

Also I've been thinking that would be logical to swap between the two Resize icons depending of the Resize type, so I'll tell you in that comment how would I implement it, so you can tell me if it seems correct or if you would do it differently, to avoid another error-prone PR:

I'll add three new variables on DebugLogManager.cs:

And one method calledpublic void SetResizeIcon():

public void SetResizeIcon()
{            
    resizeIconImage.sprite = enableHorizontalResizing ? resizeIcon : verticalResizeIcon;          
}

Finally I'll modify DebugLogManagerEditor.cs to have a private reference to itself: private DebugLogManager manager; That will be assigned in OnInspectorGUI like that: manager = (DebugLogManager)target; to be called on the same method when enableHorizontalResizing PropertyField is set so it will look something like:

public override void OnInspectorGUI()
{
    serializedObject.Update();

    manager = (DebugLogManager)target;
    //...
    EditorGUILayout.PropertyField(enableHorizontalResizing);

    manager.SetResizeIcon();
    //...
}

Tell me what do you think about it and if you like it I'll commit those changes to this same PR.

yasirkula commented 3 years ago

I'd change the resize icon in DebugLogManager's Awake function and also in a new DebugLogManager.OnValidate function as follows:

#if UNITY_EDITOR
private void OnValidate()
{
    if( UnityEditor.EditorApplication.isPlaying )
        resizeIconImage.sprite = enableHorizontalResizing ? resizeIcon : verticalResizeIcon;
}
#endif

This PR looks good. After you update the new variables' tooltip texts, I'm ready to merge it.

EricBatlle commented 3 years ago

@yasirkula Done!