[Merge] lp:~rpadovani/webbrowser-app/settings-page into lp:webbrowser-app
Riccardo Padovani
riccardo at rpadovani.com
Fri Mar 27 02:17:43 UTC 2015
> - 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?
--
https://code.launchpad.net/~rpadovani/webbrowser-app/settings-page/+merge/253975
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list