⚙ D19243 Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its metadata has been loaded, and adjust its size based on its content

meven created this revision.Feb 23 2019, 9:10 AM

Restricted Application added a project: Baloo. · View Herald TranscriptFeb 23 2019, 9:10 AM

Restricted Application added a subscriber: Baloo. · View Herald Transcript

meven requested review of this revision.Feb 23 2019, 9:10 AM

Harbormaster completed remote builds in B8723: Diff 52364.Feb 23 2019, 9:10 AM

meven added a dependent revision: D19241: Show the FileMetaDataConfigurationDialog dialog only after its widget has emmitted metadataLoaded signal.Feb 23 2019, 9:12 AM

ngraham requested changes to this revision.Feb 23 2019, 2:20 PM

ngraham added reviewers: Dolphin, Baloo.

ngraham added a subscriber: ngraham.

ngraham added inline comments.

src/filemetadataconfigwidget.cpp
190	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()

This revision now requires changes to proceed.Feb 23 2019, 2:20 PM

Use QFontMetrics to retrieve the lineheight to use.

Harbormaster completed remote builds in B8734: Diff 52379.Feb 23 2019, 2:43 PM

elvisangelaccio added a subscriber: elvisangelaccio.Feb 23 2019, 6:39 PM

elvisangelaccio added inline comments.

src/filemetadataconfigwidget.h
65	I'd call it `loadingFinished()` to match the `FileMetaDataProvider::loadingFinished` signal.

bruns added a subscriber: bruns.Feb 23 2019, 7:01 PM

bruns added inline comments.

src/filemetadataconfigwidget.cpp
191	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?

elvisangelaccio added inline comments.Feb 23 2019, 7:04 PM

src/filemetadataconfigwidget.h
65	See D19241, but I'm not convinced it's the right way to fix it. You are right that it might just be a `sizeHint` bug.

bruns added inline comments.Feb 23 2019, 7:04 PM

src/filemetadataconfigwidget.cpp
144	Probably, a https://doc.qt.io/qt-5/qwidget.html#updateGeometry is missing here.

bruns added inline comments.Feb 24 2019, 2:38 AM

src/filemetadataconfigwidget.cpp
144	Also, QListWidget / QAbstractScrollArea::setSizeAdjustPolicy() https://doc.qt.io/qt-5/qabstractscrollarea.html#public-functions

No need to call loadMetaData on Polish events when the dialog can only be shown when the metadata are loaded, this caused a issue

Harbormaster completed remote builds in B9189: Diff 53195.Mar 5 2019, 11:28 AM

. - Use m_metaDataList->sizeHintForRow for reference for a row height

Harbormaster completed remote builds in B9199: Diff 53215.Mar 5 2019, 4:55 PM

Use m_metaDataList->sizeHintForRow for reference for a row height

Harbormaster completed remote builds in B9200: Diff 53216.Mar 5 2019, 4:56 PM

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().

In D19243#425594, @bruns wrote:

Can you please try updateGeometry() instead of adding a proprietary signal?

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
190	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.

Propagate updateGeometry signal, remove an unused variable

Harbormaster completed remote builds in B9225: Diff 53256.Mar 6 2019, 9:10 AM

meven marked 4 inline comments as done.Mar 6 2019, 9:47 AM

meven added inline comments.

src/filemetadataconfigwidget.cpp
144	I am not sure this is necessary here, at least in my tests I did not have any changes.

meven marked an inline comment as done.Mar 6 2019, 9:48 AM

Propagate updateGeometry signal, remove an unused variable

Harbormaster completed remote builds in B9227: Diff 53260.Mar 6 2019, 9:50 AM

bruns added inline comments.Mar 6 2019, 12:28 PM

src/filemetadataconfigwidget.cpp
144	updateGeometry() is not a signal. See the documentation link.

Would using adjustSize on the QDialog be ok after the metadata has been loaded ?
D19241

In D19243#425813, @meven wrote:

Would using adjustSize on the QDialog be ok after the metadata has been loaded ?
D19241

Just call q->updateGeometry(). Please read the linked documentation.

Properly call updateGeometry

Harbormaster completed remote builds in B9235: Diff 53274.Mar 6 2019, 2:17 PM

meven marked 2 inline comments as done.Mar 6 2019, 2:18 PM

Rename signal loadingFinished

Harbormaster completed remote builds in B9240: Diff 53280.Mar 6 2019, 2:28 PM

