Bug 32925 - [Qt] Add Open File dialog to QtLauncher
Summary: [Qt] Add Open File dialog to QtLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-12-24 17:28 PST by Daniel Bates
Modified: 2009-12-29 10:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.25 KB, patch)
2009-12-24 17:32 PST, Daniel Bates
kenneth: review+
Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2009-12-29 10:12 PST, Daniel Bates
ariya.hidayat: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-12-24 17:28:44 PST
We should add an Open File dialog and menu item to QtLauncher to make it convenient for person to open a file.

Currently a person must either specify the path to a file as a command-line argument or type a file URL. Instead, we should have a file dialog to allow the user to open a file without memorizing its path.
Comment 1 Daniel Bates 2009-12-24 17:32:17 PST
Created attachment 45480 [details]
Patch
Comment 2 WebKit Review Bot 2009-12-24 17:37:16 PST
style-queue ran check-webkit-style on attachment 45480 [details] without any errors.
Comment 3 Kenneth Rohde Christiansen 2009-12-29 06:55:46 PST
Comment on attachment 45480 [details]
Patch

LGTM, r=me
Comment 4 Simon Hausmann 2009-12-29 07:17:09 PST
Comment on attachment 45480 [details]
Patch


> +        QFileDialog fileDialog(this, tr("Open"), "", filter);

Please use QString() instead of "".

> +        fileDialog.setAcceptMode(QFileDialog::AcceptOpen);
> +        fileDialog.setFileMode(QFileDialog::ExistingFile);
> +        fileDialog.setOptions(QFileDialog::ReadOnly);
> +
> +        if (fileDialog.exec()) {
> +            QString selectedFile = fileDialog.selectedFiles()[0];
> +            if (!selectedFile.isEmpty()) {
> +                QUrl fileURL("file://" + selectedFile);

This should be done using QUrl::fromLocalFile instead, to convert slashes correctly, support drives on Windows and support spaces in filenames, etc.

> +    void loadURL(const QUrl& url)
> +    {
> +        if (!url.isValid())
> +            return;
> +    
> +        urlEdit->setText(url.toEncoded());

That doesn't look correct. toEncoded() returns a QByteArray, urlEdit takes a QString. The conversion from a QByteArray to a QString must be done using a known encoding. It seems the use of toString() would be better.

> +        view->load(url);
> +        view->setFocus(Qt::OtherFocusReason);
> +    }
> +
>      // create the status bar, tool bar & menu
>      void setupUI()
>      {
> @@ -515,6 +539,7 @@ private:
>  
>          QMenu* fileMenu = menuBar()->addMenu("&File");
>          QAction* newWindow = fileMenu->addAction("New Window", this, SLOT(newWindow()));
> +        fileMenu->addAction(tr("Open File..."), this, SLOT(openFile()), QKeySequence(Qt::CTRL | Qt::Key_O));
>          fileMenu->addAction(tr("Print"), this, SLOT(print()), QKeySequence::Print);
>          QAction* screenshot = fileMenu->addAction("Screenshot", this, SLOT(screenshot()));
>          fileMenu->addAction("Close", this, SLOT(close()));
Comment 5 Kenneth Rohde Christiansen 2009-12-29 07:20:16 PST
(In reply to comment #4)
> (From update of attachment 45480 [details])
> 
> > +        QFileDialog fileDialog(this, tr("Open"), "", filter);
> 
> Please use QString() instead of "".

What about QLatin1String? 

Also, why are we using tr(), are we ever going to translate the launcher?
Comment 6 Ariya Hidayat 2009-12-29 08:24:36 PST
What about QGVLauncher? Would be nice to keep the symmetry by implementing the same for QGVLauncher.
Comment 7 Daniel Bates 2009-12-29 08:54:39 PST
(In reply to comment #4)
> (From update of attachment 45480 [details])
> 
> > +        QFileDialog fileDialog(this, tr("Open"), "", filter);
> 
> Please use QString() instead of "".
> 

Will do.

> > +        fileDialog.setAcceptMode(QFileDialog::AcceptOpen);
> > +        fileDialog.setFileMode(QFileDialog::ExistingFile);
> > +        fileDialog.setOptions(QFileDialog::ReadOnly);
> > +
> > +        if (fileDialog.exec()) {
> > +            QString selectedFile = fileDialog.selectedFiles()[0];
> > +            if (!selectedFile.isEmpty()) {
> > +                QUrl fileURL("file://" + selectedFile);
> 
> This should be done using QUrl::fromLocalFile instead, to convert slashes
> correctly, support drives on Windows and support spaces in filenames, etc.
> 

Will do.

> > +    void loadURL(const QUrl& url)
> > +    {
> > +        if (!url.isValid())
> > +            return;
> > +    
> > +        urlEdit->setText(url.toEncoded());
> 
> That doesn't look correct. toEncoded() returns a QByteArray, urlEdit takes a
> QString. The conversion from a QByteArray to a QString must be done using a
> known encoding. It seems the use of toString() would be better.

Will do. Will also make this change to line 221 <http://trac.webkit.org/browser/trunk/WebKitTools/QtLauncher/main.cpp?rev=52601#L221>.
Comment 8 Daniel Bates 2009-12-29 09:07:56 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 45480 [details] [details])
> > 
> > > +        QFileDialog fileDialog(this, tr("Open"), "", filter);
> > 
> > Please use QString() instead of "".
> 
> What about QLatin1String?

Is it better to use a QLatin1String over a QString? From the API for QFileDialog <http://doc.trolltech.com/4.6/qfiledialog.html> it is sufficient to pass a QString.

> 
> Also, why are we using tr(), are we ever going to translate the launcher?

I can remove it if you want? For your reference, tr() appears at following places in the code:

Line 85: <http://trac.webkit.org/browser/trunk/WebKitTools/QtLauncher/main.cpp?rev=52601#L85>
Line 520: <http://trac.webkit.org/browser/trunk/WebKitTools/QtLauncher/main.cpp?rev=52601#L520>
Line 555: <http://trac.webkit.org/browser/trunk/WebKitTools/QtLauncher/main.cpp?rev=52601#L555>
Line 582: <http://trac.webkit.org/browser/trunk/WebKitTools/QtLauncher/main.cpp?rev=52601#L582>

We should probably adopt an all or nothing approach to internationalization of QtLauncher. I think we should support possible "future" translations of the app.
Comment 9 Ariya Hidayat 2009-12-29 09:14:01 PST
> > What about QLatin1String?
> 
> Is it better to use a QLatin1String over a QString? From the API for
> QFileDialog <http://doc.trolltech.com/4.6/qfiledialog.html> it is sufficient to
> pass a QString.

QLatin1String is good for a string constant, since it avoids a copy.
However, in the above case, it is supposed to be a null string, so just use QString() as Simon suggested.

> 
> > 
> > Also, why are we using tr(), are we ever going to translate the launcher?
> 
> I can remove it if you want? For your reference, tr() appears at following
> places in the code:

My take: leave the tr as is, then we can have another patch that un-tr everything (if needed).
Comment 10 Daniel Bates 2009-12-29 09:18:19 PST
(In reply to comment #6)
> What about QGVLauncher? Would be nice to keep the symmetry by implementing the
> same for QGVLauncher.

Sure. I would suggest we do this in a separate bug.
Comment 11 Daniel Bates 2009-12-29 09:20:38 PST
Will make changes suggested by Simon Hausmann before I land.
Comment 12 Daniel Bates 2009-12-29 10:12:22 PST
Created attachment 45611 [details]
Patch

After talking with Ariya on IRC, updated patch to include similar changes to QGVLauncher.
Comment 13 WebKit Review Bot 2009-12-29 10:13:22 PST
style-queue ran check-webkit-style on attachment 45611 [details] without any errors.
Comment 14 Daniel Bates 2009-12-29 10:28:08 PST
Committed in <http://trac.webkit.org/changeset/52626>.