Add function to enlarge Configure Dialog
Needs ReviewPublic

Authored by akhilkgangadharan on Jan 9 2019, 10:27 AM.

Details

Reviewers
mardelle
Summary

To resize/enlarge the existing "Configure-Kdenlive" dialog, define sizeHint() and call setMinimumSize() to enforce a minimum size dialog.
BUG: T10286

Test Plan

Build and run with patch.
Check if dialogue size persists after closing and opening.

Diff Detail

Repository
R158 Kdenlive
Branch
patch3 (branched from refactoring_timeline)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7004
Build 7022: arc lint + arc unit
akhilkgangadharan requested review of this revision.Jan 9 2019, 10:27 AM
akhilkgangadharan created this revision.
akhilkgangadharan retitled this revision from Add function to resize Configure dialog to Add function to enlarge Configure Dialog.Jan 9 2019, 10:29 AM
akhilkgangadharan edited the summary of this revision. (Show Details)
akhilkgangadharan added a reviewer: mardelle.
akhilkgangadharan added a project: Kdenlive.
akhilkgangadharan added a subscriber: emohr.

Wasn't as easy I thought it would be ;) The "Configure Kdenlive" dialog is borrowed from the Kconfig dependency so it doesn't have an ui file in the source code of Kdenlive.
Since this is a framework/dependency related "bug", I figured it would have appeared in other KDE products as well and after some hunting - I found this revision on Plasma.

I basically implemented what they came to conclusion to, in /dialogs/kdenlivesettingsdialog.cpp
And it works out successfully. We can adjust the size if we want by adjusting the parameters, but I find the dialog now perfectly sized.

2 comments there:
A hardcoded size is never good. For example on my laptop, this results in unnecessarily wide dialog. I don't see a perfect solution but would recommend this:

1 - replace the call to setMinumumSize() with a resize(). This way, user can still make it smaller if he wants.
2 - it seems better to save/restore the settings dialog size. So I would recommand to get rid of the SizeHint() method, like this:

In KdenliveSettingsDialog::updateSettings, save the dialog size like this:
KSharedConfigPtr config = KSharedConfig::openConfig();
KConfigGroup settingsGroup(config, "settings");
settingsGroup.writeEntry("dialogSize", QVariant(size()));

Then, in the dialog creation at the end, try something like:
KSharedConfigPtr config = KSharedConfig::openConfig();
KConfigGroup settingsGroup(config, "settings");
QSize optimalSize;
if (!settingsGroup.exists() || !settingsGroup.hasKey("dialogSize")) {

optimalSize = (first opening, calculate as you did in sizeHint)

} else {

optimalSize = settingsGroup.value("dialogSize").toSize();

}
resize(optimalSize);

This way, it should open with your calculated size on first opening, and remember the actual size for later openings..
What do you think?

I didn't think it out from that point of view and I feel your suggestions together make the perfect solution possible to this problem. I'll update the revision with the changes you suggested after testing them out. Thanks for the valuable comments!

emohr added a comment.Jan 10 2019, 8:42 PM

Or you detect first the screen resolution/size and with a %-variable you make the initial setting which then can be adjusted as JBM wrote.

akhilkgangadharan added a comment.EditedJan 11 2019, 1:20 PM

Or you detect first the screen resolution/size and with a %-variable you make the initial setting which then can be adjusted as JBM wrote.

Yeah, then I'll need suggestions for size of Config dialog box for each of the most common screen resolutions used, ofcourse assuming that I can do that in the first place. I'll have to scan through the documentation to check if there's anything.

akhilkgangadharan added a comment.EditedJan 11 2019, 4:36 PM

I found a potential way to do it through KWayland framework, which has a method which returns screen size. I didn't find any usage of Kwayland in our source anywhere so far, so I'm not really sure of the implementation part, what do you think @mardelle ?
There are also simple Qt classes which do the same thing.

emohr added a comment.Jan 11 2019, 4:55 PM

For Instance: If the hard coded size exceeds 80% of the screen height set it to 80%. If the width exceeds 40% of the screen width set it to 40%. But always make it possible that the user can make it smaller or bigger as JBM wrote with the possibility to store the last settings.

I don't really understand the what you are looking for. Your current patch does take into account the screen size at line 354:

QGuiApplication::primaryScreen()->availableSize() * 0.9

This defines the size of the dialog to 90% of the screen. May not be perfect in every config but good enough for a first time opening in my opinion.
We shouldn't use KWayland functions, since it is not sure the user runs a Wayland session.

emohr added a comment.Jan 11 2019, 6:37 PM

QGuiApplication::primaryScreen()->availableSize() * 0.9

yup, that's what I meant. I don't saw that.

Add functionality to allow user defined size
Allow user to resize config dialog and store and use the same value in next reopening

Remove unncessary header files and add missing header file
Accidentally removed and added one of the header files involved in the files.