Bug 48440 – [Qt] QtTestBrowser: User input lost when toggling use of QGraphicsView

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

[Dinu Jacob]

1. Open a launcher window
2. Type in google.com
3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView

Result: View changed but 'google.com' is removed from the url text box.

Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.

[Dinu Jacob]

Created attachment 72182 [details]
Patch

[Andreas Kling]

Comment on attachment 72182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72182&action=review

First off, this behavior sounds a bit strange behavior to me, shouldn't we load what's currently loaded, and simply preserve the text in the location edit?

> WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> +    QUrl url =  getAddressUrl();

Extra space after =

> WebKitTools/QtTestBrowser/mainwindow.h:56
> +    QString getAddressUrl() const;

Name should be "addressUrl"

[Dinu Jacob]

(In reply to comment #2)
> (From update of attachment 72182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72182&action=review
> 
> First off, this behavior sounds a bit strange behavior to me, shouldn't we load what's currently loaded, and simply preserve the text in the location edit?
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > +    QUrl url =  getAddressUrl();
> 
> Extra space after =
> 
> > WebKitTools/QtTestBrowser/mainwindow.h:56
> > +    QString getAddressUrl() const;
> 
> Name should be "addressUrl"

As the page was being loaded when the view is toggled, I used the user input rather than the page to avoid having to request the page to be loaded again. If the current page is loaded and the user input is preserved, it would be confusing as there is indication of a page being loaded but that doesn't match the user input.

[Dinu Jacob]

Created attachment 72196 [details]
Patch with review comment changes

[WebKit Review Bot]

Attachment 72196 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/QtTestBrowser/launcherwindow.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 72198 [details]
Style fixed

[Kenneth Rohde Christiansen]

Comment on attachment 72198 [details]
Style fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review

> WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> +    QUrl url = addressUrl();

In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.

> WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> +        load(url.toString());

Here you convert it into a string again!

> WebKitTools/QtTestBrowser/mainwindow.cpp:120
> +QString MainWindow::addressUrl() const
> +{
> +    return urlEdit->text();
> +}

This is what I mean!

[Dinu Jacob]

(In reply to comment #7)
> (From update of attachment 72198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > +    QUrl url = addressUrl();
> 
> In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > +        load(url.toString());
> 
> Here you convert it into a string again!
> 
> > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > +QString MainWindow::addressUrl() const
> > +{
> > +    return urlEdit->text();
> > +}
> 
> This is what I mean!

load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 72198 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > 
> > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > +    QUrl url = addressUrl();
> > 
> > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > 
> > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > +        load(url.toString());
> > 
> > Here you convert it into a string again!
> > 
> > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > +QString MainWindow::addressUrl() const
> > > +{
> > > +    return urlEdit->text();
> > > +}
> > 
> > This is what I mean!
> 
> load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?

We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.

[Antonio Gomes]

(In reply to comment #0)
> 1. Open a launcher window
> 2. Type in google.com
> 3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView
> 
> Result: View changed but 'google.com' is removed from the url text box.
> 
> Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.

I can not reproduce it on trunk. Are you running 2.1 branch?

[Dinu Jacob]

(In reply to comment #10)
> (In reply to comment #0)
> > 1. Open a launcher window
> > 2. Type in google.com
> > 3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView
> > 
> > Result: View changed but 'google.com' is removed from the url text box.
> > 
> > Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.
> 
> I can not reproduce it on trunk. Are you running 2.1 branch?

No, I am on trunk.

[Dinu Jacob]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 72198 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > > 
> > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > > +    QUrl url = addressUrl();
> > > 
> > > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > > 
> > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > > +        load(url.toString());
> > > 
> > > Here you convert it into a string again!
> > > 
> > > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > > +QString MainWindow::addressUrl() const
> > > > +{
> > > > +    return urlEdit->text();
> > > > +}
> > > 
> > > This is what I mean!
> > 
> > load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?
> 
> We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.

Yes, that is what MainWindow::load(const QString& url) is using too but in addition to that it it also converts relative file path to absolute file path and I am just continuing to use that.

[Dinu Jacob]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (From update of attachment 72198 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > > > 
> > > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > > > +    QUrl url = addressUrl();
> > > > 
> > > > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > > > 
> > > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > > > +        load(url.toString());
> > > > 
> > > > Here you convert it into a string again!
> > > > 
> > > > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > > > +QString MainWindow::addressUrl() const
> > > > > +{
> > > > > +    return urlEdit->text();
> > > > > +}
> > > > 
> > > > This is what I mean!
> > > 
> > > load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?
> > 
> > We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.
> 
> Yes, that is what MainWindow::load(const QString& url) is using too but in addition to that it it also converts relative file path to absolute file path and I am just continuing to use that.

So to use the overloaded load method that takes in a QString and to check whether the url is valid before calling load, I need the conversion from Url to String again.

[Dinu Jacob]

Created attachment 72352 [details]
Patch with review comment changes

Changed logic to avoid converting to QString->QUrl->QString. Also, caling MainWindow::load instead of page()->mainFrame()->load to set the input field value that would otherwise be removed when toggling view (and set only later)

[WebKit Review Bot]

Attachment 72352 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/QtTestBrowser/launcherwindow.cpp:98:  Extra space before ) in if  [whitespace/parens] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 72359 [details]
With style fix

[Antonio Gomes]

I am on trunk link, running qt 4.7 and am not able to run into this bug.

Could you please help me to reproduce it then I can help reviewing and testing.

[Antonio Gomes]

(In reply to comment #17)
> I am on trunk link,

link = linux

[Dinu Jacob]

(In reply to comment #17)
> I am on trunk link, running qt 4.7 and am not able to run into this bug.
> 
> Could you please help me to reproduce it then I can help reviewing and testing.

Provided clarification to Antonio on IRC

[Antonio Gomes]

Comment on attachment 72359 [details]
With style fix

Before reviewing, with this patch we would end up with the following behavior:

1) load qttestbrowser
2) type 'google.com' in the urlbar
(do NOT press ENTER or any other action that triggers loading)
3) switch to graphicsview mode.
result: google.com is loaded.

do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?

[Dinu Jacob]

(In reply to comment #20)
> (From update of attachment 72359 [details])
> Before reviewing, with this patch we would end up with the following behavior:
> 
> 1) load qttestbrowser
> 2) type 'google.com' in the urlbar
> (do NOT press ENTER or any other action that triggers loading)
> 3) switch to graphicsview mode.
> result: google.com is loaded.
> 
> do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?

I used the entered url to avoid doing another load (if a page has already been loaded, it will re-load the page and then the user has to press ENTER key to load the new page again). Also, the url displayed will not be consistent with the page loaded.

[Antonio Gomes]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 72359 [details] [details])
> > Before reviewing, with this patch we would end up with the following behavior:
> > 
> > 1) load qttestbrowser
> > 2) type 'google.com' in the urlbar
> > (do NOT press ENTER or any other action that triggers loading)
> > 3) switch to graphicsview mode.
> > result: google.com is loaded.
> > 
> > do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?
> 
> I used the entered url to avoid doing another load (if a page has already been loaded, it will re-load the page and then the user has to press ENTER key to load the new page again). Also, the url displayed will not be consistent with the page loaded.

