Changeset View
Standalone View
plugins/grepview/grepoutputmodel.cpp
| Show First 20 Lines • Show All 143 Lines • ▼ Show 20 Line(s) | 96 | { | |||
|---|---|---|---|---|---|
| 144 | } | 144 | } | ||
| 145 | } | 145 | } | ||
| 146 | 146 | | |||
| 147 | QVariant GrepOutputItem::data ( int role ) const { | 147 | QVariant GrepOutputItem::data ( int role ) const { | ||
| 148 | GrepOutputModel *grepModel = static_cast<GrepOutputModel *>(model()); | 148 | GrepOutputModel *grepModel = static_cast<GrepOutputModel *>(model()); | ||
| 149 | if(role == Qt::ToolTipRole && grepModel && isText()) | 149 | if(role == Qt::ToolTipRole && grepModel && isText()) | ||
| 150 | { | 150 | { | ||
| 151 | QString start = text().left(m_change->m_range.start().column()).toHtmlEscaped(); | 151 | QString start = text().left(m_change->m_range.start().column()).toHtmlEscaped(); | ||
| 152 | QString repl = "<b>" + grepModel->replacementFor(m_change->m_oldText).toHtmlEscaped() + "</b>"; | 152 | // show replaced version in tooltip if we are in replace mode | ||
| 153 | const QString match = isCheckable() ? grepModel->replacementFor(m_change->m_oldText) : m_change->m_oldText; | ||||
kfunk: Shouldn't this be `isChecked()` (doesn't exist, I know).
What exactly does this try to fix if… | |||||
The Grep toolview has two modes: Find-only mode and Replace mode. The Find-only mode is the default mode and entered on doing the search. In the Replace mode all items get changed to isCheckable = true, resulting in all items being shown with a checkbox, so one can select by toggling the checkboxes which of the found items will be included when doing the actual replacement. So the isCheckable state reflects if in Find-only mode or Replace mode. The isChecked state only reflects if an item should be included in the replacement, it does not hint to the mode. The bug is this: Any proposal how to make that more clear in the in-code comment? kossebau: The Grep toolview has two modes: Find-only mode and Replace mode.
The Find-only mode is the… | |||||
Okay, understood, that's what I /thought/ as well. My complaint is that using isCheckable() isn't going far enough. It should be isCheckable() && isChecked() -- since it only makes sense to show the to-be-replaced text when we actually want to replace it (IOW: when it is checked). Anyhow, much better then before already: so go for it. Thanks! kfunk: Okay, understood, that's what I /thought/ as well.
My complaint is that using `isCheckable()`… | |||||
Okay, now got you. Hm, not sure if I agree, I would expect to always see the potential replaced text when in replacement mode. Will give that some observation with myself for a while to see if real world usage matches what I assume now :) kossebau: Okay, now got you. Hm, not sure if I agree, I would expect to always see the potential replaced… | |||||
| 154 | const QString repl = QLatin1String("<b>") + match.toHtmlEscaped() + QLatin1String("</b>"); | ||||
| 153 | QString end = text().right(text().length() - m_change->m_range.end().column()).toHtmlEscaped(); | 155 | QString end = text().right(text().length() - m_change->m_range.end().column()).toHtmlEscaped(); | ||
| 154 | return QVariant(QString(start + repl + end).trimmed()); | 156 | return QVariant(QString(start + repl + end).trimmed()); | ||
| 155 | } else if (role == Qt::FontRole) { | 157 | } else if (role == Qt::FontRole) { | ||
| 156 | return QFontDatabase::systemFont(QFontDatabase::FixedFont); | 158 | return QFontDatabase::systemFont(QFontDatabase::FixedFont); | ||
| 157 | } else { | 159 | } else { | ||
| 158 | return QStandardItem::data(role); | 160 | return QStandardItem::data(role); | ||
| 159 | } | 161 | } | ||
| 160 | } | 162 | } | ||
| ▲ Show 20 Lines • Show All 332 Lines • Show Last 20 Lines | |||||
Shouldn't this be isChecked() (doesn't exist, I know).
What exactly does this try to fix if not?