Merge into trunk : settings-page : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-04-07
Olivier Tilloy		2015-03-24	Approve on 2015-04-07
Bill Filler (community)			Approve on 2015-04-07
Review via email: mp+253975@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-24:

#

FAILED: Continuous integration, rev:939
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1561/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1949/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/319/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/319/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/319/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1947/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1561/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-24:

#

CI builds are failing because urlManagement.js is missing a copyright header.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-24:

#

Visually, this looks good except for the separation between the header and the list items. I think you should add a Divider there (https://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.ListItems.Divider/).

A few comments on the code itself:

- In SettingsPage.qml there are a bunch of references to 'browser', which breaks encapsulation: the SettingsPage component isn’t aware of its parent context. To avoid this, you should add top-level properties to the component for each setting, and in Browser.qml where the component is instantiated, listen to changes on those properties and update the corresponding properties on the Browser instance.

- Instead of a historyRemoved() signal, I would add a top-level 'historyModel' property, and have the corresponding action call clearAll() on the model. Also, the opacity shouldn’t be set to 0.5 after clearing. Instead, we need to add a 'count' property to the history model, and bind the opacity to its value being ≠ 0.

- The back button should probably be implemented using an AbstractButton (and it should be highlighted when pressed, to match the standard header).

- "Restore old session on startup" should probably be "Restore previous session at startup"

- "Open new tab in background" should probably be "Allow opening new tabs in background"

- The Column needs to be inside a Flickable, in case all entries don’t fit on the screen

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-25:

#

browser.allowOpenInBackgroundTab is a string, not a boolean, so the following won’t work as expected:

    control: Switch {
        checked: browser.allowOpenInBackgroundTab
        onClicked: browser.allowOpenInBackgroundTab = checked;
    }

Instead, you’ll need to check that either the value is "true", or it’s "default" and the form factor is desktop.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1563/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1568/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1571/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-26:

#

Hi Olivier,
thanks for the fast review.

I think I addressed all your requests. I split all fixes in single commits so you can review them one by one, if you prefer

> CI builds are failing because urlManagement.js is missing a copyright header.

Added

> Visually, this looks good except for the separation between the header and the list items. I think you should add a Divider there (https://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.ListItems.Divider/).

Indeed, I added it, now looks like a classic page header

> In SettingsPage.qml there are a bunch of references to 'browser', which breaks encapsulation: the SettingsPage component isn’t aware of its parent context. To avoid this, you should add top-level properties to the component for each setting, and in Browser.qml where the component is instantiated, listen to changes on those properties and update the corresponding properties on the Browser instance.

I'm not sure I understood this, I hope I fixed well

> Instead of a historyRemoved() signal, I would add a top-level 'historyModel' property, and have the corresponding action call clearAll() on the model. Also, the opacity shouldn’t be set to 0.5 after clearing. Instead, we need to add a 'count' property to the history model, and bind the opacity to its value being ≠ 0.

Done. I also added a test, as you asked on IRC - it's the first unit test I write, so let me know if could be improved. Also, let me know if we need other tests

> The back button should probably be implemented using an AbstractButton (and it should be highlighted when pressed, to match the standard header).

The higlighted when pressed was a bit tricky to implement, mainly to reproduce the shadow as it's in other apps, with a leftMargin, but I think I achieved that

> "Restore old session on startup" should probably be "Restore previous session at startup"
> "Open new tab in background" should probably be "Allow opening new tabs in background"

My bad english strikes again :-)

> The Column needs to be inside a Flickable, in case all entries don’t fit on the screen

Indeed, I added it also to the privacy page, because I think it will expande in the future

> browser.allowOpenInBackgroundTab is a string, not a boolean

Right, fixed

Hi Olivier,
thanks for the fast review.

I think I addressed all your requests. I split all fixes in single commits so you can review them one by one, if you prefer

> CI builds are failing because urlManagement.js is missing a copyright header.

Added

> Visually, this looks good except for the separation between the header and the list items. I think you should add a Divider there (https://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.ListItems.Divider/).

Indeed, I added it, now looks like a classic page header

> In SettingsPage.qml there are a bunch of references to 'browser', which breaks encapsulation: the SettingsPage component isn’t aware of its parent context. To avoid this, you should add top-level properties to the component for each setting, and in Browser.qml where the component is instantiated, listen to changes on those properties and update the corresponding properties on the Browser instance.

I'm not sure I understood this, I hope I fixed well

> Instead of a historyRemoved() signal, I would add a top-level 'historyModel' property, and have the corresponding action call clearAll() on the model. Also, the opacity shouldn’t be set to 0.5 after clearing. Instead, we need to add a 'count' property to the history model, and bind the opacity to its value being ≠ 0.

Done. I also added a test, as you asked on IRC - it's the first unit test I write, so let me know if could be improved. Also, let me know if we need other tests

> The back button should probably be implemented using an AbstractButton (and it should be highlighted when pressed, to match the standard header).

The higlighted when pressed was a bit tricky to implement, mainly to reproduce the shadow as it's in other apps, with a leftMargin, but I think I achieved that

> "Restore old session on startup" should probably be "Restore previous session at startup"
> "Open new tab in background" should probably be "Allow opening new tabs in background"

My bad english strikes again :-)

> The Column needs to be inside a Flickable, in case all entries don’t fit on the screen

Indeed, I added it also to the privacy page, because I think it will expande in the future

> browser.allowOpenInBackgroundTab is a string, not a boolean

Right, fixed

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-26:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1572/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-26:

#

A bunch of additional comments, in no particular order:

- Can the 'use strict' statement go after the license and copyright header? It looks weird before it…

- We need to make the current webview invisible while the settings page is visible, to avoid redrawing it uselessly (note that this isn’t done for the history view, but it should, really).
Setting 'visible: !settingsContainer.visible' on the top-level Item (line 108) that contains the chrome and the webviews should probably be good enough.
As a side note, run "QSG_VISUALIZE=overdraw ./src/app/webbrowser/webbrowser-app" to visualize when several opaque items overlap.

- Clicking "Reset browser settings" when a value has just been changed won’t update the corresponding entry in the view (try e.g. to change the homepage and just after that reset the settings)

- In the visual spec, it looks like all items have a thin divider at the bottom, so the "Reset browser settings" one probably needs one as well

- The new unit test looks good, can you please also test the emission of the signal, by adding a QSignalSpy?

-

review: Needs Fixing

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-27:

#

> - Can the 'use strict' statement go after the license and copyright header?
> It looks weird before it…

Ok

> - We need to make the current webview invisible while the settings page is
> visible, to avoid redrawing it uselessly (note that this isn’t done for the
> history view, but it should, really).
> Setting 'visible: !settingsContainer.visible' on the top-level Item (line 108)
> that contains the chrome and the webviews should probably be good enough.

Indeed it works. I know it isn't a good practice to mix code that should belong to a different branch, but since it's an half line change, I added also a flag for the history view - of course, if you don't agree, I remove that

> As a side note, run "QSG_VISUALIZE=overdraw ./src/app/webbrowser/webbrowser-
> app" to visualize when several opaque items overlap.

Thanks for the trick!

> - Clicking "Reset browser settings" when a value has just been changed won’t
> update the corresponding entry in the view (try e.g. to change the homepage
> and just after that reset the settings)

I'm totally lost on this one - I tried with all possible combination of browser.property and setting.property in Browser.qml, but nothing works.

Seems updating the property breaks the binding - only the view is broken, not the function itself.

Maybe after some sleep I'll be able to fix that, but every suggestion is welcome!

> - In the visual spec, it looks like all items have a thin divider at the
> bottom, so the "Reset browser settings" one probably needs one as well

I added it, but I totally don't like it: it's a divider, if there isn't anything to divide 'cause it's the last element, why do we have to add it? :-)

> - The new unit test looks good, can you please also test the emission of the
> signal, by adding a QSignalSpy?

Added

Also, I implemented the search engine choice, works as a charm (at least with DuckDuckGo).
Your solution is very elegant and very simple - I just have to don't think how much time I spent on C++ model (actually, I learned a lot, so I'm happy) :-)

