webbrowser-app

Code review comment for lp:~uriboni/webbrowser-app/search-suggestions

  1. search-suggestions
  2. Merge into trunk
Revision history for this message
Ugo Riboni (uriboni) wrote :

All suggestions implemented except for the following:

> In SearchSuggestions.qml:
> - QQuickItem already has an 'enabled' property, I don’t think it’s necessary
> to override its definition

Item.enabled has a completely different meaning tied to keyboard focus, and re-using it to do something else is wrong (and actually does not ever work since the property is toggled on and off various times by QML itself).
It was my mistake to redefine it though, as the original can't be accessed any more, so I defined a new "active" property that does the same thing, while leaving Item.enabled alone and with its original meaning.

> In test_suggestions.py:
> - in setup_suggestions_source(), you should be able to replace
> "http://localhost:{}/" by "http://test/", as there is automatic mapping of
> test:80 requests to localhost:port

Is this a DNS mapping or a redirect of some sort ? Because it does not seem to work when using XmlHttpRequest from QML to get the list of suggestions. The responseText comes out empty, which is the way XmlHttpRequest signals errors, but I can't tell more details on why.
I spent way too much time on this already, so I will leave it as it was for now.

> - In highlight_term(), this function won’t work if there’s more than one
> occurrence of term in text. This is arguably unimportant for the scope of the
> tests, but the function could be simplified (and made to work in all cases) by
> simply returning "<html>{}</html>".format(re.sub(term, lambda m:
> self.highlight(m.group(0)), text))

This does not work. In some cases I get a complain from re.py of "unbalanced parenthesis". Unless you have a quick fix I would leave it as it is.

« Back to merge proposal