meven marked 2 inline comments as done.Mar 6 2019, 2:30 PM

bruns added inline comments.Mar 6 2019, 8:15 PM

src/filemetadataconfigwidget.cpp
146	You don't need this signal when using updateGeometry().

meven marked an inline comment as done.Mar 7 2019, 7:57 AM

meven added inline comments.

src/filemetadataconfigwidget.cpp
146	I need this to let the QDialog in D19241 know it needs to adjust its size. Otherwise updateGeometry is called after the dialog is shown and the dialog is not resized accordingly.

meven marked 2 inline comments as done.Mar 7 2019, 7:57 AM

Convert connect to new syntax

Harbormaster completed remote builds in B9273: Diff 53344.Mar 7 2019, 10:01 AM

meven marked an inline comment as done.Mar 16 2019, 8:20 AM

meven added inline comments.

src/filemetadataconfigwidget.cpp
191	I wish it was so simple... https://stackoverflow.com/questions/6337589/qlistwidget-adjust-size-to-content

meven marked 2 inline comments as done.Mar 16 2019, 2:51 PM

meven mentioned this in D20525: [InformationPanel] Use the new inline configuration mode.Apr 14 2019, 9:07 AM

Obsolete as of D20524

			Path	Packages
M			src/filemetadataconfigwidget.h (4 lines)
M			src/filemetadataconfigwidget.cpp (28 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	52364	445286c		Feb 23 2019, 9:10 AM	★	★
Diff 2	52379	445286c	Use QFontMetrics to retrieve the lineheight to use.	Feb 23 2019, 2:43 PM	★	★
Diff 3	53195	22e69f4	- No need to call loadMetaData on Polish events when the dialog can only be…	Mar 5 2019, 11:28 AM	★	★
Diff 4	53215	22e69f4	. - Use m_metaDataList->sizeHintForRow for reference for a row height	Mar 5 2019, 4:55 PM	★	★
Diff 5	53216	22e69f4	- Use m_metaDataList->sizeHintForRow for reference for a row height	Mar 5 2019, 4:56 PM	★	★
Diff 6	53256	22e69f4	- Propagate updateGeometry signal, remove an unused variable	Mar 6 2019, 9:10 AM	★	★
Diff 7	53260	22e69f4	- Propagate updateGeometry signal, remove an unused variable	Mar 6 2019, 9:50 AM	★	★
Diff 8	53274	22e69f4	- Properly call updateGeometry	Mar 6 2019, 2:17 PM	★	★
Diff 9	53280	22e69f4	- Rename signal loadingFinished	Mar 6 2019, 2:28 PM	★	★
Diff 10	53344	22e69f4	- Convert connect to new syntax	Mar 7 2019, 10:01 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
9ea112942b07	c70795ab5b70	ef8d38a056a9	Meven	Properly call updateGeometry	Mar 6 2019, 2:17 PM
ef8d38a056a9	2494e0f00d3d	c9d9fbf5000e	Meven	Propagate updateGeometry signal, remove an unused variable	Mar 6 2019, 9:10 AM
c9d9fbf5000e	578a9efd1fee	419b728eb4a1	Meven	Use m_metaDataList->sizeHintForRow for reference for a row height	Mar 5 2019, 4:55 PM
419b728eb4a1	62973e645125	2e79c0284d6c	Meven	No need to call loadMetaData on Polish events when the dialog can only be shown… (Show More…)	Mar 5 2019, 11:27 AM
2e79c0284d6c	2a5f2be3e48f	22e69f404100	Meven	Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its… (Show More…)	Feb 23 2019, 9:10 AM

	Status	Author	Revision
	Abandoned	meven	D19241 Show the FileMetaDataConfigurationDialog dialog only after its widget has emmitted metadataLoaded signal
	Abandoned	meven	D19243 Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its metadata has been loaded, and adjust its size based on its content

Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its metadata has been loaded, and adjust its size based on its content
AbandonedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 53274

src/filemetadataconfigwidget.h

src/filemetadataconfigwidget.cpp

Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its metadata has been loaded, and adjust its size based on its contentAbandonedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 53274

src/filemetadataconfigwidget.h

src/filemetadataconfigwidget.cpp

Allow the FileMetaDataConfigWidget to emit a loadedMetadata signal when its metadata has been loaded, and adjust its size based on its content
AbandonedPublic
Actions

Revision Contents
Changeset List