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

[mon_gui] Logging enable, crash fix #1878

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Jan 7, 2025

Description

Enable logging receiving in mon_gui.
Check size of rows before adding them in log_model.cpp . Without that size check, in debug the monitor crashed through an assert.

@Peguen Peguen requested a review from FlorianReimold January 7, 2025 15:01
@Peguen Peguen added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 7, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -241,7 +241,9 @@ void LogModel::insertLogs(const eCAL::pb::LogMessageList& logging_pb)
// Add new elements to the bottom
size_before = logs_.size();
int size_after = (size_before + inserted_row_count < max_entries_ ? size_before + inserted_row_count : max_entries_);
beginInsertRows(QModelIndex(), size_before, size_after - 1);
bool insert_row = size_before <= size_after - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'insert_row' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool insert_row = size_before <= size_after - 1;
;const

Copy link
Member

@FlorianReimold FlorianReimold left a comment

Choose a reason for hiding this comment

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

LGTM 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants