Bug 32925 – [Qt] Add Open File dialog to QtLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 45480 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45480 [details] without any errors.

[Kenneth Rohde Christiansen]

Comment on attachment 45480 [details]
Patch

LGTM, r=me

[Simon Hausmann]

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()));

[Kenneth Rohde Christiansen]

(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?

[Ariya Hidayat]

What about QGVLauncher? Would be nice to keep the symmetry by implementing the same for QGVLauncher.

[Daniel Bates]

(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>.

[Daniel Bates]

(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.

[Ariya Hidayat]

> > 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).

[Daniel Bates]

(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.

[Daniel Bates]

Will make changes suggested by Simon Hausmann before I land.

[Daniel Bates]

Created attachment 45611 [details]
Patch

After talking with Ariya on IRC, updated patch to include similar changes to QGVLauncher.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45611 [details] without any errors.

[Daniel Bates]

Committed in <http://trac.webkit.org/changeset/52626>.