Re: Review Request 119594: fix FileDialog size restorage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Sept. 5, 2014, 7:01 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Repository: frameworkintegration Description --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
On Aug. 4, 2014, 1 nachm., Lukáš Tinkl wrote: Great, this works for me :) Now what about https://git.reviewboard.kde.org/r/119593/ Sorry, I somehow missed this comment. https://git.reviewboard.kde.org/r/119593/ will make processing more robust - as long as the restore happens while there's an event loop (and likely a platform window created with it when restoring) anything's fine w/ this patch (minus the Qt workarund) This will fail if you try to restore the size before QApplication::exec() was called (or some other eventloop started) Assuming the ship it still stands (and the Qt bug is inactive, so waiting for adjustment is pointless) i'll push this tomorrow (Friday) evening at ~19:00 UTC unless veto. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/#review63770 --- On Aug. 4, 2014, 11:57 vorm., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Aug. 4, 2014, 11:57 vorm.) Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Repository: frameworkintegration Description --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Aug. 4, 2014, 11:57 vorm.) Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Changes --- Misunderstanding - I tested the dir dialog explicitly ;-) Summary (updated) - fix FileDialog size restorage Repository: frameworkintegration Description (updated) --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs (updated) - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
On Aug. 3, 2014, 11:13 nachm., Lukáš Tinkl wrote:
No change here, ie. it doesn't restore the file dialog geometry.
Thomas Lübking wrote:
what is your precise testcase?
a bit remote because of your other patches: did you check that the
correct platformtheme lib is used? (ran into this in a custom installation.
self compiled plugin ended up in a different path than the distro qt5 plugin
path)
is the size data updated in kdeglobals?
Lukáš Tinkl wrote:
Testcase: opening a filedialog in any Qt/KDE app, restarting the app -
default file/window size
I guess I'm using the correct platformtheme, otherwise I wouldn't be
seeing KDE fialogs right? Also, toplevel windows are not restored either
Yup, the size is always correctly saved to the config files
Martin Klapetek wrote:
I can also confirm that file dialogs do not have their size restored with
this patch (using tests/qfiledialogtest and also real dialogs around the
workspace), however testing the dir selection does give me properly restored
size dialog (./qfiledialogtest --staticFunction getExistingDirectory --modal
on). Qt 5.3.1 here.
Lukáš Tinkl wrote:
Because KDirSelectDialog doesn't use KWindowConfig:
void KDirSelectDialog::Private::readConfig(const KSharedConfig::Ptr
config, const QString group)
{
m_urlCombo-clear();
KConfigGroup conf(config, group);
m_urlCombo-setHistoryItems(conf.readPathEntry(History Items,
QStringList()));
const QSize size = conf.readEntry(DirSelectDialog Size, QSize());
if (size.isValid()) {
m_parent-resize(size);
}
}
Martin Klapetek wrote:
Yup, didn't know that. Disabling this^ code breaks the resizing in dir
selection too.
There're actually more issues - nevertheless both steps are (theoretically)
required.
Creating the platform window *should* apply the widget size - I suspect a
QWidget QPA bug here.
Creating the window handle, then restoring the window handle and then
explicitly copying the QWindow size should work, though.
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119594/#review63726
---
On Aug. 4, 2014, 11:57 vorm., Thomas Lübking wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119594/
---
(Updated Aug. 4, 2014, 11:57 vorm.)
Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl,
and Martin Klapetek.
Repository: frameworkintegration
Description
---
- saving in the deconstrutor is insufficient,
the dialog might survive the runtime
- needs to be saved when the dialog is finished or just closed
(the closeEvent is not invoked if at least a sync dialog is finished)
- ensure a windowHandle and then restore the window size before calling
::exec()
- workaround an apparent QWidget QPA bug where even for a created platform
window
the QWindow geometry is not applied on the QWidget
Diffs
-
src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7
src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e
src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1
src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5
Diff: https://git.reviewboard.kde.org/r/119594/diff/
Testing
---
See
https://git.reviewboard.kde.org/r/119512/
Thanks,
Thomas Lübking
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/#review63765 --- Ship it! This does fix the issue here; +1 from me Also, are you investigating/reporting the bug to Qt devs? - Martin Klapetek On Aug. 4, 2014, 1:57 p.m., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Aug. 4, 2014, 1:57 p.m.) Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Repository: frameworkintegration Description --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
On Aug. 4, 2014, 12:11 nachm., Martin Klapetek wrote: This does fix the issue here; +1 from me Also, are you investigating/reporting the bug to Qt devs? On a quick glimpse, it looks like the issue is that QWidgetWindow syncs stuff and that it syncs (only) in ::event() ie. the eventloop must be up for QWindow::setGeometry() to impact QWidget::geometry() - I fear that's out of scope for a noon break ;-) I'll try to have a more detailed look this week. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/#review63765 --- On Aug. 4, 2014, 11:57 vorm., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Aug. 4, 2014, 11:57 vorm.) Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Repository: frameworkintegration Description --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119594: fix FileDialog size restorage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/#review63770 --- Ship it! Great, this works for me :) Now what about https://git.reviewboard.kde.org/r/119593/ - Lukáš Tinkl On Srp. 4, 2014, 1:57 odp., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119594/ --- (Updated Srp. 4, 2014, 1:57 odp.) Review request for KDE Frameworks, kdelibs, Aleix Pol Gonzalez, Lukáš Tinkl, and Martin Klapetek. Repository: frameworkintegration Description --- - saving in the deconstrutor is insufficient, the dialog might survive the runtime - needs to be saved when the dialog is finished or just closed (the closeEvent is not invoked if at least a sync dialog is finished) - ensure a windowHandle and then restore the window size before calling ::exec() - workaround an apparent QWidget QPA bug where even for a created platform window the QWindow geometry is not applied on the QWidget Diffs - src/platformtheme/kdeplatformfiledialogbase.cpp b823bc7 src/platformtheme/kdeplatformfiledialogbase_p.h 8ef5b1e src/platformtheme/kdeplatformfiledialoghelper.h 406a4f1 src/platformtheme/kdeplatformfiledialoghelper.cpp 520b6f5 Diff: https://git.reviewboard.kde.org/r/119594/diff/ Testing --- See https://git.reviewboard.kde.org/r/119512/ Thanks, Thomas Lübking ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
