[Sugar-devel] [PATCH Get Books] Port to Gtk3 SL #3681

[Manuel Kaufmann]

On Tue, Jun 19, 2012 at 3:56 PM, Gonzalo Odiard <gonzalo at laptop.org> wrote:
> The short version is: please, don't add more changes to the needed by the
> port.
> If you find a bug, or anything wrong with the code, is very good, but is
> better include it
> in another patch. We can decide if have sense apply in the old branch too.

Ok, some of them have an explanation. They are not added just because.
I will explain this below.

> Also, pleeeease :) don't change spaces if not needed. In particular, the
> second line,
> when continue after the 80 chars, I prefer use 8 chars indentation. I am not
> sure if there are a agreed way here, but if not, better keep the code as is.

About spaces. There is a convention[1] used that says we should use 4
chars for indentation (in Python Code): PEP8

> Is better if you can put here the list of non working things.

 * Cancel search button. It shows a really big X when you start
typing: http://bugs.sugarlabs.org/ticket/3385

 * Clicking some things on the screen makes the ListView grows a bit
and it doesn't fit on the screen (the right scroll-bar goes outside
the window)

 * Some time when I click on the Search Entry all the toolbar is moved a bit

 * [ALTERNATIVE] When the "generic book image" is shown (at the bottom
left), the gray color used to fill it is not the same as the
background. I changed the color with is the GdkPixbuf filled to match
with the darker one (style.COLOR_BUTTON_GREY) because I couldn't find
how to fill the alpha of the "generic_cover.png" with the old color
(style.COLOR_PANEL_GREY)

 * After searching and selecting a book, if we change the format to
EPUB the Get Book button is moved a bit

 * When the book download starts, a Cancel is shown but it doesn't
have the X icon

 * The background of the rows on the ListView do not cycle between
white and gray

 * The Catalogs column title is bigger than the other column titles
(this was working on this way but I think it's wrong)

 * The column width should be re-sized to fit with the text inside
them with a max width pre-set

 * Right clicking on the column titles doesn't show the Gtk.Menu popup

 * [IMPORTANT] I'm not sure what is org.freedesktop.UDisks.Device for?
I don't understand it very well but I had to remove it because it was
failing

>> [1] http://wiki.sugarlabs.org/go/User:Humitos/PortingGetBooks
> The wiki page looks outdated in parts. Please update it.

What are the things that are outdated?

> This is a example of changing indentation/spaces:

I will keep this in mind.

> This is a change, looks ok, but better in another patch:

[...]

> Same here:

[...]

The fixs that you mentioned above are not present in the gtk2 version
of the Activity. I mean, I didn't fix an old bug here, in fact, I
fixed the bugs that appeared after the migration. I tried to get the
same behaviour on both version without adding and removing anything. I
think this is related on how are the signals emitted between gtk2 and
Gtk3 and that is why I wrote that comment.

> About all this pixbuf code, we need see if is better use cairo, but is ok do
> it in another patch:

Yes, I agree with you. I think it should go in another patch.

> Probably the problem in devicemanager.py is you are using
>
> import dbus
> and not
> from gi.repository import DBus

OK. I will take a look at this. Do you have an example about how to use this?

from gi.repository import DBus

>> --- a/extListview.py
>> +++ b/extListview.py
>
>
> I am ok with removing the code we don't use here.
> In the future, I would like use a standard Gtk widget.

Yes. I think this class was used for another purpose and it's bigger
than our necessity.

> Testing the Gtk3 version, I have found the following error:
> When I click in a book title in the listview, the information about the book
> should be displayed in the textview at the botton of the screen. In the Gtk3
> version, nothing is displayed. In the log I see:

Yes. This is working on the English version of the Activity. I will
take a look a this, but I didn't change anything related with this.

> I wait a new patch, excuse me by the late review.

OK. You will receive a new patch ;)

See you and thanks for this review!

[1] http://www.python.org/dev/peps/pep-0008/

-- 
Kaufmann Manuel
Blog: http://humitos.wordpress.com/
Porfolio: http://fotos.mkaufmann.com.ar/
PyAr: http://www.python.com.ar/