Code review comment for lp:~smartgpx/bzr-gtk/workaround_for_lp494140

Revision history for this message
Vincent Ladeuil (vila) wrote :

> Thank you for your attempt to 'mentor' by giving me the opportunity to
> explore this.
>
> I thought it was beyond my gtk/python skills at present. But I've got
> a proposal.

Hehe, obviously you're too humble here since you demonstrate your understanding of the issue below :)

>
> My thinking is as follows. 'self.prev_button.set_menu' expects self to
> be a MenuToolButton, which we should have established by calling
> set_tool_item_type.
> But for PyGtk < 2.10 we haven't done that.

But if you don't encounter the issue, does it matter ?
I mentioned the warning only to give you feedback about why my changes were needed.
If the behavior is fine on your side with them, no need to do *more* changes :)

>
> But to avoid the runtime error we can at least inspect whether we can
> call set_menu
> on the button we actually have...

That's sound, but not needed because we did that earlier.

>
> And we need to do this twice, for prev_button and next_button.
>
> Seems to run Visualize cleanly now.
>

Good. AIUI you may not have the history associated with the 'next' and 'previous' buttons, but that's the price to pay for running an older PyGtk version I'm afraid.

> Did I merge your changes correctly and following correct
> etiquette/attribution?

I have no idea since you didn't push :)

The diff below is still your original one and the associated branch is still at revno 672.

« Back to merge proposal