Changeset View
Standalone View
src/views/tooltips/tooltipmanager.cpp
| Show First 20 Lines • Show All 76 Lines • ▼ Show 20 Line(s) | 72 | { | |||
|---|---|---|---|---|---|
| 77 | m_itemRect.adjust(-m_margin, -m_margin, m_margin, m_margin); | 77 | m_itemRect.adjust(-m_margin, -m_margin, m_margin, m_margin); | ||
| 78 | m_item = item; | 78 | m_item = item; | ||
| 79 | 79 | | |||
| 80 | m_transientParent = transientParent; | 80 | m_transientParent = transientParent; | ||
| 81 | 81 | | |||
| 82 | // Only start the retrieving of the content, when the mouse has been over this | 82 | // Only start the retrieving of the content, when the mouse has been over this | ||
| 83 | // item for 200 milliseconds. This prevents a lot of useless preview jobs and | 83 | // item for 200 milliseconds. This prevents a lot of useless preview jobs and | ||
| 84 | // meta data retrieval, when passing rapidly over a lot of items. | 84 | // meta data retrieval, when passing rapidly over a lot of items. | ||
| 85 | delete m_fileMetaDataWidget; | 85 | m_fileMetaDataWidget.reset(new DolphinFileMetaDataWidget()); | ||
| 86 | m_fileMetaDataWidget = new DolphinFileMetaDataWidget(); | 86 | connect(m_fileMetaDataWidget.data(), &DolphinFileMetaDataWidget::metaDataRequestFinished, | ||
| 87 | connect(m_fileMetaDataWidget, &DolphinFileMetaDataWidget::metaDataRequestFinished, | | |||
| 88 | this, &ToolTipManager::slotMetaDataRequestFinished); | 87 | this, &ToolTipManager::slotMetaDataRequestFinished); | ||
elvisangelaccio: Why `reset()` it twice? The second `reset()` call should be enough | |||||
I was just trying to mimic the original behavior. If there is only a single reset then there will be a short period of time where two DolphinFileMetaDataWidget instances are allocated because first the new DolphinFileMetaDataWidget is allocated, then it it passed to the reset function, which deletes the instance it owns and keeps the new one. A standard way of avoiding this is simply to call reset first without any arguments, then the original instance is deleted before the new one is allocated. But I can change it to a single reset call, what do you think? hallas: I was just trying to mimic the original behavior. If there is only a single reset then there… | |||||
Hmm, this feels like a theoretical problem only. IMHO readability and consistency of code should be preferred (we don't reset a QScopedPointer like this anywhere else in dolphin). elvisangelaccio: >If there is only a single reset then there will be a short period of time where two… | |||||
hallas: I have removed the extra `reset` to keep things consistent :) | |||||
| 89 | connect(m_fileMetaDataWidget, &DolphinFileMetaDataWidget::urlActivated, | 88 | connect(m_fileMetaDataWidget.data(), &DolphinFileMetaDataWidget::urlActivated, | ||
QScopedPointer::get() is Qt 5.11+ only, please use QScopedPointer::data() instead. elvisangelaccio: `QScopedPointer::get()` is Qt 5.11+ only, please use `QScopedPointer::data()` instead. | |||||
hallas: Good catch ;) I have changed this to `QScopedPointer::data()` | |||||
| 90 | this, &ToolTipManager::urlActivated); | 89 | this, &ToolTipManager::urlActivated); | ||
| 91 | 90 | | |||
| 92 | m_contentRetrievalTimer->start(); | 91 | m_contentRetrievalTimer->start(); | ||
| 93 | m_showToolTipTimer->start(); | 92 | m_showToolTipTimer->start(); | ||
| 94 | m_toolTipRequested = true; | 93 | m_toolTipRequested = true; | ||
| 95 | Q_ASSERT(!m_metaDataRequested); | 94 | Q_ASSERT(!m_metaDataRequested); | ||
| 96 | } | 95 | } | ||
| 97 | 96 | | |||
| ▲ Show 20 Lines • Show All 106 Lines • ▼ Show 20 Line(s) | 199 | if (m_fileMetaDataWidget->preview().isNull() || m_metaDataRequested) { | |||
| 204 | return; | 203 | return; | ||
| 205 | } | 204 | } | ||
| 206 | 205 | | |||
| 207 | // Adjust the size to get a proper sizeHint() | 206 | // Adjust the size to get a proper sizeHint() | ||
| 208 | m_fileMetaDataWidget->adjustSize(); | 207 | m_fileMetaDataWidget->adjustSize(); | ||
| 209 | if (!m_tooltipWidget) { | 208 | if (!m_tooltipWidget) { | ||
| 210 | m_tooltipWidget.reset(new KToolTipWidget()); | 209 | m_tooltipWidget.reset(new KToolTipWidget()); | ||
| 211 | } | 210 | } | ||
| 212 | m_tooltipWidget->showBelow(m_itemRect, m_fileMetaDataWidget, m_transientParent); | 211 | m_tooltipWidget->showBelow(m_itemRect, m_fileMetaDataWidget.data(), m_transientParent); | ||
elvisangelaccio: What about this one? ;) | |||||
hallas: Sorry about that, fixed now ;) | |||||
| 213 | m_toolTipRequested = false; | 212 | m_toolTipRequested = false; | ||
| 214 | } | 213 | } | ||
| 215 | 214 | | |||
Why reset() it twice? The second reset() call should be enough