Issue 184410043: initial version of InteractiveImageMap.py
|
Created:
10 years, 1 month ago by andreas.bihlmaier Modified:
10 years, 1 month ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
Descriptioninitial version of InteractiveImageMap.py
Patch Set 1 #
Total comments: 42
Patch Set 2 : corrections according to comments by Thomas.J.Waldmann #
Total comments: 29
Patch Set 3 : fix issues pointed out in patch set 2 #MessagesTotal messages: 3
some first feedback, thanks for putting it on codereview site. if you have worked on the issues and improved the source, please upload your work using upload.py and give the SAME id 184410043, to it updates THIS codereview (not creating a new CR). https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py File InteractiveImageMap.py (right): https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode2 InteractiveImageMap.py:2: Description missing. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode15 InteractiveImageMap.py:15: @copyright: 2014 by Andreas Bihlmaier license missing. as this links to moin code, it must be same license as moin (see license string in any moin file). https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode18 InteractiveImageMap.py:18: from __future__ import print_function no need. neither for print nor for python 3 compatibility (moin 1.9 only runs with 2.x and that will never change). https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode27 InteractiveImageMap.py:27: remove empty line https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode37 InteractiveImageMap.py:37: return '://' in text hmm, looks familiar, do we have that somewhere in moin? can you import it from there? https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode41 InteractiveImageMap.py:41: class Parser(ParserBase): docstring missing? also for most other classes/funcs. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode54 InteractiveImageMap.py:54: self.request.write(formatter.rawHTML(output_msg)) always indent by multiples of 4 blanks. please configure your editor to always do that for .py files (and never use tabs). to really find all formatting issues, I suggest you run the pep8 checker on your source code. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode55 InteractiveImageMap.py:55: except: what exception do you want to catch here? https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode61 InteractiveImageMap.py:61: splitters = line.split(';;') "items" maybe? https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode67 InteractiveImageMap.py:67: key, val = keyval[0], keyval[1] key, val = keyval https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode73 InteractiveImageMap.py:73: unescaped_text = text.replace('<<BR>>', '<<BR>>') huh? https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode83 InteractiveImageMap.py:83: html = ''' as a general comment: building up a large piece of html iteratively with += is a bit unpretty. you could try to use some templates that better show the whole picture and have placeholders for the variable parts, then compute the variable parts and put them into the template. if needed, also use a template for the smaller parts. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode86 InteractiveImageMap.py:86: ''' rather omit the http: - if your wiki runs on https, it also should use https: for the js (otherwise browser might warn about only partially encrypted content). of course that means it needs to serve the js also with https. so src="//cnd....". https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode93 InteractiveImageMap.py:93: if not 'name' in image_dict: correct. for better reading, python also allows to say: if 'name' not in image_dict: ... https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:114: image_dict.pop('width') you can do both at once: image_width = image_dict.pop('width') https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:120: return self.fail(formatter, 'picsrc contains excess arguments') hmm, excess or excessive? https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:123: html += ' usemap="#%s">' % map_name maybe instead of generating this with += .. you could first have a k/v dict and then use ' '.join("%s=%s" % (k,v) for k, v in d.iteritems()). https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:197: </script> maybe you could move this bigger string piece from the middle of the function to some global module level string constant. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:208: except: which exception exactly are you catching? "except:" (also known as "diaper [anti]pattern") is evil and should be only used in very specific circumstances. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:209: self.request.write(formatter.image(TODO)) TODO? https://codereview.appspot.com/184410043/diff/1/example.wiki File example.wiki (right): https://codereview.appspot.com/184410043/diff/1/example.wiki#newcode4 example.wiki:4: turltebot_base;;shape=poly;;coords=16,240,17,292,33,308,60,320,91,326,124,329,156,329,194,325,226,315,249,297,253,287,254,242,233,224,54,215,28,228,16,240;;tooltip=Turtlebot base plattform;;description=See [[http://wiki.ros.org/turtlebot|ROS wiki]] typo "turlte" https://codereview.appspot.com/184410043/diff/1/example.wiki#newcode7 example.wiki:7: After attaching turltebot_2_lg.png to this page, the result should look and work like [[http://andreasbihlmaier.github.io/html/2014-10-30-ros_beginner_documentation.html]]. same typo
Sign in to reply to this message.
more feedback https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py File InteractiveImageMap.py (right): https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:5: mouse-over information and on click dynamic change of description box on 2 blanks: "and on" https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:38: <script src="//cdn.jsdelivr.net/jquery/1.11.1/jquery.min.js"></script> maybe use the normal jquery cdn? 1.11.2? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:39: <script src="//andreasbihlmaier.github.io/js/jquery.imagemapster.min.js"></script> is that url expected to be long-time stable? does the original author of imagemapster also serve the file at a location that is expected to be used from 3rd parties? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:41: <img id="%(image_id)s" src="%(imgurl)s" width=%(image_width)s usemap="#%(map_name)s"> image_url, for consistency? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:47: <div style="text-align: left; clear: both; width: %(image_width)spx; height: 200px; border: 1px solid black;" id="description"></div> if you have multiple interactive image maps on 1 wiki page, will this lead to duplicate IDs? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:74: //onMouseover: function (e) { is this needed? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:83: //toolTipClose: ["tooltip-click", "area-click"], is this needed? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:115: except: # TODO catch non-HTML formatter all places that call fail() just output simple text. why do you need a rawHTML formatter for that? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:119: """Return ';;' seperated 'key=val' tupples as dict.""" separated ... tuples https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:123: for splitter in items[1:]: for item in items: note the general pattern: for <singular> in <plural> - this makes nice to read and consistent code https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:128: d[wikiutil.escape(key)] = wikiutil.escape(val) of course you need to escape the values to avoid XSS when you put the values into html, but do you also put the keys into html? https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:136: unescaped_text = text.replace('<<BR>>', '<<BR>>') i don't understand why you are doing this. the description may be wiki markup, so <<BR>> could be in there, but <... rather looks like html. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:142: return buf.getvalue().replace('\n', ' ') why replace \n by blank? in html, this should not matter. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:220: self.request.write(formatter.rawHTML(html)) did you check all places where you put user-provided strings into html whether you escape them properly? if you forget one place, there will be an XSS security issue. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:221: except: # TODO catch non-HTML formatter try to be more specific with exception catching, move write() out of try/except.
Sign in to reply to this message.
I'm sorry forgot to "publish + mail" my first comments. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py File InteractiveImageMap.py (right): https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode2 InteractiveImageMap.py:2: On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > Description missing. Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode15 InteractiveImageMap.py:15: @copyright: 2014 by Andreas Bihlmaier On 2014/12/13 17:19:16, Thomas.J.Waldmann wrote: > license missing. as this links to moin code, it must be same license as moin > (see license string in any moin file). Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode18 InteractiveImageMap.py:18: from __future__ import print_function On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > no need. neither for print nor for python 3 compatibility (moin 1.9 only runs > with 2.x and that will never change). Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode27 InteractiveImageMap.py:27: On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > remove empty line Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode37 InteractiveImageMap.py:37: return '://' in text On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > hmm, looks familiar, do we have that somewhere in moin? can you import it from > there? This is contained in quite a few other macros and plugins, but I could not find it within MoinMoin core. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode41 InteractiveImageMap.py:41: class Parser(ParserBase): On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > docstring missing? also for most other classes/funcs. Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode54 InteractiveImageMap.py:54: self.request.write(formatter.rawHTML(output_msg)) On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > always indent by multiples of 4 blanks. > > please configure your editor to always do that for .py files (and never use > tabs). > > to really find all formatting issues, I suggest you run the pep8 checker on your > source code. How important is "E501 line too long" to you? I think it is prettier not to break the html markup just to have short lines. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode55 InteractiveImageMap.py:55: except: On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > what exception do you want to catch here? see reply at end of file. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode61 InteractiveImageMap.py:61: splitters = line.split(';;') On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > "items" maybe? Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode67 InteractiveImageMap.py:67: key, val = keyval[0], keyval[1] On 2014/12/13 17:19:16, Thomas.J.Waldmann wrote: > key, val = keyval Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode73 InteractiveImageMap.py:73: unescaped_text = text.replace('<<BR>>', '<<BR>>') On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > huh? Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode83 InteractiveImageMap.py:83: html = ''' On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > as a general comment: building up a large piece of html iteratively with += is a > bit unpretty. > > you could try to use some templates that better show the whole picture and have > placeholders for the variable parts, then compute the variable parts and put > them into the template. if needed, also use a template for the smaller parts. Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode86 InteractiveImageMap.py:86: ''' On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > rather omit the http: - if your wiki runs on https, it also should use https: > for the js (otherwise browser might warn about only partially encrypted > content). > > of course that means it needs to serve the js also with https. > > so src="//cnd....". Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcode93 InteractiveImageMap.py:93: if not 'name' in image_dict: On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > correct. for better reading, python also allows to say: > > if 'name' not in image_dict: ... Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:114: image_dict.pop('width') On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > you can do both at once: > > image_width = image_dict.pop('width') Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:120: return self.fail(formatter, 'picsrc contains excess arguments') On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > hmm, excess or excessive? According to http://www.dict.cc/?s=excess excess is correct for "überzählig". https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:123: html += ' usemap="#%s">' % map_name On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > maybe instead of generating this with += .. you could first have a k/v dict and > then use ' '.join("%s=%s" % (k,v) for k, v in d.iteritems()). Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:197: </script> On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > maybe you could move this bigger string piece from the middle of the function to > some global module level string constant. Acknowledged. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:208: except: On 2014/12/13 17:19:16, Thomas.J.Waldmann wrote: > which exception exactly are you catching? > > "except:" (also known as "diaper [anti]pattern") is evil and should be only used > in very specific circumstances. see below. https://codereview.appspot.com/184410043/diff/1/InteractiveImageMap.py#newcod... InteractiveImageMap.py:209: self.request.write(formatter.image(TODO)) On 2014/12/13 17:19:17, Thomas.J.Waldmann wrote: > TODO? This was copied from http://moinmo.in/ParserMarket/ImageMap?action=AttachFile&do=view&target=Image... I did not quite understand what should be done in case current formatter is not an HTML formatter https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py File InteractiveImageMap.py (right): https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:38: <script src="//cdn.jsdelivr.net/jquery/1.11.1/jquery.min.js"></script> On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > maybe use the normal jquery cdn? 1.11.2? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:39: <script src="//andreasbihlmaier.github.io/js/jquery.imagemapster.min.js"></script> On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > is that url expected to be long-time stable? > > does the original author of imagemapster also serve the file at a location that > is expected to be used from 3rd parties? Unfortunately, it was necessary to add a small patch to imagemapster (https://github.com/jamietre/ImageMapster was not updated for a year). In the long term the modified version must be moved to a better place. Since I'm writing this mainly for wiki.ros.org, once accepted for inclusion, I'd change the URL to something located there. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:41: <img id="%(image_id)s" src="%(imgurl)s" width=%(image_width)s usemap="#%(map_name)s"> On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > image_url, for consistency? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:47: <div style="text-align: left; clear: both; width: %(image_width)spx; height: 200px; border: 1px solid black;" id="description"></div> On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > if you have multiple interactive image maps on 1 wiki page, will this lead to > duplicate IDs? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:74: //onMouseover: function (e) { On 2014/12/20 13:52:42, Thomas.J.Waldmann wrote: > is this needed? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:83: //toolTipClose: ["tooltip-click", "area-click"], On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > is this needed? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:115: except: # TODO catch non-HTML formatter On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > all places that call fail() just output simple text. why do you need a rawHTML > formatter for that? Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:119: """Return ';;' seperated 'key=val' tupples as dict.""" On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > separated ... tuples Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:123: for splitter in items[1:]: On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > for item in items: > > note the general pattern: > > for <singular> in <plural> - this makes nice to read and consistent code Acknowledged. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:128: d[wikiutil.escape(key)] = wikiutil.escape(val) On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > of course you need to escape the values to avoid XSS when you put the values > into html, but do you also put the keys into html? In fact, the keys do not make it into the html. However, I wanted to be sure that even if they did (or will in the future due to adding functionality), I won't be bitten by injections. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:136: unescaped_text = text.replace('<<BR>>', '<<BR>>') On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > i don't understand why you are doing this. > > the description may be wiki markup, so <<BR>> could be in there, but <... > rather looks like html. Input text was already run through wikiutil.escape(), which hinders people from injecting unwanted strings into the final page. Yet, the sequence "<<BR>>" (and only this one) should be passed literally to text_moin_wiki.Parser() in order to be interpreted. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:142: return buf.getvalue().replace('\n', ' ') On 2014/12/20 13:52:42, Thomas.J.Waldmann wrote: > why replace \n by blank? in html, this should not matter. It does matter in the javascript part, i.e. var description_map_turtlebot_2_lgpng = { kinect: '<span class="anchor" id="line-1-3"></span><p class="line862">Driver: <a class="http" href="http://wiki.ros.org/openni_camera">openni_camera</a> <br> Data: <a class="http" href="http://docs.ros.org/api/sensor_msgs/html/msg/PointCloud2.html">sensor_msgs/PointCloud2</a> <br> Visualization: <a class="http" href="http://wiki.ros.org/rviz/DisplayTypes/PointCloud">rviz PointCloud</a> <br> Processing: <a class="http" href="http://wiki.ros.org/pcl">Point Cloud Library (PCL)</a> ',turltebot_base: '<span class="anchor" id="line-1-4"></span><p class="line862">See <a class="http" href="http://wiki.ros.org/turtlebot">ROS wiki</a> ', }; does not work because of the newline in the '' delimited string. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:220: self.request.write(formatter.rawHTML(html)) On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > did you check all places where you put user-provided strings into html whether > you escape them properly? if you forget one place, there will be an XSS security > issue. I did check twice. https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#ne... InteractiveImageMap.py:221: except: # TODO catch non-HTML formatter On 2014/12/20 13:52:43, Thomas.J.Waldmann wrote: > try to be more specific with exception catching, move write() out of try/except. Acknowledged.
Sign in to reply to this message.
|
