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.
Created attachment 45480 [details] Patch
style-queue ran check-webkit-style on attachment 45480 [details] without any errors.
Comment on attachment 45480 [details] Patch LGTM, r=me
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()));
(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?
What about QGVLauncher? Would be nice to keep the symmetry by implementing the same for QGVLauncher.
(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>.
(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.
> > 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).
(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.
Will make changes suggested by Simon Hausmann before I land.
Created attachment 45611 [details] Patch After talking with Ariya on IRC, updated patch to include similar changes to QGVLauncher.
style-queue ran check-webkit-style on attachment 45611 [details] without any errors.
Committed in <http://trac.webkit.org/changeset/52626>.