[Merge] lp:~artmello/webbrowser-app/webbrowser-app-format_top_sites into lp:webbrowser-app

Arthur Mello arthur.mello at canonical.com
Tue Apr 14 22:02:45 UTC 2015


>  - The fix to src/app/webbrowser/history-model.cpp (insertStatement) should go
> in the prerequisite branch (lp:~artmello/webbrowser-app/webbrowser-app-
> remove_top_sites), not this one.

Fixed

>  - I’m not sure I understand the new unit test
> (shouldUpdateRowCountAndNotifyAfterAnEntryIsRemoved): when removing the first
> row, I would expect a rowsRemoved signal to be emitted for the first row, and
> a rowsInserted signal to be emitted for the new row that comes into view. Is
> this not how the limit proxy model works?

I think, based on comments/code starting at line 210 of limit-proxy-model.cpp, that if we have like 7 bookmark entries (with a limit of 5) and remove a single entry, the model will not emit rowsRemoved and will only emit dataChanged. If I understood the code correct, it will keep returning 5 entries but now with different values for some of them (depending of the position of the one you removed). The unittest is checking that the underlying model is reporting that a row is removed and that the limit-proxy-model is only emitting dataChanged. I thought that the limit proxy model was emitting rowsRemoved and not the rowsInserted and that could be the reason why the UI is not correctly updated, but it doesn't seem to be the case.
-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-format_top_sites/+merge/255298
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list