Skip to content

Modified class for additional functionality #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Modified class for additional functionality #4

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d19189
Modified class for additional functionality
maojrs Jul 26, 2013
b4f06ab
@maojrs modifications under updated version
maojrs Aug 6, 2013
c05f030
Repeat: @maojrs changes to updated version of JSAnimation
maojrs Aug 6, 2013
c5d7f95
Fiexed indentation
maojrs Aug 6, 2013
be3be0b
Fiexed indentation 2
maojrs Aug 6, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 39 additions & 19 deletions JSAnimation/html_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,17 @@ def _load_base64(self, filename):
return 'data:image/{0};base64,{1}'.format(self.extension,
data.encode('base64'))

PREV_INCLUDE = """
{add_html}
"""

JS_INCLUDE = """
<script language="javascript">
/* Define the Animation class */
function Animation(frames, img_id, slider_id, interval, loop_select_id){
function Animation(frames, img_id, slider_id, frame_id, interval, loop_select_id){
this.img_id = img_id;
this.slider_id = slider_id;
this.frame_id = frame_id;
this.loop_select_id = loop_select_id;
this.interval = interval;
this.current_frame = 0;
Expand Down Expand Up @@ -63,6 +67,7 @@ def _load_base64(self, filename):
this.current_frame = frame;
document.getElementById(this.img_id).src = this.frames[this.current_frame].src;
document.getElementById(this.slider_id).value = this.current_frame;
document.getElementById(this.frame_id).value = this.current_frame;
}

Animation.prototype.next_frame = function()
Expand Down Expand Up @@ -167,7 +172,7 @@ def _load_base64(self, filename):

DISPLAY_TEMPLATE = """
<div class="animation" align="center">
<img id="_anim_img{id}">
<img id="_anim_img{id}" style="width:{frame_width}px">
<br>
<input id="_anim_slider{id}" type="range" style="width:350px" name="points" min="0" max="1" step="1" value="0" onchange="anim{id}.set_frame(parseInt(this.value));"></input>
<br>
Expand All @@ -181,6 +186,7 @@ def _load_base64(self, filename):
<button onclick="anim{id}.last_frame()"><img class="anim_icon" src="{icons.last}"></button>
<button onclick="anim{id}.faster()">+</button>
<form action="#n" name="_anim_loop_select{id}" class="anim_control">
<input id="_frameno_id" type="textbox" size="1" onchange="anim{id}.set_frame(parseInt(this.value));" onpaste="this.onchange();" oninput="this.onchange();"></input>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of adding the frame counter here. Although there are places where this functionality would be helpful, it breaks some of the symmetry of the design.