I didn't add yet the search engine files, which ones do we want?

Could I suggest Google, Bing, Yahoo and DuckDuckGo?

>  - Can the 'use strict' statement go after the license and copyright header?
> It looks weird before it…

Ok

>  - We need to make the current webview invisible while the settings page is
> visible, to avoid redrawing it uselessly (note that this isn’t done for the
> history view, but it should, really).
> Setting 'visible: !settingsContainer.visible' on the top-level Item (line 108)
> that contains the chrome and the webviews should probably be good enough.

Indeed it works. I know it isn't a good practice to mix code that should belong to a different branch, but since it's an half line change, I added also a flag for the history view - of course, if you don't agree, I remove that

> As a side note, run "QSG_VISUALIZE=overdraw ./src/app/webbrowser/webbrowser-
> app" to visualize when several opaque items overlap.

Thanks for the trick!
 
>  - Clicking "Reset browser settings" when a value has just been changed won’t
> update the corresponding entry in the view (try e.g. to change the homepage
> and just after that reset the settings)

I'm totally lost on this one - I tried with all possible combination of browser.property and setting.property in Browser.qml, but nothing works.

Seems updating the property breaks the binding - only the view is broken, not the function itself.

Maybe after some sleep I'll be able to fix that, but every suggestion is welcome!

>  - In the visual spec, it looks like all items have a thin divider at the
> bottom, so the "Reset browser settings" one probably needs one as well

I added it, but I totally don't like it: it's a divider, if there isn't anything to divide 'cause it's the last element, why do we have to add it? :-)

>  - The new unit test looks good, can you please also test the emission of the
> signal, by adding a QSignalSpy?

Added

Also, I implemented the search engine choice, works as a charm (at least with DuckDuckGo).
Your solution is very elegant and very simple - I just have to don't think how much time I spent on C++ model (actually, I learned a lot, so I'm happy) :-)

I didn't add yet the search engine files, which ones do we want?

Could I suggest Google, Bing, Yahoo and DuckDuckGo?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-27:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1574/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

