Skip to content

Addressing PR comments #110

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.

Sign up for GitHub

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

Draft
Claude wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Addressing PR comments #110

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
480e060
Initial plan
Claude Mar 6, 2026
d1d6469
Clarify Type attribute explanation and remove bold formatting
Claude Mar 6, 2026
99d3583
Make Type bold only on pre-failure attribute failure
Claude Mar 6, 2026
37090a8
Fix Type tooltip and FailTime condition per PR feedback
Claude Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/gui/gsc_info_window.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,7 @@ void GscInfoWindow::fill_ui_ata_attributes(const StoragePropertyRepository& prop

model_columns.add(columns_->ata_attribute_table_columns.type);
num_tree_col = app_gtkmm_create_tree_view_column(columns_->ata_attribute_table_columns.type, *treeview,
_("Type"), _("Alarm condition is reached when normalized value becomes less than or equal to threshold. Type indicates whether it's a signal of drive's pre-failure time or just an old age."), false, true);
_("Type"), _("Indicates whether an alarm for this attribute signals imminent drive failure (pre-failure) or normal wear from drive age (old age)."), false, true);

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

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

The tooltip wording "signals imminent drive failure" is stronger than the wording used elsewhere in this table (e.g., the Flags tooltip says "if the attribute failed, the drive is failing"). Consider aligning terminology (avoid implying immediacy) so the Type tooltip doesn’t inadvertently alarm users or contradict the existing explanation text.

Suggested change
_("Type"), _("Indicates whether an alarm for this attribute signals imminent drive failure (pre-failure) or normal wear from drive age (old age)."), false, true);
_("Type"), _("Indicates whether this attribute is classified as pre-failure (related to potential drive failure) or old age (related to normal wear from drive age)."), false, true);

Copilot uses AI. Check for mistakes.

@ashaduri ashaduri Mar 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot Don't use imminent, but leave a strong statement

Copilot AI Mar 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Updated in 970836b. Changed to: "Indicates whether an alarm for this attribute signals drive failure (pre-failure) or normal wear from drive age (old age)." — removed "imminent" while keeping the direct statement about drive failure.


// Doesn't carry that much info. Advanced users can look at the flags.
// model_columns.add(attribute_table_columns.updated);
Expand Down Expand Up @@ -2098,7 +2098,9 @@ void GscInfoWindow::cell_renderer_for_ata_attributes(Gtk::CellRenderer* cr,
crt->property_weight() = Pango::WEIGHT_BOLD;
}
if (column_index == columns_->ata_attribute_table_columns.type.index()) {
if (attribute.attr_type == AtaStorageAttribute::AttributeType::Prefail) {
// Bold only when attribute has failed AND it's pre-failure type
if (attribute.when_failed != AtaStorageAttribute::FailTime::None
&& attribute.attr_type == AtaStorageAttribute::AttributeType::Prefail) {
crt->property_weight() = Pango::WEIGHT_BOLD;
Comment on lines +2101 to 2105

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

The new condition treats FailTime::Unknown as “failed” because it checks when_failed != None. That can incorrectly bold “pre-failure” when the failure status is simply unknown/unparsed (text parser initializes when_failed to Unknown). Consider explicitly checking for FailTime::Past or FailTime::Now (or excluding Unknown) so bolding matches the comment (“has failed”).

Copilot uses AI. Check for mistakes.

@ashaduri ashaduri Mar 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copilot AI Mar 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in f2b95d3. Changed the condition from when_failed != None (which incorrectly includes Unknown) to explicitly check when_failed == Past || when_failed == Now, so bold is only applied when the attribute has actually failed.

} else { // reset to default value if reloading
crt->property_weight().reset_value();
Expand Down
Loading