0001255: use thumbnails of subsection instead of the whole image in person specific views

(0003059)
tgehrig (reporter)
2007-09-27 06:32

I removed the debian part of the patch in gramps-2.2.8-subsection3.patch.bz2

(0003062)
bmcage (administrator)
2007-09-27 08:37

Tobias, can you delete the patch that is not ok anymore? It get's confusing otherwise.

If you make a patch for 3.0 I would probably also apply the 2.2.8 patch, if ok with Brian.

Nice patch, did not apply it, but the code looks good to me. Some quick comments nevertheless:
1/in ImgManip.py, I see twice
w=pixbuf.get_width()
h=pixbuf.get_heigth()
Is that needed?

2/don't give parameter if they are named and same as default value.
So not
ImgManip.get_thumbnail_path(obj.get_path(), None, ph.get_rectangle())
but
ImgManip.get_thumbnail_path(obj.get_path(), rectangle=ph.get_rectangle())
Same in relationview

3/documentation strings. I know you add stuff to some modules that have no or little comment strings, but nevertheless, new code should have it. So every method, class, function, module you add should have the
""" documentation
"""
section. Sorry! Eg in your set_uppery I don't know from reading code if left or right corner.

4/You should not use the type function. See http://www.python.org/dev/peps/pep-0008/ [^]
So you should use IsInstance(Obj,tuple). Anyway, is there a need here to check that? I understand the reasoning, but as you set rectangle yourself in the code as a tuple, if it exists it is a tupple, no? If somebody misuses rectangle, I would prefer an error that is grabbed by the exception handler, instead of the code that keeps working without people knowing something is wrong. Feel free to disagree on this.

(0003063)
tgehrig (reporter)
2007-09-27 09:06

I would like to delete the first patch, but how do I do that? I don't find anything:-/

(0003066)
bmcage (administrator)
2007-09-27 09:58

Ok, I supposed as uploader you could delete it yourself, apparently not.
I did it for you (Delete link is behind the filename for me)

(0003069)
tgehrig (reporter)
2007-09-27 10:50

About your comments:
1.) is not really needed;-). As I said, it was a quick hack;-). I have fixed this here locally for 3.0. I will publish that patch soon. I don't want to do twice the work. So is it okay if I only update the patch for 3.0?
3.) Okay, I hate to do documentation, but I understand that it is necessary:-/.
4.) I just copied what was already in the code. But as you said normally rectangle should always be None or a tuple.

(0003071)
bmcage (administrator)
2007-09-27 11:06

only patch for 3.0 is fine with me.
It is an extensive feature, so if you can life without it for 2.2.x (or only on your box), then only send in 3.0 patch

(0003072)
tgehrig (reporter)
2007-09-27 11:23

Those that want this feature in 2.2.8 probably can also live with the current patch;-).

(0003074)
bmcage (administrator)
2007-09-27 16:32

note that I need your name and email as it must appear in ChangeLog file.
Send it in the patch (eg in ChangeLog) or to me directly.
That is to contact you should we want to sell gramps ;-)

(0003080)
tgehrig (reporter)
2007-09-27 18:25

I have just submitted a new ticket for 3.x here: http://bugs.gramps-project.org/view.php?id=1259 [^]
This should hopefully match your requirements.

(0003091)
bmcage (administrator)
2007-09-28 03:56

We won't add this to 2.2.x, as that branch is in bug-fix only mode.

See issue 0001259 for the 3.0 patch that will be included.

