Code review comment for lp://qastaging/~jnuzman/simple-scan/bug-843361

Revision history for this message
Joseph Nuzman (jnuzman) wrote :

OK, looks good.

As to the other changes:

Moving existing page add to end of BookView constructor: looks problematic
to me to populate the page_data hash and then a few lines later initialize
page_data with with "new".

Emitting cleared signal before destroying pages list in Book.clear(): I
wanted to ensure the pages were still around to disconnect from when the
PageView destructor is called, but I guess since the PageView holds a
reference to the Page, it shouldn't be an issue.

Joe

On Thu, Jul 12, 2012 at 8:46 AM, Robert Ancell
<email address hidden>wrote:

> Hi, thanks for this.
>
> I've committed the signal disconnections along with some other ones. I'm a
> little wary of committing the other changes as I'm not sure why they fix
> the problem but am investigating the code around these.
> --
> https://code.launchpad.net/~jnuzman/simple-scan/bug-843361/+merge/113867<https://code.launchpad.net/%7Ejnuzman/simple-scan/bug-843361/+merge/113867>
> You are the owner of lp:~jnuzman/simple-scan/bug-843361.
>

« Back to merge proposal