> Indeed it works. I know it isn't a good practice to mix code that should
> belong to a different branch, but since it's an half line change, I added also
> a flag for the history view - of course, if you don't agree, I remove that

Very good initiative!

> > - Clicking "Reset browser settings" when a value has just been changed
> won’t
> > update the corresponding entry in the view (try e.g. to change the homepage
> > and just after that reset the settings)
>
> I'm totally lost on this one - I tried with all possible combination of
> browser.property and setting.property in Browser.qml, but nothing works.
>
> Seems updating the property breaks the binding - only the view is broken, not
> the function itself.
>
> Maybe after some sleep I'll be able to fix that, but every suggestion is
> welcome!

I’ll take a closer look now.

> > - In the visual spec, it looks like all items have a thin divider at the
> > bottom, so the "Reset browser settings" one probably needs one as well
>
> I added it, but I totally don't like it: it's a divider, if there isn't
> anything to divide 'cause it's the last element, why do we have to add it? :-)

I don’t like it either, but let’s stick to the visual spec, it’s easy enough to change it later on.

> Also, I implemented the search engine choice, works as a charm (at least with
> DuckDuckGo).
> Your solution is very elegant and very simple - I just have to don't think how
> much time I spent on C++ model (actually, I learned a lot, so I'm happy) :-)
>
> I didn't add yet the search engine files, which ones do we want?
>
> Could I suggest Google, Bing, Yahoo and DuckDuckGo?

Yeah, I’d like to add more search engines to the list by default, but that will require more work as we would probably need to monitor several folders (system-wide + locally installed by the user), so let’s do that separately.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

Note: a bunch of branches were merged earlier today, and now there are minor conflicts in Browser.qml when merging this branch into trunk. Can you please address them?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

In SettingsPage.qml, because you refer to dataLocation, you should "import Ubuntu.Web 0.2".

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

Can you please update the visuals of the Search Engine page to match https://docs.google.com/a/canonical.com/presentation/d/1woHjO8K4iqyVZZlfQ4BXL0DhYbwkEmZ7wvcUhYzHDRk/edit#slide=id.g354125fb9_042 ?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

Since we’re using the FolderListModel now, we need to add qml-module-qt-labs-folderlistmodel as a runtime dependency of the webbrowser-app binary package (in debian/control, before line 39).

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

Indeed changing an option in the settings page breaks the initial property binding, and thus the properties are not reset in the settings page (even though their values are correctly reset).

We need to tackle this differently: the SettingsPage object needs a 'settings' property, which will be the shared Settings instance, and changing values there will update the properties on the shared instance directly, instead of going through an additional indirection. I have update my qt-labs-settings branch to allow that, please merge it into yours.
As well as getting rid of most of the top-level properties on the SettingsPage component (except historyModel, which is still needed), this will also allow us to remove the restoreDefaults signal.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

> > Could I suggest Google, Bing, Yahoo and DuckDuckGo?
>
> Yeah, I’d like to add more search engines to the list by default, but that
> will require more work as we would probably need to monitor several folders
> (system-wide + locally installed by the user), so let’s do that separately.

Until we enable that (and we ensure that at least two options are shipped by default), we’ll need to hide the Search Engines entry in the settings if we detect that the number of xml files in the searchengines/ folder is < 2. So the FolderListModel instance needs to move a few levels higher in the hierarchy and gain an id in order to be referenced from various places in the SettingsPage component.

review: Needs Fixing

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-27:

#

I think I addressed all your comments. Now the branch is synced both upstream and with your branch

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

I managed to get in a situation where two search engine entries are checked:

- open settings
- click "Search Engines"
- verify that e.g. the first one is checked
- check the second one
- click "Search Engines" again
- check the first one
- click "Search Engines" again

-> both entries are checked

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-27:

#

"Reset browser settings" doesn’t seem to reset the default values for checkbox options like "Restore previous session at startup" and "Allow opening new tabs in background". You might need to bind the checked value of the checkbox to the value of the setting itself.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-27:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1581/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-31:

#

Fixed all problems plus things we discussed on IRC :-)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

201 + subText: settingsObject.searchEngine

this is the filename (minus the .xml extension), not very user friendly. Instead, the subtext should have the display name of the engine, for which we need to instantiate a SearchEngine object:

    SearchEngine {
        id: currentSearchEngine
        filename: settingsObject.searchEngine
    }
    subText: currentSearchEngine.name

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

In src/app/webbrowser/Browser.qml, a blank line was removed, and one extra line was added. Can you please revert those two changes?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

In src/app/webbrowser/SettingsPageHeader.qml:

- Does the actual header really need to be a ListItem.Empty ? Can’t it just be a Rectangle?

- The 'text' property should be of type string, not var

- Can you rename 'trigger' to 'back' ?

- Does the ListItem.Divider really need a coloured Rectangle inside itself? Isn’t it themed correctly by default?

- The top-level item should be a Column, its anchors shouldn’t be defined in the definition of the component itself, and its height should probably be set to childrenRect.height.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-31:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1590/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-31:

#

> In src/app/webbrowser/SettingsPageHeader.qml:
>
> - Does the actual header really need to be a ListItem.Empty ? Can’t it just
> be a Rectangle?

Done