<input type="radio" name="state" value="once" {once_checked}> Once </input>
<input type="radio" name="state" value="loop" {loop_checked}> Loop </input>
<input type="radio" name="state" value="reflect" {reflect_checked}> Reflect </input>
Expand All @@ -194,31 +200,30 @@ def _load_base64(self, filename):
(function() {{
var img_id = "_anim_img{id}";
var slider_id = "_anim_slider{id}";
var frame_id = "_frameno_id"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this string variable hard-coded?

var loop_select_id = "_anim_loop_select{id}";
var frames = new Array({Nframes});
{fill_frames}

/* set a timeout to make sure all the above elements are created before
the object is initialized. */
setTimeout(function() {{
anim{id} = new Animation(frames, img_id, slider_id, {interval}, loop_select_id);
anim{id} = new Animation(frames, img_id, slider_id, frame_id, {interval}, loop_select_id);
}}, 0);
}})()
</script>
"""

INCLUDED_FRAMES = """
for (var i=0; i<{Nframes}; i++){{
frames[i] = "{frame_dir}/frame" + ("0000000" + i).slice(-7) + ".{frame_format}";
}}
frames = {frame_list}
"""


def _included_frames(frame_list, frame_format):
def _included_frames(frame_fullnames):
"""frame_list should be a list of filenames"""
return INCLUDED_FRAMES.format(Nframes=len(frame_list),
frame_dir=os.path.dirname(frame_list[0]),
frame_format=frame_format)
return INCLUDED_FRAMES.format(Nframes=len(frame_fullnames),
frame_list=frame_fullnames)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're directly inserting a string representation of a python object into the javascript. This may work, but I would rather explicitly construct valid javascript code. For one, I can never remember whether this will use the __repr__ or the __str__ method, nor can I remember the differences between the two. Explicit is better than implicit.




def _embedded_frames(frame_list, frame_format):
Expand Down Expand Up @@ -246,8 +251,12 @@ def new_id(cls):
return '%16x' % cls.rng.getrandbits(64)

def __init__(self, fps=30, codec=None, bitrate=None, extra_args=None,
metadata=None, embed_frames=False, default_mode='loop'):
metadata=None, embed_frames=False, frame_dir=None, add_html='',
frame_width=650,default_mode='loop'):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to hard-code the frame width here. If we create a figure with a given size in matplotlib, this size should be reflected in the resulting animation.

self.embed_frames = embed_frames
self.frame_dir = frame_dir
self.add_html = add_html
self.frame_width = frame_width
self.default_mode = default_mode.lower()

if self.default_mode not in ['loop', 'once', 'reflect']:
Expand All @@ -258,21 +267,29 @@ def __init__(self, fps=30, codec=None, bitrate=None, extra_args=None,
super(HTMLWriter, self).__init__(fps, codec, bitrate,
extra_args, metadata)

def setup(self, fig, outfile, dpi, frame_dir=None):
def setup(self, fig, outfile, dpi):
if os.path.splitext(outfile)[-1] not in ['.html', '.htm']:
raise ValueError("outfile must be *.htm or *.html")

if not self.embed_frames:
if frame_dir is None:
frame_dir = outfile.rstrip('.html') + '_frames'
if not os.path.exists(frame_dir):
os.makedirs(frame_dir)
frame_prefix = os.path.join(frame_dir, 'frame')
if self.frame_dir is None:
self.frame_dir= outfile.rstrip('.html') + '_frames'
if not os.path.exists(self.frame_dir):
os.makedirs(self.frame_dir)
frame_prefix = os.path.join(self.frame_dir, 'frame')
else:
frame_prefix = None

super(HTMLWriter, self).setup(fig, outfile, dpi,
frame_prefix, clear_temp=False)

# Set frame fullname; it can be replaced in child class
def set_framename(self):
frame_fullname = self._temp_names
for i in range(len(self._temp_names)):
frame_name = 'frame' + str(i).zfill(4) + "." + self.frame_format
frame_fullname[i] = os.path.join(self.frame_dir, frame_name)
return frame_fullname
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, these indentations need to be the standard four spaces. Mixing indentation levels makes the code difficult to read, and can sometimes lead to unexpected results.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also set_framename is a pretty misleading function name. get_frame_tempfile_list or something along those lines would be better.


def grab_frame(self, **savefig_kwargs):
if self.embed_frames:
Expand Down Expand Up @@ -300,8 +317,8 @@ def communicate(self):
self.frame_format)
else:
# temp names is filled by FileMovieWriter
fill_frames = _included_frames(self._temp_names,
self.frame_format)
frame_fullname = self.set_framename()
fill_frames = _included_frames(frame_fullname)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable naming convention is very confusing. Here frame_fullname is actually a string-formatted list of all the frame locations, correct? A more descriptive variable name would be helpful.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now I see that it's just a python list, but I had to dig pretty deep to figure that out.


mode_dict = dict(once_checked='',
loop_checked='',
Expand All @@ -311,10 +328,13 @@ def communicate(self):
interval = int(1000. / self.fps)

with open(self.outfile, 'w') as of:
if (self.add_html != ''):
of.write(PREV_INCLUDE.format(add_html=self.add_html))
of.write(JS_INCLUDE)
of.write(DISPLAY_TEMPLATE.format(id=self.new_id(),
Nframes=len(self._temp_names),
fill_frames=fill_frames,
interval=interval,
icons=_Icons(),
frame_width=self.frame_width,
**mode_dict))