Store arguments before creating QApplication
ClosedPublic

Authored by broulik on Dec 19 2016, 6:43 AM.

Details

Summary

Qt removes certain arguments, such as title and icon, from argv before our QCommandLineParser gets a chance to parse them.

BUG: 373677

Test Plan
kdialog --passivepopup "Hello" --title "Test" --icon "dialog-warning"

kdialog --sorry "I'm very sorry" --title "Test"

(kdialog crashes in ~KIconLoader when I pass --icon to --sorry but that also happens without this patch)

(What branch should this go in?)

Diff Detail

Repository
R229 KDialog
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik updated this revision to Diff 9148.Dec 19 2016, 6:43 AM
broulik retitled this revision from to Store arguments before creating QApplication.
broulik updated this object.
broulik edited the test plan for this revision. (Show Details)
broulik added a reviewer: dfaure.
broulik set the repository for this revision to R229 KDialog.
cfeck added a subscriber: cfeck.Dec 20 2016, 8:30 PM

Hm, if '--title' is stripped from arguments due to Qt processing it, is there a way to get the result of that processing? Otherwise, if Qt just silently removes the option, I would consider it a Qt bug that should get fixed.

dfaure accepted this revision.Dec 20 2016, 8:40 PM
dfaure edited edge metadata.

It's not *just* "silently removed", Qt uses --title to set the title of the first main window...
(see git grep firstWindowTitle in qtbase).

Arguably the Qt bug is the if (type() == Qt::Window) {...} in qwindow.cpp:518, maybe it should test for Qt::Dialog too, but that makes more sense for kdialog than for other apps where the first msg box would steal the user-specified title that was meant for the actual mainwindow...

So, no Qt bug I'd say. At most a missing feature NoBuiltinArgumentHandling.

The comment in this patch could say ".. and applies them to the first Qt::Window, while here we show only dialogs".

There is additional encoding complication with argv on Windows which probably make this patch wrong on non-latin1 Windows systems, but I don't know the details, other than the fact that Qt uses win32 API rather than argv.

This revision is now accepted and ready to land.Dec 20 2016, 8:40 PM
This revision was automatically updated to reflect the committed changes.