Skip to content

Conversation

@notargs
Copy link
Owner

@notargs notargs commented Jun 14, 2025

  • Implement GetCurrentConsoleLogs & ClearConsoleLogs
  • Remove GetLogHistory

Copilot AI review requested due to automatic review settings June 14, 2025 19:08

This comment was marked as outdated.

notargs and others added 2 commits June 15, 2025 04:09
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@notargs notargs requested a review from Copilot June 14, 2025 19:11
@notargs notargs merged commit bdabccd into main Jun 14, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces the legacy GetLogHistory feature by introducing two new console log tools and cleaning up related artifacts.

  • Add GetCurrentConsoleLogs and ClearConsoleLogs methods for querying and clearing the Unity Editor console via reflection.
  • Remove the old LogCollector class and its init call.
  • Update documentation (README.md and README.ja.md) and add new supporting types (LogEntry, LogMessageFlags).

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
McpTools/McpUnityEditorTool.cs Added GetCurrentConsoleLogs, ClearConsoleLogs, removed GetLogHistory
McpTools/LogCollector.cs (+.meta) & .cs Removed legacy log collector implementation
McpTools/LogMessageFlags.cs (+.meta) & .cs Added new LogMessageFlags enum
McpTools/LogEntry.cs (+.meta) & .cs Added new LogEntry record
McpServerRunner.cs Removed LogCollector.Initialize() call
README.md & README.ja.md Updated tool list entries
Comments suppressed due to low confidence (2)

UnityNaturalMCPServer/Editor/McpTools/McpUnityEditorTool.cs:38

  • There's a typo in the description: "Maches all logs" should be "Matches all logs".
""(Maches all logs), \"error\", \"warning\", \"log\", \"compile-error\"(This is all you need to check for compilation errors.), \"compile-warning\"

UnityNaturalMCPServer/Editor/McpTools/McpUnityEditorTool.cs:35

  • [nitpick] There are no tests covering the new GetCurrentConsoleLogs and ClearConsoleLogs functionality. Adding unit or integration tests would help validate and prevent regressions.
public async Task<IReadOnlyList<LogEntry>> GetCurrentConsoleLogs(

Comment on lines +57 to +58
var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);
Copy link

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

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

Internal UnityEditor APIs like GetCount and GetEntryInternal are not public. Update BindingFlags to include BindingFlags.NonPublic (e.g., BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic) to reliably locate these methods.

Suggested change
var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);
var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic);

Copilot uses AI. Check for mistakes.
Assert.IsNotNull(logEntries);

var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);
Copy link

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

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

GetEntryInternal is an internal method. You should include BindingFlags.NonPublic when calling GetMethod to avoid a null result at runtime.

Suggested change
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +59
var logEntries = Type.GetType("UnityEditor.LogEntries,UnityEditor.dll");
Assert.IsNotNull(logEntries);

var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);

Copy link

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The reflection logic to get the LogEntries type and methods is duplicated in both GetCurrentConsoleLogs and ClearConsoleLogs. Consider extracting a helper to reduce code duplication.

Suggested change
var logEntries = Type.GetType("UnityEditor.LogEntries,UnityEditor.dll");
Assert.IsNotNull(logEntries);
var getCountMethod = logEntries.GetMethod("GetCount", BindingFlags.Public | BindingFlags.Static);
var getEntryInternalMethod = logEntries.GetMethod("GetEntryInternal", BindingFlags.Public | BindingFlags.Static);
var (logEntries, getCountMethod, getEntryInternalMethod) = GetLogEntriesReflectionData();
Assert.IsNotNull(logEntries);

Copilot uses AI. Check for mistakes.
@notargs notargs deleted the brush_up_get_logs branch June 15, 2025 13:27
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