Issue History
Date Modified	Username	Field	Change
2007-09-27 06:25	tgehrig	New Issue
2007-09-27 06:25	tgehrig	File Added: gramps-2.2.8-subsection2.patch.bz2
2007-09-27 06:31	tgehrig	File Added: gramps-2.2.8-subsection3.patch.bz2
2007-09-27 06:32	tgehrig	Note Added: 0003059
2007-09-27 08:37	bmcage	Note Added: 0003062
2007-09-27 09:06	tgehrig	Note Added: 0003063
2007-09-27 09:57	bmcage	File Deleted: gramps-2.2.8-subsection2.patch.bz2
2007-09-27 09:58	bmcage	Note Added: 0003066
2007-09-27 10:50	tgehrig	Note Added: 0003069
2007-09-27 11:06	bmcage	Note Added: 0003071
2007-09-27 11:23	tgehrig	Note Added: 0003072
2007-09-27 16:32	bmcage	Note Added: 0003074
2007-09-27 18:25	tgehrig	Note Added: 0003080
2007-09-28 03:56	bmcage	Status	new => closed
2007-09-28 03:56	bmcage	Note Added: 0003091
2007-09-28 03:56	bmcage	Resolution	open => won't fix
2009-07-04 05:43	romjerome	Relationship added	related to 0002513

Notes
(0003059) tgehrig (reporter) 2007-09-27 06:32	I removed the debian part of the patch in gramps-2.2.8-subsection3.patch.bz2

(0003062) bmcage (administrator) 2007-09-27 08:37	Tobias, can you delete the patch that is not ok anymore? It get's confusing otherwise. If you make a patch for 3.0 I would probably also apply the 2.2.8 patch, if ok with Brian. Nice patch, did not apply it, but the code looks good to me. Some quick comments nevertheless: 1/in ImgManip.py, I see twice w=pixbuf.get_width() h=pixbuf.get_heigth() Is that needed? 2/don't give parameter if they are named and same as default value. So not ImgManip.get_thumbnail_path(obj.get_path(), None, ph.get_rectangle()) but ImgManip.get_thumbnail_path(obj.get_path(), rectangle=ph.get_rectangle()) Same in relationview 3/documentation strings. I know you add stuff to some modules that have no or little comment strings, but nevertheless, new code should have it. So every method, class, function, module you add should have the """ documentation """ section. Sorry! Eg in your set_uppery I don't know from reading code if left or right corner. 4/You should not use the type function. See http://www.python.org/dev/peps/pep-0008/ [^] So you should use IsInstance(Obj,tuple). Anyway, is there a need here to check that? I understand the reasoning, but as you set rectangle yourself in the code as a tuple, if it exists it is a tupple, no? If somebody misuses rectangle, I would prefer an error that is grabbed by the exception handler, instead of the code that keeps working without people knowing something is wrong. Feel free to disagree on this.

(0003063) tgehrig (reporter) 2007-09-27 09:06	I would like to delete the first patch, but how do I do that? I don't find anything:-/

(0003066) bmcage (administrator) 2007-09-27 09:58	Ok, I supposed as uploader you could delete it yourself, apparently not. I did it for you (Delete link is behind the filename for me)

(0003069) tgehrig (reporter) 2007-09-27 10:50	About your comments: 1.) is not really needed;-). As I said, it was a quick hack;-). I have fixed this here locally for 3.0. I will publish that patch soon. I don't want to do twice the work. So is it okay if I only update the patch for 3.0? 3.) Okay, I hate to do documentation, but I understand that it is necessary:-/. 4.) I just copied what was already in the code. But as you said normally rectangle should always be None or a tuple.

(0003071) bmcage (administrator) 2007-09-27 11:06	only patch for 3.0 is fine with me. It is an extensive feature, so if you can life without it for 2.2.x (or only on your box), then only send in 3.0 patch

(0003072) tgehrig (reporter) 2007-09-27 11:23	Those that want this feature in 2.2.8 probably can also live with the current patch;-).

(0003074) bmcage (administrator) 2007-09-27 16:32	note that I need your name and email as it must appear in ChangeLog file. Send it in the patch (eg in ChangeLog) or to me directly. That is to contact you should we want to sell gramps ;-)

(0003080) tgehrig (reporter) 2007-09-27 18:25	I have just submitted a new ticket for 3.x here: http://bugs.gramps-project.org/view.php?id=1259 [^] This should hopefully match your requirements.

(0003091) bmcage (administrator) 2007-09-28 03:56	We won't add this to 2.2.x, as that branch is in bug-fix only mode. See issue 0001259 for the 3.0 patch that will be included.