Diff Detail
- Repository
- R824 Baloo Widgets
- Branch
- show-dialog
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9189 Build 9207: arc lint + arc unit
src/filemetadataconfigwidget.cpp | ||
---|---|---|
192 | Better to read the actual text metrics rather than assuming a height. Something like this: const QFontmetrics metrics(d->m_metaDataList->font()) const int lineHeight = metrics.height() |
src/filemetadataconfigwidget.h | ||
---|---|---|
65 | I'd call it loadingFinished() to match the FileMetaDataProvider::loadingFinished signal. |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
193 | This feels like papering over a different issue - the QListWidget should provide an appropriate sizeHint, as it knows its items, and each list item should provide a correct size hint. | |
src/filemetadataconfigwidget.h | ||
65 | The question is - who is going to receive this signal? |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
147 | Probably, a https://doc.qt.io/qt-5/qwidget.html#updateGeometry is missing here. |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
147 | Also, QListWidget / QAbstractScrollArea::setSizeAdjustPolicy() |
- No need to call loadMetaData on Polish events when the dialog can only be shown when the metadata are loaded, this caused a issue
Can you please try updateGeometry() instead of adding a proprietary signal?
The custom sizeHint() code indeed seems to be necessary, as although the scroll area view-container calculates the content size (otherwise the scrollbars would not work) it does not propagate it to its own sizeHint().
Of course, no need to implore me, I learn as I go thanks to the review process.
I missed the first mention of updateGeometry.
Thanks for pointing it out again :)
The custom sizeHint() code indeed seems to be necessary, as although the scroll area view-container calculates the content size (otherwise the scrollbars would not work) it does not propagate it to its own sizeHint().
According to my testing the original sizeHint() returns a constant regardless of its content.
src/filemetadataconfigwidget.cpp | ||
---|---|---|
192 | Thanks, I am not too familiar with the Kde/Qt API, this was a question in the related change https://phabricator.kde.org/D19241 But the suggested line height is too low. |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
147 | I am not sure this is necessary here, at least in my tests I did not have any changes. |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
147 | updateGeometry() is not a signal. See the documentation link. |
Would using adjustSize on the QDialog be ok after the metadata has been loaded ?
D19241
src/filemetadataconfigwidget.cpp | ||
---|---|---|
149 | You don't need this signal when using updateGeometry(). |
src/filemetadataconfigwidget.cpp | ||
---|---|---|
193 | I wish it was so simple... |