> - The 'text' property should be of type string, not var

Done

> - Can you rename 'trigger' to 'back' ?

Done

> - Does the ListItem.Divider really need a coloured Rectangle inside itself?
> Isn’t it themed correctly by default?

Yes, it needs the rectangle, otherwise when flickable goes under it it is semitrasparent

> - The top-level item should be a Column, its anchors shouldn’t be defined in
> the definition of the component itself, and its height should probably be set
> to childrenRect.height.

Done

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

In src/app/webbrowser/SettingsPage.qml:

- Instead of having searchEngineContainer and privacyContainer, we could have one single subpageContainer, as we will never have two subpages instantiated at the same time anyway.

- Most ListItems have an action defined with onTriggered, but the one for "Reset browser settings" only has onClicked. The result is the same, but can the code be made consistent (I don’t have any preference either way, other than the use of onClicked results in less code :))

- Instead of directly setting the opacity on the list item for "Clear Browsing History", you should set the 'enabled' property to (historyModel.count > 0)

- The code would be more readable if the header came before the flickable (just set 'clip' to true on the flickable, I’m told that rectangular clipping is cheap these days, and as a bonus you won’t need a coloured rectangle inside the divider in the header).

- Just a personal preference: I would instantiate the Bindings next to the switches their refer to, for improved readability.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

According to the visual spec, the font size for the header is x-large.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

> - Instead of having searchEngineContainer and privacyContainer, we could have
> one single subpageContainer, as we will never have two subpages instantiated
> at the same time anyway.

And most importantly, that would allow us to set the visibility of the main settings page accordingly (i.e. hide it whenever there’s a subpage overlaid on top of it).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-31:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1591/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

- "clip: true" needs to be set on the flickables, not on the headers.

- you can simplify the anchors of the header’s label a good deal by setting "anchors.verticalCenter: parent.verticalCenter"

- instead of trying to set the visibility of the entire page (which won’t work indeed), you can set the visibility of the main header and of the main flickable

review: Needs Fixing

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-03-31:

#

Thanks, fixed :-)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-31:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1592/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-31:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1593/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-31:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1594/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-03-31:

#

Looks good now, thanks Riccardo!

review: Approve

Revision history for this message

Bill Filler (bfiller) wrote on 2015-04-03:

#

I found some problems with the Homepage setting after clicking on it:
1) autocaps should be turned off on the field (Qt.ImhNoAutoUppercase)
2) predictive text should be turned off on the field (Qt.ImhNoPredictiveText)
3) set the hint for url field so the keyboard will display .com keys (Qt.ImhUrlCharactersOnly) - actually doing this might mean you don't need 1) and 2) as it might happen automatically
4) Upon displaying the dialog you should request focus on the input field with the caret at the end of the line. This will cause osk to appear by default saving the user an extra click
5) after making a change to a new site and pressing "Save", it is not saved and does not appear in the Homepage field in the main Settings.

There is no visual difference between "Privacy" and "Reset browser settings", but one brings you to another page (Privacy) and the other executes an action immediately. I believe Privacy should have a expansion arrow (like Privacy >) such that it's clear it leads you to another page.

review: Needs Fixing

lp:~rpadovani/webbrowser-app/settings-page updated on 2015-04-06

968. By Riccardo Padovani on 2015-04-06: Address Bill's comments

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-04-06:

#

Thanks for your review, Bill.

> I found some problems with the Homepage setting after clicking on it:
> 1) autocaps should be turned off on the field (Qt.ImhNoAutoUppercase)
> 2) predictive text should be turned off on the field (Qt.ImhNoPredictiveText)
> 3) set the hint for url field so the keyboard will display .com keys
> (Qt.ImhUrlCharactersOnly) - actually doing this might mean you don't need 1)
> and 2) as it might happen automatically

Right, done. I explicitly set all 3 properties

> 4) Upon displaying the dialog you should request focus on the input field with
> the caret at the end of the line. This will cause osk to appear by default
> saving the user an extra click

Right, done

> 5) after making a change to a new site and pressing "Save", it is not saved
> and does not appear in the Homepage field in the main Settings.

Sorry, I'm not able to reproduce that, works fine here.
Do you have any log? Does it happens only in some conditions?

> There is no visual difference between "Privacy" and "Reset browser settings",
> but one brings you to another page (Privacy) and the other executes an action
> immediately. I believe Privacy should have a expansion arrow (like Privacy >)
> such that it's clear it leads you to another page.

While I agree with you on that, Oliver and I chose to stick to design for the first implementation: we will probably remove also the divider after the last element, but at the moment we follow that slide: https://docs.google.com/presentation/d/1woHjO8K4iqyVZZlfQ4BXL0DhYbwkEmZ7wvcUhYzHDRk/edit#slide=id.g354125fb9_030

Of course, I can implement that if you know (not only for Privacy but also for search engines), just let me know.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-06:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1604/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-07:

#

Bill raised some good points, thanks for addressing them so quickly.
Following up on this, I think we should validate the contents of the homepage text field before saving them. For example, if I type something that is clearly not a URL (e.g. "foo bar baz"), it shouldn’t override the current value.