Kenneth, KLing?

[Dinu Jacob]

Created attachment 79184 [details]
Preserve user input

[WebKit Review Bot]

Attachment 79184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/la..." exit_code: 1

Tools/QtTestBrowser/launcherwindow.cpp:143:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/QtTestBrowser/launcherwindow.cpp:144:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Tools/QtTestBrowser/launcherwindow.cpp:536:  Extra space before ) in if  [whitespace/parens] [5]
Tools/QtTestBrowser/launcherwindow.cpp:538:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/QtTestBrowser/launcherwindow.cpp:539:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

New changes made where the current page is loaded but preserve the user input, based on comments received.

[Dinu Jacob]

Created attachment 79186 [details]
With style fix

[Antonio Gomes]

Comment on attachment 79186 [details]
With style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=79186&action=review

> Tools/QtTestBrowser/launcherwindow.h:210
> +    QString inputUrl;

m_inputUrl as the other?

[Dinu Jacob]

Created attachment 79187 [details]
Updated with review comment

[Andreas Kling]

Comment on attachment 79187 [details]
Updated with review comment

r=me

[WebKit Commit Bot]

Comment on attachment 79187 [details]
Updated with review comment

Clearing flags on attachment: 79187

Committed r76860: <http://trac.webkit.org/changeset/76860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.