Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(796)

Issue 184410043: initial version of InteractiveImageMap.py

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 3 months ago by andreas.bihlmaier
Modified:
9 years, 3 months ago
Reviewers:
thomas.j.waldmann
Visibility:
Public.

Description

initial 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -27 lines) Patch
M InteractiveImageMap.py View 1 2 7 chunks +23 lines, -27 lines 0 comments Download

Messages

Total messages: 3
Thomas.J.Waldmann
some first feedback, thanks for putting it on codereview site. if you have worked on ...
9 years, 3 months ago (2014-12-13 17:19:18 UTC) #1
Thomas.J.Waldmann
more feedback https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py File InteractiveImageMap.py (right): https://codereview.appspot.com/184410043/diff/20001/InteractiveImageMap.py#newcode5 InteractiveImageMap.py:5: mouse-over information and on click dynamic change ...
9 years, 3 months ago (2014-12-20 13:52:43 UTC) #2
andreas.bihlmaier
9 years, 3 months ago (2014-12-23 12:48:02 UTC) #3
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('&lt;&lt;BR&gt;&gt;',
'<<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 &lt;...
> 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b