Skip to content

Conversation

HTRamsey
Copy link
Collaborator

No description provided.

@HTRamsey HTRamsey requested a review from Copilot September 16, 2025 11:01
Copy link
Contributor

@Copilot 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 provides a comprehensive cleanup and improvement of the QGCApplication class, focusing on modernizing C++ practices, improving thread safety, and enhancing memory management. The changes include better resource management using smart pointers, improved thread-safe design patterns, and more robust error handling throughout the application initialization sequence.

  • Modernized C++ patterns with smart pointers, atomic variables, and RAII
  • Enhanced thread safety with proper mutex usage and atomic initialization tracking
  • Improved error handling and logging throughout the application lifecycle

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/QGCApplication.h Major refactoring with smart pointers, thread-safe members, and improved API design
src/QGCApplication.cc Complete rewrite of implementation with modern C++ practices and better error handling
src/FactSystem/ParameterManager.cc Minor fix to parameter cache directory handling
src/Comms/TCPLink.cc Corrected logging category name from test to qgc namespace
src/Comms/LinkManager.h Added serial permissions check function declaration
src/Comms/LinkManager.cc Moved ZeroConf code and added serial permissions validation
Comments suppressed due to low confidence (1)

src/QGCApplication.cc:1

  • This function is defined in QGCApplication.cc but belongs to LinkManager class. This will cause linking errors as the function should be implemented in LinkManager.cc where it's properly scoped to the LinkManager class.
/****************************************************************************

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@HTRamsey HTRamsey force-pushed the dev-qgcapp-cleanup branch 2 times, most recently from 33032a6 to a1482e4 Compare September 16, 2025 12:55
@HTRamsey HTRamsey requested a review from Copilot September 16, 2025 13:51
Copy link
Contributor

@Copilot 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@HTRamsey HTRamsey requested a review from Copilot September 16, 2025 17:37
Copy link
Contributor

@Copilot 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

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

Comments suppressed due to low confidence (2)

src/QGCApplication.cc:1

  • Typo in the log message: 'SericeType' should be 'ServiceType' (missing 'v').
/****************************************************************************

src/QGCApplication.cc:1

  • Multiple spelling errors in the comment: 'doesnt' should be 'doesn't' (missing apostrophe) and 'trailling' should be 'trailing' (extra 'l').
/****************************************************************************

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@HTRamsey HTRamsey force-pushed the dev-qgcapp-cleanup branch 3 times, most recently from 7b0efc7 to 6096aa3 Compare September 25, 2025 06:14
@HTRamsey HTRamsey requested a review from Copilot September 25, 2025 06:50
Copy link
Contributor

@Copilot 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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/Utilities/Platform.cc:1

  • QProcess is used on lines 255-267 but the include is missing. This will cause compilation errors.
/****************************************************************************

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// We don't want unit tests to use the same QSettings space as the normal app. So we tweak the app
// name. Also we want to run unit tests with clean settings every time.
applicationName = QStringLiteral("%1_unittest").arg(QGC_APP_NAME);
applicationName = QLatin1String("%1_unittest").arg(QLatin1String(QGC_APP_NAME));
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

QLatin1String does not have an arg() method. Use QString instead or convert to QString first before calling arg().

Suggested change
applicationName = QLatin1String("%1_unittest").arg(QLatin1String(QGC_APP_NAME));
applicationName = QString("%1_unittest").arg(QGC_APP_NAME);

Copilot uses AI. Check for mistakes.

Comment on lines 87 to 92
applicationName = QLatin1String("%1_unittest").arg(QLatin1String(QGC_APP_NAME));
} else {
#ifdef QGC_DAILY_BUILD
// This gives daily builds their own separate settings space. Allowing you to use daily and stable builds
// side by side without daily screwing up your stable settings.
applicationName = QStringLiteral("%1 Daily").arg(QGC_APP_NAME);
applicationName = QLatin1String("%1 Daily").arg(QLatin1String(QGC_APP_NAME));
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

QLatin1String does not have an arg() method. Use QString instead or convert to QString first before calling arg().

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant