Issue 10599043: Fixes resizing of transclusions
DescriptionFixes resizing of transclusions
Patch Set 1 : Test cr to verify the method #Patch Set 2 : Fixes resizing with transclusions #
Total comments: 17
Patch Set 3 : More pythonic, some fixes, more defensive #
Total comments: 12
Patch Set 4 : Fix for passing attributes everytime, more pythonic #
Total comments: 5
Patch Set 5 : Fix for some ugly things #
Total comments: 21
Patch Set 6 : Fixes for argument passing, html ns used #
Total comments: 5
Patch Set 7 : More improvements #
MessagesTotal messages: 16
some comments https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:17: import urlparse that import should be on top of the imports and have a look if there is also one in werkzeug https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include.py File MoinMoin/converter/include.py (left): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include... MoinMoin/converter/include.py:339: why do you remove that line in the same cs?
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/html_ou... File MoinMoin/converter/html_out.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:358: attrib[html.height] = elem.get(xlink.height) attrib_whitelist = { xlink.width: html.width, xlink.height: html.height, } for orig, new in attrib_whitelist.iteritems(): try: attrib[new] = elem[orig] except KeyError: pass https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include.py File MoinMoin/converter/include.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include... MoinMoin/converter/include.py:339: page_doc = page.content.internal_representation(elem=elem) I don't like passing the whole element here, you only need to pass the selected attributes. https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:837: for attr in args: for attr, value in args.iteritems(): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:838: if attr[0] != '&': if not attr.startswith('&'): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:839: actual_attr[attr] = args[attr] actual_attr[attr] = value https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:841: actual_args[attr[1:]] = args[attr] drop the not from the condition and swap the blocks https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py#... MoinMoin/items/content.py:180: def internal_representation(self, converters=['smiley'], elem=None): don't pass the whole element, just pass the attributes that matter https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py#... MoinMoin/items/content.py:207: doc = input_conv(self.rev, self.contenttype, elem=elem) input_conv already has arguments parameter that you could use. No need for this check then.
Sign in to reply to this message.
Added replies. All changes fixed in Patch Set 3. https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/html_ou... File MoinMoin/converter/html_out.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:358: attrib[html.height] = elem.get(xlink.height) elem is not a dict, i changed according to it being an Element tree instance. On 2013/06/26 20:50:02, TheSheep wrote: > attrib_whitelist = { > xlink.width: html.width, > xlink.height: html.height, > } > for orig, new in attrib_whitelist.iteritems(): > try: > attrib[new] = elem[orig] > except KeyError: > pass https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:17: import urlparse found one in werkzeug :) On 2013/06/26 20:38:44, ReimarBauer wrote: > that import should be on top of the imports > > and have a look if there is also one in werkzeug https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include.py File MoinMoin/converter/include.py (left): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include... MoinMoin/converter/include.py:339: corrected. On 2013/06/26 20:38:44, ReimarBauer wrote: > why do you remove that line in the same cs? https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include.py File MoinMoin/converter/include.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/converter/include... MoinMoin/converter/include.py:339: page_doc = page.content.internal_representation(elem=elem) ok, passed elem.attrib On 2013/06/26 20:50:02, TheSheep wrote: > I don't like passing the whole element here, you only need to pass the selected > attributes. https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py#... MoinMoin/items/content.py:207: doc = input_conv(self.rev, self.contenttype, elem=elem) I think we should not remove this check because, there are some other cases which will trigger this from the include converter such as " {{ localpage }} " .. here use of height and width wont make any sense, but these and more might be still passed into the respective converters as 'arguments' .. and can cause unexpected behaviour. What do you think? On 2013/06/26 20:50:02, TheSheep wrote: > input_conv already has arguments parameter that you could use. > No need for this check then.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py#... MoinMoin/items/content.py:207: doc = input_conv(self.rev, self.contenttype, elem=elem) Maybe check line 144, moinwiki converter.. for a possible unexpected use of 'arguments' .. On 2013/06/26 20:50:02, TheSheep wrote: > input_conv already has arguments parameter that you could use. > No need for this check then.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/16002/MoinMoin/items/content.py#... MoinMoin/items/content.py:207: doc = input_conv(self.rev, self.contenttype, elem=elem) Yes, it's using them exactly for what we want to use it here -- passing additional attributes to be put in the representation.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/html_ou... File MoinMoin/converter/html_out.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:360: if elem.get(orig) is not None: if orig in elem: https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:363: pass This is completely unneeded. https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:40: pass Check explicitly for None, not for side effects of having None like this. What if some other code that you have there raises AttributeError? You will spend years trying to find the bug. https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:46: xlink.height: height, What if width and height are not specified? https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:50: attrib[xlink.href].query = url_encode(query, charset=CHARSET, encode_keys=True) If you move that to line 40, then you can just pass query to the Iri constructor directly. https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/include.py File MoinMoin/converter/include.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/include... MoinMoin/converter/include.py:340: page_doc = page.content.internal_representation(attrib=elem.attrib) please use English words as parameter names when possible https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:843: Won't that treat width and &width the same way? https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:857: attrib.update(html_attr) attrib = { xinclude.href: target, xinclude.width : actual_attr.get('width'), xinclude.height : actual_attr.get('height'), } https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:869: attrib.update(html_attr) ditto https://codereview.appspot.com/10599043/diff/35001/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/items/content.py#... MoinMoin/items/content.py:209: doc = input_conv(self.rev, self.contenttype) always pass arguments
Sign in to reply to this message.
Fix for passing attributes everytime, more pythonic. I'm currently looking up for other converters, they are mostly similar, if its ok, i'll put in that too for review https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:46: xlink.height: height, If the width and height are not specified, it remains None, this is handled in the html emitter. It passes only valid stuff. On 2013/06/27 07:11:08, TheSheep wrote: > What if width and height are not specified? https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/35001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:843: it is just that the one's with '&' go into the actual_args var. whereas the others go in the actual_attr, which have been handled differently below. On 2013/06/27 07:11:08, TheSheep wrote: > Won't that treat width and &width the same way? https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:1038: if arguments and hasattr(arguments, 'keyword'): This change is because the arguments which we pass in some cases from the include converter, is a dict and does not have the attribute 'keyword'. Moreover, in the next line arguments.keyword is being used, which IMO is optimistic if we check only the existence of arguments.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:40: if attr.has_key(orig.name): use in instead of has_key
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:33: actual_query = '' query https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:41: attr[orig.name] = new maybe iterate over attr, not arguments? https://codereview.appspot.com/10599043/diff/50001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:46: query = url_encode(query, charset=CHARSET, encode_keys=True) query_keys = url_decode(actual_query) query_keys.update(do='get', rev=rev.revid) query = url_encode(query_keys, charset=CHARSET, encode_keys=True)
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/html_ou... File MoinMoin/converter/html_out.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:360: if orig in elem.attrib and elem.get(orig): are you expecting empty values or why do you check "and elem.get(orig)" ? why is it checking "in" elem.attrib and then getting from "elem"? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:36: 'height': None, why is one '' and the other None if not given? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:43: attr['query'] = arguments.get(xinclude.href).query reduce code duplication in these 2 lines. https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:46: query_keys.update({'do':'get', 'rev':rev.revid}) did you test if this practically works? i mean: is this url correct and does it get handled? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:54: xlink.height: attr['height'], that feels wrong. xlink doesn't have width and height, right? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:837: actual_attr = {} if the first is plural, the second should be also. is that "actual_" part of the name helpful? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:855: xinclude.height : actual_attr.get('height'), hmm, btw: how about the "t=..." value for the transformation (rotate, mirror, ...)? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:866: xlink.height : actual_attr.get('height'), xlink doesn't have width/height either, feels unclean. https://codereview.appspot.com/10599043/diff/62001/MoinMoin/items/content.py File MoinMoin/items/content.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/items/content.py#... MoinMoin/items/content.py:180: def internal_representation(self, converters=['smiley'], attributes=None): needs docstring update
Sign in to reply to this message.
Added replies to all comments. https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/html_ou... File MoinMoin/converter/html_out.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/html_ou... MoinMoin/converter/html_out.py:360: if orig in elem.attrib and elem.get(orig): Yes, if no value is entered we have None, and needs to be handled so that it doesnt get into the HTML. elem is an emerald tree element and .get for that has been written like that i think. elem.attrib is a dict, so "in" works On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > are you expecting empty values or why do you check "and elem.get(orig)" ? > > why is it checking "in" elem.attrib and then getting from "elem"? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:36: 'height': None, Finally have to use url_decode on the query, which does not accept None. On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > why is one '' and the other None if not given? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:46: query_keys.update({'do':'get', 'rev':rev.revid}) Yes, works, as we discussed, the user needs to enter "&h=10" for instance to trigger the PIL. On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > did you test if this practically works? > > i mean: is this url correct and does it get handled? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:54: xlink.height: attr['height'], yeah, doesnt have. asked waldi for suggestions. On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > that feels wrong. xlink doesn't have width and height, right? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:837: actual_attr = {} Hmm, initially, everything is args, like "&width=100,height=50". actual_args finally deals with only "&width=100". similarly actual_attr deals with "height=50". On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > if the first is plural, the second should be also. > > is that "actual_" part of the name helpful? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:855: xinclude.height : actual_attr.get('height'), Hmm, short addition, maybe add a different patchset. On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > hmm, btw: how about the "t=..." value for the transformation (rotate, mirror, > ...)? https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:866: xlink.height : actual_attr.get('height'), asked waldi for suggestions. On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > xlink doesn't have width/height either, feels unclean.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:43: attr['query'] = arguments.get(xinclude.href).query Is calculating "arguments.get(xinclude.href).query" prior to check and assign ok? On 2013/06/28 14:57:24, Thomas.J.Waldmann wrote: > reduce code duplication in these 2 lines.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:855: xinclude.height : actual_attr.get('height'), Please quote the xinclude spec for the width and height attribute. Otherwise use the html namespace, if this attribute matches the semantic and syntax spcified. https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:1038: if arguments and hasattr(arguments, 'keyword'): Please elaborate. parse_block should never get something different. If this is a bug-fix, do it separate.
Sign in to reply to this message.
Added reply for change to parse_block. https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:1038: if arguments and hasattr(arguments, 'keyword'): To specify the attributes like width and height at the time of creation of the image, we passed the attributes (like width, height) to internal representation code @ https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/include.py The initialiser passes it here, and since it is just a dict, doesnt have the attribute 'keyword', hence i performed a check here. On 2013/06/29 14:14:25, waldi wrote: > Please elaborate. parse_block should never get something different. If this is a > bug-fix, do it separate.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/62001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:1038: if arguments and hasattr(arguments, 'keyword'): On 2013/06/29 15:24:24, sharky93 wrote: > To specify the attributes like width and height at the time of creation of the > image, we passed the attributes (like width, height) to internal representation This is nice, but a bug in the caller. Don't do that.
Sign in to reply to this message.
https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/image_i... File MoinMoin/converter/image_in.py (right): https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:37: } why did you put query in here? it doesn't seem to belong https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/image_i... MoinMoin/converter/image_in.py:42: attr['query'] = arguments.keyword.get(xinclude.href).query or '' why do you repeat that in the loop? https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/include.py File MoinMoin/converter/include.py (right): https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/include... MoinMoin/converter/include.py:342: page_doc = page.content.internal_representation(attributes=Arguments(keyword = elem.attrib)) no spaces around = https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/moinwik... File MoinMoin/converter/moinwiki_in.py (right): https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:856: } how about just using attrib all along instead of actual_attrs, and doing attrib[xinclude.href] = target here? https://codereview.appspot.com/10599043/diff/82001/MoinMoin/converter/moinwik... MoinMoin/converter/moinwiki_in.py:867: } ditto
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
