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

Proposed by Riccardo Padovani
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 970
Merged at revision: 956
Proposed branch: lp:~rpadovani/webbrowser-app/settings-page
Merge into: lp:webbrowser-app
Prerequisite: lp:~osomon/webbrowser-app/qt-labs-settings
Diff against target: 663 lines (+473/-36)
10 files modified
debian/control (+1/-0)
src/app/webbrowser/AddressBar.qml (+4/-35)
src/app/webbrowser/Browser.qml (+25/-0)
src/app/webbrowser/CMakeLists.txt (+1/-1)
src/app/webbrowser/SettingsPage.qml (+277/-0)
src/app/webbrowser/SettingsPageHeader.qml (+94/-0)
src/app/webbrowser/history-model.cpp (+4/-0)
src/app/webbrowser/history-model.h (+2/-0)
src/app/webbrowser/urlManagement.js (+50/-0)
tests/unittests/history-model/tst_HistoryModelTests.cpp (+15/-0)
To merge this branch: bzr merge lp:~rpadovani/webbrowser-app/settings-page
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Olivier Tilloy Approve
Bill Filler (community) Approve
Review via email: mp+253975@code.launchpad.net

This proposal supersedes a proposal from 2015-03-24.

Commit message

Add settings page, per design specification. This adds qml-module-qt-labs-folderlistmodel and qml-module-qt-labs-settings as runtime dependencies for webbrowser-app.

Description of the change

Add settings page, per design specification.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

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

Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

> - 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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

> 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 :

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 :

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 :
Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

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 :

> > 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 :

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 :

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 :

"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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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

Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

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 :

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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

> 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 :

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 :

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 :

> - 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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

 - "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 :

Thanks, fixed :-)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks Riccardo!

review: Approve
Revision history for this message
Bill Filler (bfiller) wrote :

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
968. By Riccardo Padovani

Address Bill's comments

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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 :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

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
969. By Riccardo Padovani

Check if a domain is valid in the homepage setting before saving it

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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 :

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

970. By Riccardo Padovani

