Code review comment for lp:~rpadovani/webbrowser-app/settings-page

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?

« Back to merge proposal