For this I think you’ll need to move the looksLikeAUrl() function from AddressBar.qml to urlManagement.js. We probably want to enable the "Save" button conditionnally only when the contents of the text field is a valid URL.

Revision history for this message

Bill Filler (bfiller) wrote on 2015-04-07:

#

All working well for me now. I can't reproduce the problem with changing the home page not showing up for me. Seems to work all the time now. +1 to land this.

review: Approve

lp:~rpadovani/webbrowser-app/settings-page updated on 2015-04-07

969. By Riccardo Padovani on 2015-04-07: Check if a domain is valid in the homepage setting before saving it

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-04-07:

#

Thanks for the suggestion Olivier, I implemented that.

BTW, we should take a look to the function looksLikeAUrl(). It checks if tld.length is < 5 (.match(/\.[a-zA-Z]{2,4}$/))
Now TLD could be very long[0], so we should remove that restriction (also if they aren't used so much at the moment, the only one I know is com.google)

[0]: https://en.wikipedia.org/wiki/List_of_Internet_top-level_domains#ICANN-era_generic_top-level_domains

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-07:

#

> BTW, we should take a look to the function looksLikeAUrl(). It checks if
> tld.length is < 5 (.match(/\.[a-zA-Z]{2,4}$/))
> Now TLD could be very long[0], so we should remove that restriction (also if
> they aren't used so much at the moment, the only one I know is com.google)

Good point. Can you file a bug report to track the issue separately?

lp:~rpadovani/webbrowser-app/settings-page updated on 2015-04-07

970. By Riccardo Padovani on 2015-04-07: Merge from trunk and resolve conflicts

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-07:

#

Looks good now, thanks Riccardo!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-07:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1611/rebuild

review: Needs Fixing (continuous-integration)

webbrowser-app

Merge lp:~rpadovani/webbrowser-app/settings-page into lp:webbrowser-app

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'debian/control'
 --- debian/control	2015-04-07 17:11:13 +0000
 +++ debian/control	2015-04-07 17:11:13 +0000
@@ -36,6 +36,7 @@
           fonts-liberation,
           liboxideqt-qmlplugin (>= 1.5),
           libqt5sql5-sqlite,
++         qml-module-qt-labs-folderlistmodel,
           qml-module-qt-labs-settings,
           qml-module-qtquick2 (>= 5.4) | qtdeclarative5-qtquick2-plugin (>= 5.4),
           qml-module-qtquick-dialogs | qtdeclarative5-dialogs-plugin,
 === modified file 'src/app/webbrowser/AddressBar.qml'
 --- src/app/webbrowser/AddressBar.qml	2015-03-25 14:12:12 +0000
 +++ src/app/webbrowser/AddressBar.qml	2015-04-07 17:11:13 +0000
@@ -21,6 +21,7 @@
  import Ubuntu.Components.Popups 1.0
  import com.canonical.Oxide 1.0 as Oxide
  import ".."
++import "urlManagement.js" as UrlManagement
  FocusScope {
      id: addressbar
@@ -88,7 +89,7 @@
                      readonly property bool reload: addressbar.activeFocus && addressbar.text &&
                                                     (addressbar.text == addressbar.actualUrl)
--                    readonly property bool looksLikeAUrl: internal.looksLikeAUrl(addressbar.text.trim())
++                    readonly property bool looksLikeAUrl: UrlManagement.looksLikeAUrl(addressbar.text.trim())
                      name: addressbar.loading ? "stop" :
                            reload ? "reload" :
@@ -235,38 +236,6 @@
          property var securityCertificateDetails: null
--        function looksLikeAUrl(address) {
--            var terms = address.split(/\s/)
--            if (terms.length > 1) {
--                return false
--            }
--            if (address.substr(0, 1) == "/") {
--                return true
--            }
--            if (address.match(/^https?:\/\//i) ||
--                address.match(/^file:\/\//i) ||
--                address.match(/^[a-z]+:\/\//i)) {
--                return true
--            }
--            if (address.split('/', 1)[0].match(/\.[a-zA-Z]{2,4}$/)) {
--                return true
--            }
--            if (address.split('/', 1)[0].match(/^(?:[0-9]{1,3}\.){3}[0-9]{1,3}/)) {
--                return true
--            }
--            return false
--        }
--
--        function fixUrl(address) {
--            var url = address
--            if (address.substr(0, 1) == "/") {
--                url = "file://" + address
--            } else if (address.indexOf("://") == -1) {
--                url = "http://" + address
--            }
--            return url
--        }
--
          function escapeHtmlEntities(query) {
              return query.replace(/\W/, encodeURIComponent)
+         }
@@ -278,8 +247,8 @@
          function validate() {
              var query = text.trim()
--            if (internal.looksLikeAUrl(query)) {
--                requestedUrl = internal.fixUrl(query)
++            if (UrlManagement.looksLikeAUrl(query)) {
++                requestedUrl = UrlManagement.fixUrl(query)
              } else {
                  requestedUrl = internal.buildSearchUrl(query)
+             }
 === modified file 'src/app/webbrowser/Browser.qml'
 --- src/app/webbrowser/Browser.qml	2015-04-07 17:11:13 +0000
 +++ src/app/webbrowser/Browser.qml	2015-04-07 17:11:13 +0000
@@ -102,6 +102,7 @@
      Item {
          anchors.fill: parent
++        visible: !settingsContainer.visible && !historyViewContainer.visible
          TabChrome {
              id: invisibleTabChrome
@@ -245,6 +246,12 @@
                      iconName: "tab-new"
                      enabled: formFactor != "mobile"
                      onTriggered: browser.openUrlInNewTab("", true)
++                },
++                Action {
++                    objectName: "settings"
++                    text: i18n.tr("Settings")
++                    iconName: "settings"
++                    onTriggered: settingsComponent.createObject(settingsContainer)
+                 }
+             ]
+         }
@@ -478,6 +485,24 @@
+         }
+     }
++    Item {
++        id: settingsContainer
++
++        visible: children.length > 0
++        anchors.fill: parent
++
++        Component {
++            id: settingsComponent
++
++            SettingsPage {
++                anchors.fill: parent
++                historyModel: browser.historyModel
++                settingsObject: settings
++                onDone: destroy()
++            }
++        }
++    }
++
      TabsModel {
          id: tabsModel
 === modified file 'src/app/webbrowser/CMakeLists.txt'
 --- src/app/webbrowser/CMakeLists.txt	2015-04-07 17:11:13 +0000
 +++ src/app/webbrowser/CMakeLists.txt	2015-04-07 17:11:13 +0000
@@ -42,7 +42,7 @@
  install(TARGETS ${WEBBROWSER_APP}
          RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
--file(GLOB QML_FILES *.qml)
++file(GLOB QML_FILES *.qml *.js)
  install(FILES ${QML_FILES} DESTINATION ${CMAKE_INSTALL_DATADIR}/webbrowser-app/webbrowser)
  install(DIRECTORY assets
 === added file 'src/app/webbrowser/SettingsPage.qml'
 --- src/app/webbrowser/SettingsPage.qml	1970-01-01 00:00:00 +0000
 +++ src/app/webbrowser/SettingsPage.qml	2015-04-07 17:11:13 +0000
@@ -0,0 +1,277 @@
++/*
++ * Copyright 2015 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++import QtQuick 2.0
++import Qt.labs.folderlistmodel 2.1
++import Qt.labs.settings 1.0
++import Ubuntu.Components 1.1
++import Ubuntu.Components.Popups 1.0
++import Ubuntu.Components.ListItems 1.0 as ListItem
++import Ubuntu.Web 0.2
++import webbrowserapp.private 0.1
++
++import "urlManagement.js" as UrlManagement
++
++Item {
++    id: settingsItem
++
++    property QtObject historyModel
++    property Settings settingsObject
++
++    signal done()
++
++    Rectangle {
++        anchors.fill: parent
++        color: "#f6f6f6"
++    }
++
++    FolderListModel {
++        id: searchEngineFolder
++        folder: dataLocation +"/searchengines"
++        showDirs: false
++        nameFilters: ["*.xml"]
++        sortField: FolderListModel.Name
++    }
++
++    SettingsPageHeader {
++        id: title
++
++        onBack: settingsItem.done()
++        text: i18n.tr("Settings")
++        visible: !subpageContainer.visible
++    }
++
++    Flickable {
++        anchors {
++            top: title.bottom
++            left: parent.left
++            right: parent.right
++            bottom: parent.bottom
++        }
++
++        visible: !subpageContainer.visible
++        clip: true
++        contentHeight: settingsCol.height
++
++        Column {
++            id: settingsCol
++
++            width: parent.width
++
++            ListItem.Subtitled {
++                SearchEngine {
++                    id: currentSearchEngine
++                    filename: settingsObject.searchEngine
++                }
++                text: i18n.tr("Search engine")
++                subText: currentSearchEngine.name
++
++                visible: searchEngineFolder.count > 1
++
++                onClicked: searchEngineComponent.createObject(subpageContainer);
++            }
++
++            ListItem.Subtitled {
++                text: i18n.tr("Homepage")
++                subText: settingsObject.homepage
++
++                onClicked: PopupUtils.open(homepageDialog)
++            }
++
++            ListItem.Standard {
++                text: i18n.tr("Restore previous session at startup")
++                highlightWhenPressed: false
++
++                control: Switch {
++                    id: restoreSessionSwitch
++                    onClicked: settingsObject.restoreSession = checked;
++                }
++
++                Binding {
++                    target: restoreSessionSwitch; property: "checked";
++                    value: settingsObject.restoreSession
++                }
++            }
++
++            ListItem.Standard {
++                text: i18n.tr("Allow opening new tabs in background")
++                highlightWhenPressed: false
++
++                control: Switch {
++                    id: allowOpenInBackgroundTabSwitch
++
++                    onClicked: settingsObject.allowOpenInBackgroundTab = checked ? 'true' : 'false';
++                }
++
++                Binding {
++                    target: allowOpenInBackgroundTabSwitch; property: "checked";
++                    value: settingsObject.allowOpenInBackgroundTab === 'true' ||
++                    (settingsObject.allowOpenInBackgroundTab === 'default' &&
++                        formFactor === "desktop")
++                }
++            }
++
++            ListItem.Standard {
++                text: i18n.tr("Privacy")
++
++                onClicked: privacyComponent.createObject(subpageContainer);
++            }
++
++            ListItem.Standard {
++                text: i18n.tr("Reset browser settings")
++                onClicked: settingsObject.restoreDefaults();
++            }
++        }
++    }
++
++    Item {
++        id: subpageContainer
++
++        visible: children.length > 0
++        anchors.fill: parent
++
++        Component {
++            id: searchEngineComponent
++
++            Item {
++                id: searchEngineItem
++                anchors.fill: parent
++
++                Rectangle {
++                    anchors.fill: parent
++                    color: "#f6f6f6"
++                }
++
++                SettingsPageHeader {
++                    id: searchEngineTitle
++
++                    onBack: searchEngineItem.destroy();
++                    text: i18n.tr("Search engine")
++                }
++
++                ListView {
++                    anchors {
++                        top: searchEngineTitle.bottom
++                        left: parent.left
++                        right: parent.right
++                        bottom: parent.bottom
++                    }
++
++                    model: searchEngineFolder
++
++                    delegate: ListItem.Standard {
++                        SearchEngine {
++                            id: searchEngineDelegate
++                            filename: model.fileBaseName
++                        }
++                        text: searchEngineDelegate.name
++
++                        control: CheckBox {
++                            checked: settingsObject.searchEngine == searchEngineDelegate.filename;
++                            onClicked: {
++                                settingsObject.searchEngine = searchEngineDelegate.filename;
++                                searchEngineItem.destroy();
++                            }
++                        }
++                    }
++                }
++            }
++        }
++
++        Component {
++            id: privacyComponent
++
++            Item {
++                id: privacyItem
++                anchors.fill: parent
++
++                Rectangle {
++                    anchors.fill: parent
++                    color: "#f6f6f6"
++                }
++
++                SettingsPageHeader {
++                    id: privacyTitle
++                    onBack: privacyItem.destroy();
++                    text: i18n.tr("Privacy")
++                }
++
++                Flickable {
++                    anchors {
++                        top: privacyTitle.bottom
++                        left: parent.left
++                        right: parent.right
++                        bottom: parent.bottom
++                    }
++
++                    clip: true
++
++                    contentHeight: privacyCol.height
++
++                    Column {
++                        id: privacyCol
++                        width: parent.width
++
++                        ListItem.Standard {
++                            text: i18n.tr("Clear Browsing History")
++                            onClicked: historyModel.clearAll();
++                            enabled: historyModel.count > 0
++                        }
++                    }
++                }
++            }
++        }
++    }
++
++    Component {
++        id: homepageDialog
++        Dialog {
++            id: dialogue
++            title: i18n.tr("Homepage")
++
++            Component.onCompleted: {
++                homepageTextField.forceActiveFocus();
++                homepageTextField.cursorPosition = homepageTextField.text.length
++            }
++
++            TextField {
++                id: homepageTextField
++                text: settingsObject.homepage
++                inputMethodHints: Qt.ImhNoAutoUppercase | Qt.ImhNoPredictiveText | Qt.ImhUrlCharactersOnly
++            }
++
++            Button {
++                anchors { left: parent.left; right: parent.right }
++                text: i18n.tr("Cancel")
++                onClicked: PopupUtils.close(dialogue);
++            }
++
++            Button {
++                anchors { left: parent.left; right: parent.right }
++                text: i18n.tr("Save")
++                enabled: UrlManagement.looksLikeAUrl(homepageTextField.text)
++                color: "#3fb24f"
++                onClicked: {
++                    settingsObject.homepage = UrlManagement.fixUrl(homepageTextField.text);
++                    PopupUtils.close(dialogue);
++                }
++            }
++        }
++    }
++}
++
 === added file 'src/app/webbrowser/SettingsPageHeader.qml'
 --- src/app/webbrowser/SettingsPageHeader.qml	1970-01-01 00:00:00 +0000
 +++ src/app/webbrowser/SettingsPageHeader.qml	2015-04-07 17:11:13 +0000
@@ -0,0 +1,94 @@
++ /*
++ * Copyright 2015 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++import QtQuick 2.0
++import Ubuntu.Components 1.1
++import Ubuntu.Components.ListItems 1.0 as ListItem
++
++/*
++ * Component to use as page header in settings page and subpages
++ *
++ * It has a back() signal fired when back button is pressed and a text
++ * property to set the page title
++ */
++
++Column {
++    id: root
++    signal back()
++    property string text
++
++    height: childrenRect.height
++
++    anchors {
++        left: parent.left
++        right: parent.right
++    }
++
++    Rectangle {
++        id: title
++
++        height: units.gu(7) - divider.height
++        anchors { left: parent.left; right: parent.right }
++
++        Rectangle {
++            anchors.fill: parent
++            color: "#f6f6f6"
++        }
++
++        AbstractButton {
++            id: backButton
++            width: height
++
++            onTriggered: root.back()
++            anchors {
++                top: parent.top
++                bottom: parent.bottom
++                left: parent.left
++            }
++
++            Rectangle {
++                anchors.fill: parent
++                anchors.leftMargin: units.gu(1)
++                anchors.rightMargin: units.gu(1)
++                color: "#E6E6E6"
++                visible: parent.pressed
++            }
++
++            Icon {
++                name: "back"
++                anchors {
++                    fill: parent
++                    topMargin: units.gu(2)
++                    bottomMargin: units.gu(2)
++                }
++            }
++        }
++
++        Label {
++            anchors {
++                left: backButton.right
++                verticalCenter: parent.verticalCenter
++            }
++            text: root.text
++            fontSize: 'x-large'
++        }
++    }
++
++    ListItem.Divider {
++        id: divider
++    }
++}
 === modified file 'src/app/webbrowser/history-model.cpp'
 --- src/app/webbrowser/history-model.cpp	2014-08-05 17:39:44 +0000
 +++ src/app/webbrowser/history-model.cpp	2015-04-07 17:11:13 +0000
@@ -222,6 +222,7 @@
          m_entries.prepend(entry);
          endInsertRows();
          insertNewEntryInDatabase(entry);
++        Q_EMIT rowCountChanged();
      } else {
          QVector<int> roles;
          roles << Visits;
@@ -281,6 +282,7 @@
      removeByIndex(getEntryIndex(url));
      removeEntryFromDatabaseByUrl(url);
++    Q_EMIT rowCountChanged();
+ }
  /*!
@@ -298,6 +300,7 @@
+         }
+     }
      removeEntriesFromDatabaseByDomain(domain);
++    Q_EMIT rowCountChanged();
+ }
  void HistoryModel::removeByIndex(int index)
@@ -363,6 +366,7 @@
          m_entries.clear();
          endResetModel();
          clearDatabase();
++        Q_EMIT rowCountChanged();
+     }
+ }
 === modified file 'src/app/webbrowser/history-model.h'
 --- src/app/webbrowser/history-model.h	2014-08-05 17:39:44 +0000
 +++ src/app/webbrowser/history-model.h	2015-04-07 17:11:13 +0000
@@ -32,6 +32,7 @@
      Q_OBJECT
      Q_PROPERTY(QString databasePath READ databasePath WRITE setDatabasePath NOTIFY databasePathChanged)
++    Q_PROPERTY(int count READ rowCount NOTIFY rowCountChanged)
      Q_ENUMS(Roles)
@@ -64,6 +65,7 @@
  Q_SIGNALS:
      void databasePathChanged() const;
++    void rowCountChanged();
  private:
      QSqlDatabase m_database;
 === added file 'src/app/webbrowser/urlManagement.js'
 --- src/app/webbrowser/urlManagement.js	1970-01-01 00:00:00 +0000
 +++ src/app/webbrowser/urlManagement.js	2015-04-07 17:11:13 +0000
@@ -0,0 +1,50 @@
++/*
++ * Copyright 2015 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++*/
++'use strict';
++
++function fixUrl(address) {
++    var url = address
++    if (address.substr(0, 1) == "/") {
++        url = "file://" + address
++    } else if (address.indexOf("://") == -1) {
++        url = "http://" + address
++    }
++    return url
++}
++
++function looksLikeAUrl(address) {
++    var terms = address.split(/\s/)
++    if (terms.length > 1) {
++        return false
++    }
++    if (address.substr(0, 1) == "/") {
++        return true
++    }
++    if (address.match(/^https?:\/\//i) ||
++        address.match(/^file:\/\//i) ||
++        address.match(/^[a-z]+:\/\//i)) {
++        return true
++    }
++    if (address.split('/', 1)[0].match(/\.[a-zA-Z]{2,4}$/)) {
++        return true
++    }
++    if (address.split('/', 1)[0].match(/^(?:[0-9]{1,3}\.){3}[0-9]{1,3}/)) {
++        return true
++    }
++    return false
++}
 === modified file 'tests/unittests/history-model/tst_HistoryModelTests.cpp'
 --- tests/unittests/history-model/tst_HistoryModelTests.cpp	2014-08-05 17:39:44 +0000
 +++ tests/unittests/history-model/tst_HistoryModelTests.cpp	2015-04-07 17:11:13 +0000
@@ -243,6 +243,21 @@
          QCOMPARE(model->rowCount(), 0);
+     }
++    void shouldCountNumberOfEntries()
++    {
++        QSignalSpy spyCount(model, SIGNAL(rowCountChanged()));
++        QCOMPARE(model->property("count").toInt(), 0);
++        model->add(QUrl("http://example.org/"), "Example Domain", QUrl());
++        QCOMPARE(model->property("count").toInt(), 1);
++        QCOMPARE(spyCount.count(), 1);
++        model->add(QUrl("http://example.com/"), "Example Domain", QUrl());
++        QCOMPARE(model->property("count").toInt(), 2);
++        QCOMPARE(spyCount.count(), 2);
++        model->clearAll();
++        QCOMPARE(model->property("count").toInt(), 0);
++        QCOMPARE(spyCount.count(), 3);
++    }
++
  };
  QTEST_MAIN(HistoryModelTests)