Show the FileMetaDataConfigurationDialog dialog only after its widget has emmitted metadataLoaded signal
AbandonedPublic

Authored by meven on Feb 23 2019, 9:00 AM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Dolphin
Summary

Fixes 389571

bug https://bugs.kde.org/show_bug.cgi?id=389571

Questions:

  • Should I add a max height to the dialog ?
  • Is the design of letting dialog choosing when to show up ok ? It introduces a sort of dependency between the InformationPanelContent and the FileMetaDataConfigurationDialog since now the diablog has a particular behavior.
  • How to run the tests ?

Before:


After

Test Plan

Open the file metadata configuration dialog with a fresh dolphinrc (right clic on the information panel -> Configure)
The whole content of the dialog is visible.

Diff Detail

Repository
R318 Dolphin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8735
Build 8753: arc lint + arc unit
meven created this revision.Feb 23 2019, 9:00 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptFeb 23 2019, 9:00 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
meven requested review of this revision.Feb 23 2019, 9:00 AM
meven edited the summary of this revision. (Show Details)Feb 23 2019, 9:19 AM
meven edited the test plan for this revision. (Show Details)
ngraham added a subscriber: ngraham.

You can run the tests with ctest while in the build directory (or the source directory, if you did an in-source build (boo!)).

The general approach seems reasonable, but just complicated by the fact that the dialog's content is provided by something in baloo-widgets. Dolphin is the only client of that, so in the long term I would support moving it back into Dolphin.

Anyway, that's up to the Dolphin folks. :) Thanks for the patch!

src/panels/information/filemetadataconfigurationdialog.cpp
37

Unrelated change.

src/panels/information/filemetadataconfigurationdialog.h
73 ↗(On Diff #52362)

Typo. :)

meven updated this revision to Diff 52381.Feb 23 2019, 2:47 PM
meven edited the summary of this revision. (Show Details)
meven edited the test plan for this revision. (Show Details)

Fix typo, add a comment

FWIW I don't think you need to add a max height since you're using SizeHint, and Qt takes care of that automatically by not allowing it to be taller than the screen is.

src/panels/information/filemetadataconfigurationdialog.cpp
85

Please use modern connect syntax for new connections

https://wiki.qt.io/New_Signal_Slot_Syntax

src/panels/information/informationpanelcontent.cpp
306

A slightly more direct approach might be to listen for the metadataLoaded signal here, and then hook it up to show() in a lambda function, if that works.

elvisangelaccio requested changes to this revision.Feb 23 2019, 7:02 PM
elvisangelaccio added a subscriber: elvisangelaccio.

Is the design of letting dialog choosing when to show up ok ?

Please don't, a QDialog is supposed to be shown by using show() or open(), it shouldn't open on its own.

This revision now requires changes to proceed.Feb 23 2019, 7:02 PM
meven updated this revision to Diff 52510.Feb 25 2019, 10:15 AM
meven marked 2 inline comments as done.

Correctly position context menu under wayland with a secondary screen

meven added a comment.EditedFeb 25 2019, 10:18 AM

Correctly position context menu under wayland with a secondary screen

I wanted to put those changes in another review.
I am not familiar arc / phabricator usage hence this error, I wish to undo it.
I am going to have a look at the documentation.

The last changes fix https://bugs.kde.org/show_bug.cgi?id=404799

meven updated this revision to Diff 52511.Feb 25 2019, 10:22 AM

Correctly position context menu under wayland with a secondary screen

Remove it manually from this revision, then create a new branch from Dolphin's git master and re-apply your changes there, and create a new revision.

meven updated this revision to Diff 53181.Mar 5 2019, 8:35 AM

Show the FileMetaDataConfigurationDialog dialog only after its widget has emmitted metadataLoaded signal

meven added a comment.Mar 5 2019, 8:41 AM

Is the design of letting dialog choosing when to show up ok ?

Please don't, a QDialog is supposed to be shown by using show() or open(), it shouldn't open on its own.

Do you have an alternative design here : The dialog must be shown only after it's content has been asynchronously loaded.

The alternative I can see is to let the caller wait to display the dialog until it is ready the display signals it is ready.

meven updated this revision to Diff 53194.Mar 5 2019, 11:23 AM
  • The information panel content shows the dialog once the dialog is ready
meven marked an inline comment as done.Mar 5 2019, 4:50 PM
meven added inline comments.
src/panels/information/informationpanelcontent.cpp
306

I have replaced this with a signal/slot to let the InformationPanelContent displays the dialog once ready.

meven marked 2 inline comments as done.Mar 5 2019, 4:51 PM
meven updated this revision to Diff 53217.Mar 5 2019, 4:58 PM
  • Use modern signal/slot syntax
meven marked an inline comment as done.Mar 5 2019, 4:58 PM
meven updated this revision to Diff 53255.Mar 6 2019, 9:06 AM
  • Use the updateGeometry signal to propagate that the config widget is ready to be shown
meven updated this revision to Diff 53275.Mar 6 2019, 2:22 PM
  • Use adjustSize to adjust the dialog size
meven updated this revision to Diff 53276.Mar 6 2019, 2:24 PM
  • Use adjustSize to adjust the dialog size
meven updated this revision to Diff 53278.Mar 6 2019, 2:25 PM
  • Use adjustSize to adjust the dialog size
meven added inline comments.Mar 6 2019, 2:25 PM
src/panels/information/filemetadataconfigurationdialog.cpp
70 ↗(On Diff #52362)

I am not entirely sure this is a good practice.

meven updated this revision to Diff 53284.Mar 6 2019, 2:46 PM
  • Rename signal loadingFinished and related slot
meven abandoned this revision.Apr 23 2019, 7:38 AM

Obsolete as of D20524