[Merge] lp:~uriboni/webbrowser-app/topsite-previews into lp:webbrowser-app

Ugo Riboni ugo.riboni at canonical.com
Wed Oct 7 21:20:58 UTC 2015


> A first incomplete round of (mostly) functional review, with comments in no
> particular order:

Fixed unless noted below:

> In UrlPreviewDelegate.hide_from_history(), the comment mentions bug #1205144,
> which has been fixed in the UITK, so the hack can now be cleaned up. Same in
> test_new_tab_view.py.

We still can't use the CPO's method to click on an action because of another bug.
We can however get the action button by objectName, so I fixed that at least.
 
> I don’t think this is a bug introduced by your changes, rather an issue that’s
> always been there, but it is now more visible: initially, the captures for my
> top sites grid are blank (expected). If from there I click on one top site to
> open it, wait for the page to load, then close the tab, and open a new tab
> again, there is no capture for the page that I just viewed. This is because a
> capture is not made when closing a tab (only when switching tabs). Not sure
> how much work this would require to implement, but worth taking a look anyway.

The other case when the preview is not captured is when we navigate somewhere else, and while we might be able to delay the unloading of the tab, I don't know if we can't easily delay navigating away on a link click.

This idea of delaying hiding the tab is also making me a bit nervous anyway. Would it be too expensive CPU-wise to periodically call grabToImage without saving to disk, and saving to disk only when the tab stops being visible or the URL changes ?
 
> On a related note, can captures be removed when closing tabs? I thought this
> was implemented already, but it doesn’t seem to work.

It is implemented and I tested it again and it works here.
Can you give me steps to reproduce this problem ?
Please keep in mind that when closing a tab, if the site is in the top sites its preview will not be deleted.

> The context menu that appears on grid items when long-pressing/right-clicking
> can be dismissed when clicked outside, but the click is propagated to
> whatever’s underneath, so if I click anywhere to dismiss the menu I might
> activate a top site.

This sounds like a bug in the ActionSelectionPopover not setting up the exclusion area correctly.
I will investigate more and file a bug.
 
> Not a big deal as (AFAIK) keyboard navigation was never implemented in narrow
> views, but since the grid view handles it correctly, can the narrow new tab
> view be fixed to fully support keyboard nav? If it would require too many
> changes, I’m fine with leaving it for another branch.

The real problem with the narrow mode version is that it uses a Column+Repeater setup. I plan to convert that into a proper ListView to get better performance, and keyboard navigation will work almost "for free" after that.
So let's keep it for that other MR.
-- 
https://code.launchpad.net/~uriboni/webbrowser-app/topsite-previews/+merge/269771
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list