xmacinfo [xmacinfo]

@xmacinfo: No, you didn't. I simply added the review tags; that is why my comment appears empty.

Have you filed a patch for emfield? out of the box it already has support for a ton of providers, it would seem this functionality would work best as a patch to that module.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.

OP means original poster, which is you.

I forgot to set the status of this report as per my previous comment.

The CVS application was closed because it didn't get any feedback.
Considering that the maintainer of Embedded Media Field is creating some projects for the integration between Embedded Media Field and media providers, I think it doesn't create any problem to have also this project hosted on Drupal.org.

Yikes!

Sorry to take so long to reply here; ticket ended up dropping below my radar.

The Embedded Media Module is now in the process of dropping all its providers, in favor of providers being supported external to the module. The Media module has already done this (which will be the successor to emfield). That's because it's a real pain to hold up releases because some rinky-dink provider changes their API, and to encourage more developers to help maintain the sucker.

I haven't reviewed the code yet, but I plan to today.

Thanks,
Aaron

Function names, constants, and Drupal variable names should be prefixed by the module name; the module name is media_tagtele.module, then the function names need to start with media_tagtele_, the constant names must start with MEDIA_TAGTELE_, and Drupal variable names need to start with media_tagtele_ (when the variable is the one defined from the module, not when the variable is defined from another module, and used from the proposed module).

The license file must be removed, as it is not possible to commit such files.

The module works as advertised, and the code is clean. A few misc comments (not blockers, but things that should probably be addressed before a release).

* The README needs to wrap at 80 characters.

* Not sure about the // TagTélé feature has been created by xmacinfo(http://drupal.org/user/12131) comment in the module; at the least it should be in the /** section, and more likely in the README.

* The provider does not support thumbnails, and considering from my experience, that's the number one issue with various providers in the queues, you should at the very least mention that in the $features of emvideo_tagtele_info().

I looked a bit into the thumbnail issue, and found that it *appears* there may be a standard format for image urls: for instance, the thumbnail for http://www.tagtele.com/videos/voir/52415 currently resides at http://s1.ttimg.com/img/videos/thumbs192x108/52415_default.jpg?201002251.... I'm not certain if the thumbnail server farm changes periodically (it often does with other providers), but if with testing this turns out not to be the case, you could implement something like:

function emvideo_tagtele_thumbnail($field, $item, $formatter, $node, $width, $height, $options = array()) {
  return "http://s1.ttimg.com/img/videos/thumbs192x108/{$item['value']}_default.jpg";
}

Alas, it probably won't be so simple. I hunted around for an API, but couldn't find one easily. Might be worth contacting them; I've found in the past that most providers (other than YouTube) have been responsive to requests.

In any case, these issues are minor and easily resolved, so I'll mark this RTBC.

Thanks,
Aaron

Sorry kiamlaluno, didn't mean to cross-post.

In the .inc file, unfortunately, the current API for emfield breaks the rules (due to legacy) for function names. Thus, we have things like hook emvideo_PROVIDER_info, where the emvideo_ trumps the usual rule. Sorry about that; it's addressed in the Media module.

Perhaps you'd like to wait for a d7 version to review? xmacinfo indicated an upcoming version for that?

ah yes, the LICENSE.txt file (as has been pointed out) needs to be dropped, as do the following in the .info file, which will be added by the packager:

version = "6.x-1.0"
project = media_tagtele

It would also be enough that the module file is renamed to emvideo_tagtele.module. The file for new module would have then a different name.

There have not been replies in the last week. I am marking this application as won't fix.

This looks great to me. The issues w/ emvideo naming will be addressed in v2 of emfield, but for now, see #18 and #22 for the reasons for the function names; that's a mistake of the legacy emfield api, not of xmacinfo.