Merge from trunk and resolve conflicts

Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks Riccardo!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
36 fonts-liberation,36 fonts-liberation,
37 liboxideqt-qmlplugin (>= 1.5),37 liboxideqt-qmlplugin (>= 1.5),
38 libqt5sql5-sqlite,38 libqt5sql5-sqlite,
39 qml-module-qt-labs-folderlistmodel,
39 qml-module-qt-labs-settings,40 qml-module-qt-labs-settings,
40 qml-module-qtquick2 (>= 5.4) | qtdeclarative5-qtquick2-plugin (>= 5.4),41 qml-module-qtquick2 (>= 5.4) | qtdeclarative5-qtquick2-plugin (>= 5.4),
41 qml-module-qtquick-dialogs | qtdeclarative5-dialogs-plugin,42 qml-module-qtquick-dialogs | qtdeclarative5-dialogs-plugin,
4243
=== 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 @@
21import Ubuntu.Components.Popups 1.021import Ubuntu.Components.Popups 1.0
22import com.canonical.Oxide 1.0 as Oxide22import com.canonical.Oxide 1.0 as Oxide
23import ".."23import ".."
24import "urlManagement.js" as UrlManagement
2425
25FocusScope {26FocusScope {
26 id: addressbar27 id: addressbar
@@ -88,7 +89,7 @@
8889
89 readonly property bool reload: addressbar.activeFocus && addressbar.text &&90 readonly property bool reload: addressbar.activeFocus && addressbar.text &&
90 (addressbar.text == addressbar.actualUrl)91 (addressbar.text == addressbar.actualUrl)
91 readonly property bool looksLikeAUrl: internal.looksLikeAUrl(addressbar.text.trim())92 readonly property bool looksLikeAUrl: UrlManagement.looksLikeAUrl(addressbar.text.trim())
9293
93 name: addressbar.loading ? "stop" :94 name: addressbar.loading ? "stop" :
94 reload ? "reload" :95 reload ? "reload" :
@@ -235,38 +236,6 @@
235236
236 property var securityCertificateDetails: null237 property var securityCertificateDetails: null
237238
238 function looksLikeAUrl(address) {
239 var terms = address.split(/\s/)
240 if (terms.length > 1) {
241 return false
242 }
243 if (address.substr(0, 1) == "/") {
244 return true
245 }
246 if (address.match(/^https?:\/\//i) ||
247 address.match(/^file:\/\//i) ||
248 address.match(/^[a-z]+:\/\//i)) {
249 return true
250 }
251 if (address.split('/', 1)[0].match(/\.[a-zA-Z]{2,4}$/)) {
252 return true
253 }
254 if (address.split('/', 1)[0].match(/^(?:[0-9]{1,3}\.){3}[0-9]{1,3}/)) {
255 return true
256 }
257 return false
258 }
259
260 function fixUrl(address) {
261 var url = address
262 if (address.substr(0, 1) == "/") {
263 url = "file://" + address
264 } else if (address.indexOf("://") == -1) {
265 url = "http://" + address
266 }
267 return url
268 }
269
270 function escapeHtmlEntities(query) {239 function escapeHtmlEntities(query) {
271 return query.replace(/\W/, encodeURIComponent)240 return query.replace(/\W/, encodeURIComponent)
272 }241 }
@@ -278,8 +247,8 @@
278247
279 function validate() {248 function validate() {
280 var query = text.trim()249 var query = text.trim()
281 if (internal.looksLikeAUrl(query)) {250 if (UrlManagement.looksLikeAUrl(query)) {
282 requestedUrl = internal.fixUrl(query)251 requestedUrl = UrlManagement.fixUrl(query)
283 } else {252 } else {
284 requestedUrl = internal.buildSearchUrl(query)253 requestedUrl = internal.buildSearchUrl(query)
285 }254 }
286255
=== 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 @@
102102
103 Item {103 Item {
104 anchors.fill: parent104 anchors.fill: parent
105 visible: !settingsContainer.visible && !historyViewContainer.visible
105106
106 TabChrome {107 TabChrome {
107 id: invisibleTabChrome108 id: invisibleTabChrome
@@ -245,6 +246,12 @@
245 iconName: "tab-new"246 iconName: "tab-new"
246 enabled: formFactor != "mobile"247 enabled: formFactor != "mobile"
247 onTriggered: browser.openUrlInNewTab("", true)248 onTriggered: browser.openUrlInNewTab("", true)
249 },
250 Action {
251 objectName: "settings"
252 text: i18n.tr("Settings")
253 iconName: "settings"
254 onTriggered: settingsComponent.createObject(settingsContainer)
248 }255 }
249 ]256 ]
250 }257 }
@@ -478,6 +485,24 @@
478 }485 }
479 }486 }
480487
488 Item {
489 id: settingsContainer
490
491 visible: children.length > 0
492 anchors.fill: parent
493
494 Component {
495 id: settingsComponent
496
497 SettingsPage {
498 anchors.fill: parent
499 historyModel: browser.historyModel
500 settingsObject: settings
501 onDone: destroy()
502 }
503 }
504 }
505
481 TabsModel {506 TabsModel {
482 id: tabsModel507 id: tabsModel
483508
484509
=== 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 @@
42install(TARGETS ${WEBBROWSER_APP}42install(TARGETS ${WEBBROWSER_APP}
43 RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})43 RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
4444
45file(GLOB QML_FILES *.qml)45file(GLOB QML_FILES *.qml *.js)
46install(FILES ${QML_FILES} DESTINATION ${CMAKE_INSTALL_DATADIR}/webbrowser-app/webbrowser)46install(FILES ${QML_FILES} DESTINATION ${CMAKE_INSTALL_DATADIR}/webbrowser-app/webbrowser)
4747
48install(DIRECTORY assets48install(DIRECTORY assets
4949
=== 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 @@
1/*
2 * Copyright 2015 Canonical Ltd.
3 *
4 * This file is part of webbrowser-app.
5 *
6 * webbrowser-app is free software; you can redistribute it and/or modify
7 * it under the terms of the GNU General Public License as published by
8 * the Free Software Foundation; version 3.
9 *
10 * webbrowser-app is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details.
14 *
15 * You should have received a copy of the GNU General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 */
18
19import QtQuick 2.0
20import Qt.labs.folderlistmodel 2.1
21import Qt.labs.settings 1.0
22import Ubuntu.Components 1.1
23import Ubuntu.Components.Popups 1.0
24import Ubuntu.Components.ListItems 1.0 as ListItem
25import Ubuntu.Web 0.2
26import webbrowserapp.private 0.1
27
28import "urlManagement.js" as UrlManagement
29
30Item {
31 id: settingsItem
32
33 property QtObject historyModel
34 property Settings settingsObject
35
36 signal done()
37
38 Rectangle {
39 anchors.fill: parent
40 color: "#f6f6f6"
41 }
42
43 FolderListModel {
44 id: searchEngineFolder
45 folder: dataLocation +"/searchengines"
46 showDirs: false
47 nameFilters: ["*.xml"]
48 sortField: FolderListModel.Name
49 }
50
51 SettingsPageHeader {
52 id: title
53
54 onBack: settingsItem.done()
55 text: i18n.tr("Settings")
56 visible: !subpageContainer.visible
57 }
58
59 Flickable {
60 anchors {
61 top: title.bottom
62 left: parent.left
63 right: parent.right
64 bottom: parent.bottom
65 }
66
67 visible: !subpageContainer.visible
68 clip: true
69 contentHeight: settingsCol.height
70
71 Column {
72 id: settingsCol
73
74 width: parent.width
75
76 ListItem.Subtitled {
77 SearchEngine {
78 id: currentSearchEngine
79 filename: settingsObject.searchEngine
80 }
81 text: i18n.tr("Search engine")
82 subText: currentSearchEngine.name
83
84 visible: searchEngineFolder.count > 1
85
86 onClicked: searchEngineComponent.createObject(subpageContainer);
87 }
88
89 ListItem.Subtitled {
90 text: i18n.tr("Homepage")
91 subText: settingsObject.homepage
92
93 onClicked: PopupUtils.open(homepageDialog)
94 }
95
96 ListItem.Standard {
97 text: i18n.tr("Restore previous session at startup")
98 highlightWhenPressed: false
99
100 control: Switch {
101 id: restoreSessionSwitch
102 onClicked: settingsObject.restoreSession = checked;
103 }
104
105 Binding {
106 target: restoreSessionSwitch; property: "checked";
107 value: settingsObject.restoreSession
108 }
109 }
110
111 ListItem.Standard {
112 text: i18n.tr("Allow opening new tabs in background")
113 highlightWhenPressed: false
114
115 control: Switch {
116 id: allowOpenInBackgroundTabSwitch
117
118 onClicked: settingsObject.allowOpenInBackgroundTab = checked ? 'true' : 'false';
119 }
120
121 Binding {
122 target: allowOpenInBackgroundTabSwitch; property: "checked";
123 value: settingsObject.allowOpenInBackgroundTab === 'true' ||
124 (settingsObject.allowOpenInBackgroundTab === 'default' &&
125 formFactor === "desktop")
126 }
127 }
128
129 ListItem.Standard {
130 text: i18n.tr("Privacy")
131
132 onClicked: privacyComponent.createObject(subpageContainer);
133 }
134
135 ListItem.Standard {
136 text: i18n.tr("Reset browser settings")
137 onClicked: settingsObject.restoreDefaults();
138 }
139 }
140 }
141
142 Item {
143 id: subpageContainer
144
145 visible: children.length > 0
146 anchors.fill: parent
147
148 Component {
149 id: searchEngineComponent
150
151 Item {
152 id: searchEngineItem
153 anchors.fill: parent
154
155 Rectangle {
156 anchors.fill: parent
157 color: "#f6f6f6"
158 }
159
160 SettingsPageHeader {
161 id: searchEngineTitle
162
163 onBack: searchEngineItem.destroy();
164 text: i18n.tr("Search engine")
165 }
166
167 ListView {
168 anchors {
169 top: searchEngineTitle.bottom
170 left: parent.left
171 right: parent.right
172 bottom: parent.bottom
173 }
174
175 model: searchEngineFolder
176
177 delegate: ListItem.Standard {
178 SearchEngine {
179 id: searchEngineDelegate
180 filename: model.fileBaseName
181 }
182 text: searchEngineDelegate.name
183
184 control: CheckBox {
185 checked: settingsObject.searchEngine == searchEngineDelegate.filename;
186 onClicked: {
187 settingsObject.searchEngine = searchEngineDelegate.filename;
188 searchEngineItem.destroy();
189 }
190 }
191 }
192 }
193 }
194 }
195
196 Component {
197 id: privacyComponent
198
199 Item {
200 id: privacyItem
201 anchors.fill: parent
202
203 Rectangle {
204 anchors.fill: parent
205 color: "#f6f6f6"
206 }
207
208 SettingsPageHeader {
209 id: privacyTitle
210 onBack: privacyItem.destroy();
211 text: i18n.tr("Privacy")
212 }
213
214 Flickable {
215 anchors {
216 top: privacyTitle.bottom
217 left: parent.left
218 right: parent.right
219 bottom: parent.bottom
220 }
221
222 clip: true
223
224 contentHeight: privacyCol.height
225
226 Column {
227 id: privacyCol
228 width: parent.width
229
230 ListItem.Standard {
231 text: i18n.tr("Clear Browsing History")
232 onClicked: historyModel.clearAll();
233 enabled: historyModel.count > 0
234 }
235 }
236 }
237 }
238 }
239 }
240
241 Component {
242 id: homepageDialog
243 Dialog {
244 id: dialogue
245 title: i18n.tr("Homepage")
246
247 Component.onCompleted: {
248 homepageTextField.forceActiveFocus();
249 homepageTextField.cursorPosition = homepageTextField.text.length
250 }
251
252 TextField {
253 id: homepageTextField
254 text: settingsObject.homepage
255 inputMethodHints: Qt.ImhNoAutoUppercase | Qt.ImhNoPredictiveText | Qt.ImhUrlCharactersOnly
256 }
257
258 Button {
259 anchors { left: parent.left; right: parent.right }
260 text: i18n.tr("Cancel")
261 onClicked: PopupUtils.close(dialogue);
262 }
263
264 Button {
265 anchors { left: parent.left; right: parent.right }
266 text: i18n.tr("Save")
267 enabled: UrlManagement.looksLikeAUrl(homepageTextField.text)
268 color: "#3fb24f"
269 onClicked: {
270 settingsObject.homepage = UrlManagement.fixUrl(homepageTextField.text);
271 PopupUtils.close(dialogue);
272 }
273 }
274 }
275 }
276}
277
0278
=== 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 @@
1 /*
2 * Copyright 2015 Canonical Ltd.
3 *
4 * This file is part of webbrowser-app.
5 *
6 * webbrowser-app is free software; you can redistribute it and/or modify
7 * it under the terms of the GNU General Public License as published by
8 * the Free Software Foundation; version 3.
9 *
10 * webbrowser-app is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details.
14 *
15 * You should have received a copy of the GNU General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 */
18import QtQuick 2.0
19import Ubuntu.Components 1.1
20import Ubuntu.Components.ListItems 1.0 as ListItem
21
22/*
23 * Component to use as page header in settings page and subpages
24 *
25 * It has a back() signal fired when back button is pressed and a text
26 * property to set the page title
27 */
28
29Column {
30 id: root
31 signal back()
32 property string text
33
34 height: childrenRect.height
35
36 anchors {
37 left: parent.left
38 right: parent.right
39 }
40
41 Rectangle {
42 id: title
43
44 height: units.gu(7) - divider.height
45 anchors { left: parent.left; right: parent.right }
46
47 Rectangle {
48 anchors.fill: parent
49 color: "#f6f6f6"
50 }
51
52 AbstractButton {
53 id: backButton
54 width: height
55
56 onTriggered: root.back()
57 anchors {
58 top: parent.top
59 bottom: parent.bottom
60 left: parent.left
61 }
62
63 Rectangle {
64 anchors.fill: parent
65 anchors.leftMargin: units.gu(1)
66 anchors.rightMargin: units.gu(1)
67 color: "#E6E6E6"
68 visible: parent.pressed
69 }
70
71 Icon {
72 name: "back"
73 anchors {
74 fill: parent
75 topMargin: units.gu(2)
76 bottomMargin: units.gu(2)
77 }
78 }
79 }
80
81 Label {
82 anchors {
83 left: backButton.right
84 verticalCenter: parent.verticalCenter
85 }
86 text: root.text
87 fontSize: 'x-large'
88 }
89 }
90
91 ListItem.Divider {
92 id: divider
93 }
94}
095
=== 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 @@
222 m_entries.prepend(entry);222 m_entries.prepend(entry);
223 endInsertRows();223 endInsertRows();
224 insertNewEntryInDatabase(entry);224 insertNewEntryInDatabase(entry);
225 Q_EMIT rowCountChanged();
225 } else {226 } else {
226 QVector<int> roles;227 QVector<int> roles;
227 roles << Visits;228 roles << Visits;
@@ -281,6 +282,7 @@
281282
282 removeByIndex(getEntryIndex(url));283 removeByIndex(getEntryIndex(url));
283 removeEntryFromDatabaseByUrl(url);284 removeEntryFromDatabaseByUrl(url);
285 Q_EMIT rowCountChanged();
284}286}
285287
286/*!288/*!
@@ -298,6 +300,7 @@
298 }300 }
299 }301 }
300 removeEntriesFromDatabaseByDomain(domain);302 removeEntriesFromDatabaseByDomain(domain);
303 Q_EMIT rowCountChanged();
301}304}
302305
303void HistoryModel::removeByIndex(int index)306void HistoryModel::removeByIndex(int index)
@@ -363,6 +366,7 @@
363 m_entries.clear();366 m_entries.clear();
364 endResetModel();367 endResetModel();
365 clearDatabase();368 clearDatabase();
369 Q_EMIT rowCountChanged();
366 }370 }
367}371}
368372
369373
=== 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 @@
32 Q_OBJECT32 Q_OBJECT
3333
34 Q_PROPERTY(QString databasePath READ databasePath WRITE setDatabasePath NOTIFY databasePathChanged)34 Q_PROPERTY(QString databasePath READ databasePath WRITE setDatabasePath NOTIFY databasePathChanged)
35 Q_PROPERTY(int count READ rowCount NOTIFY rowCountChanged)
3536
36 Q_ENUMS(Roles)37 Q_ENUMS(Roles)
3738
@@ -64,6 +65,7 @@
6465
65Q_SIGNALS:66Q_SIGNALS:
66 void databasePathChanged() const;67 void databasePathChanged() const;
68 void rowCountChanged();
6769
68private:70private:
69 QSqlDatabase m_database;71 QSqlDatabase m_database;
7072
=== 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 @@
1/*
2 * Copyright 2015 Canonical Ltd.
3 *
4 * This file is part of webbrowser-app.
5 *
6 * webbrowser-app is free software; you can redistribute it and/or modify
7 * it under the terms of the GNU General Public License as published by
8 * the Free Software Foundation; version 3.
9 *
10 * webbrowser-app is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details.
14 *
15 * You should have received a copy of the GNU General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17*/
18'use strict';
19
20function fixUrl(address) {
21 var url = address
22 if (address.substr(0, 1) == "/") {
23 url = "file://" + address
24 } else if (address.indexOf("://") == -1) {
25 url = "http://" + address
26 }
27 return url
28}
29
30function looksLikeAUrl(address) {
31 var terms = address.split(/\s/)
32 if (terms.length > 1) {
33 return false
34 }
35 if (address.substr(0, 1) == "/") {
36 return true
37 }
38 if (address.match(/^https?:\/\//i) ||
39 address.match(/^file:\/\//i) ||
40 address.match(/^[a-z]+:\/\//i)) {
41 return true
42 }
43 if (address.split('/', 1)[0].match(/\.[a-zA-Z]{2,4}$/)) {
44 return true
45 }
46 if (address.split('/', 1)[0].match(/^(?:[0-9]{1,3}\.){3}[0-9]{1,3}/)) {
47 return true
48 }
49 return false
50}
051
=== 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 @@
243 QCOMPARE(model->rowCount(), 0);243 QCOMPARE(model->rowCount(), 0);
244 }244 }
245245
246 void shouldCountNumberOfEntries()
247 {
248 QSignalSpy spyCount(model, SIGNAL(rowCountChanged()));
249 QCOMPARE(model->property("count").toInt(), 0);
250 model->add(QUrl("http://example.org/"), "Example Domain", QUrl());
251 QCOMPARE(model->property("count").toInt(), 1);
252 QCOMPARE(spyCount.count(), 1);
253 model->add(QUrl("http://example.com/"), "Example Domain", QUrl());
254 QCOMPARE(model->property("count").toInt(), 2);
255 QCOMPARE(spyCount.count(), 2);
256 model->clearAll();
257 QCOMPARE(model->property("count").toInt(), 0);
258 QCOMPARE(spyCount.count(), 3);
259 }
260
246};261};
247262
248QTEST_MAIN(HistoryModelTests)263QTEST_MAIN(HistoryModelTests)

Subscribers

People subscribed via source and target branches

to status/vote changes: