[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [GMG-Devel] [GSoC 2016] Tasks remaining
From: |
Christopher Allan Webber |
Subject: |
Re: [GMG-Devel] [GSoC 2016] Tasks remaining |
Date: |
Fri, 15 Jul 2016 10:13:40 -0500 |
User-agent: |
mu4e 0.9.16; emacs 24.5.1 |
Okay so... this is a bit long. Here's my code review.
The second bullet point could use some feedback from others, especially
breton/Boris.
- Some commits as "root" and "Ubuntu"... please rebase the branch and
fix the author field of these commits
- Right now there's kind of an intermediate state between being a
plugin and being "core" code. First of all, it does have its own
custom_subtitles plugin path, but:
#+BEGIN_SRC diff
--- /dev/null
+++
b/mediagoblin/db/migrations/versions/afd3d1da5e29_subtitle_plugin_initial_migration.py
@@ -0,0 +1,36 @@
+"""Subtitle plugin initial migration
+
+Revision ID: afd3d1da5e29
+Revises: 228916769bd2
+Create Date: 2016-06-03 11:48:03.369079
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'afd3d1da5e29'
+down_revision = '228916769bd2'
+branch_labels = ('subtitles_plugin',)
+depends_on = None
+
#+END_SRC
This has its own unique branch label. But if we look at the
models:
#+BEGIN_SRC diff
--- a/mediagoblin/db/models.py
+++ b/mediagoblin/db/models.py
@@ -573,6 +573,15 @@ class MediaEntry(Base, MediaEntryMixin, CommentingMixin):
name=v["name"], filepath=v["filepath"])
)
+ subtitle_files_helper = relationship("MediaSubtitleFile",
+ cascade="all, delete-orphan",
+ order_by="MediaSubtitleFile.created"
+ )
+ subtitle_files = association_proxy("subtitle_files_helper", "dict_view",
+ creator=lambda v: MediaSubtitleFile(
+ name=v["name"], filepath=v["filepath"])
+ )
+
#+END_SRC
This is attached to the core. The models should also be in
mediagoblin/plugins/custom_subtitles/models.py if this is
to be its own plugin.
This applies to video only. If anything, it should be attached to
the video plugin, but not the main media entry.
I'm actually all for the subtitle plugin becoming a "core" part of
the video plugin, if others are as well. I think in general it's a
nice feature. Having a boolean (as currently sort of exists,
though it should be moved to a different section)
So a decision needs to be made about whether or not the subtitle
stuff gets added to the video code proper or is its own plugin,
with a dependency on the video plugin. It shouldn't be attached
to core, at any case.
I'd also be interested in breton weighing in on this one.
- path_subtitle is duplicated twice.
- path_subtitle is also only used once... is it worth the decorator?
- '/c_s/<string:path>/edit/' for custom_subtitles is kind of a weird url
- so edit_subtitles could maybe reduce a level of indentation by doing this:
#+BEGIN_SRC python
def edit_subtitles(request, media):
if not mg_globals.app_config['allow_subtitles']:
raise Forbidden("Subtitles are disabled")
#+END_SRC
- it also looks like the "if 'subtitle_file' in request.files" is really more
about splitting things up between when there's a GET or a POST?
Also, is there any error handling that should be done?
Consider looking at how other views utilize wtforms to both handle
errors on forms.
- It does look like you're using the public store for subtitle files.
Good!
- subtitle, not subttitle, in the message flashed from edit_subtitles
- Instead of doing the try: finally: on the subtitle_public_file, you
can use a "with" statement, which will automatically close it once it
leaves the with's scope.
- Not critical, feel free to ask on irc for clarity: It might be
possible to write the file in chunks without having to pass the
.read() into the store file's .write()... I haven't tried this
myself with this particular file, but the write method on our file
interface will try to do a chunked write if it can if one file
itself is passed to its write method.
- It's a good idea to do a redirect after handling a POST. This means
that if a user does something, if they press the refresh button, it
won't simply resubmit the same data.
- If you add new files, please add the appropriate copyright headers!
- I notice that custom_subtitles uses open_subtitle. However, consider
that your subtitle has, at this point, been stored in the public_store.
You're constructing things from an absolute path,
- As for the css, I'm worried that it adds something about the "body"
element. Why is this?
- Also css wise, probably more unique names should be chosen than
"box" (almost everything in css is "boxy") and "overlay". Maybe
prefixing with "lightbox-" is good enough?
- In custom_subtitles, split this line up:
#+BEGIN_SRC html
<form action="{{
request.urlgen('mediagoblin.edit.custom_subtitles',path=path) }}" method="POST"
enctype="multipart/form-data">
#+END_SRC
Like so:
#+BEGIN_SRC html
<form action="{{
request.urlgen('mediagoblin.edit.custom_subtitles',path=path) }}"
method="POST"
enctype="multipart/form-data">
#+END_SRC
- This line should be indented one space for consistency:
#+BEGIN_SRC html
{{ csrf_token }}
#+END_SRC
- Same in edit_subtitles.
- The various .html files should go in the plugin's templates
directory... be that the video media type's templates or the
subtitle plugin's directory.
- Similarly, the assets should be in the plugin's own directory.
Look up how the "assetlink" system works in the mediagoblin docs,
and look at how other plugins provide their own assets.
- If subtitles are their own plugin and not a video media type,
then they should use a hook to insert into video.html,
rather than the addition to the template as done now.
- I'm not a big fan of inline javascript as in custom_subtitles.html...
for one thing, it's very risky. Consider the part that has this:
#+BEGIN_SRC html
<script>
var xmlhttp, text;
xmlhttp = new XMLHttpRequest();
xmlhttp.open('GET', '{{ path }}', false);
xmlhttp.send();
text = xmlhttp.responseText;
document.getElementById('test').value = text;
</script>
#+END_SRC
Consider: a malicious actor could be very clever and use something to
do code injection through the {{ path }} element (look up xkcd comic
of "little bobby tables" to see what I mean...!)
Instead, separate the javascript out into its own file. To provide
the "path" variable to the script, provide an html element like this:
<input type="hidden" id="subtitles-get-path"
value="{{ path }}" />
Then in your javascript, use the DOM to extract this element's value.
- *Don't* edit media.html. Use the media_sideinfo hook!
- Instead of adding code to the delete_media_files function,
we should provide a hook in this function that you should make use of.
Probably a "hook_runall" call.
- tools/subtitles.py should be actually be a "tools.py" in the subtitles
plugin's directory.
- The method you're using to open_subtitle() and save_subtitle() here
are the reasons your code isn't working with the cloud storage code.
Instead, you should use the storage api's own methods for opening
and saving files. Don't access their file paths on disk directly!
If you do this, your code will be generic enough to work with *all*
public store systems we provide.
- Likewise, rather than putting the routes in user_pages/routing.py,
you should instead handle them in your plugin directly. Take a
look at how mediagoblin/plugins/basic_auth/__init__.py does this,
in the setup_plugin section.
I know that's a lot! But I also know that a lot of these things are
hard to pick up without someone walking you through them.
Feel free to ping me or reply to here if you have any questions!
- Chris