CVS edit link for omllobet

I wanted to be able to have a sort of paint application in my web and i found http://code.google.com/p/paintweb/ that doesn't require any plugin in the browser. So I developed a module to integrate it on my web.

The idea is to be able to draw. You can start from scratch or import an image from flickr. The optional flickr integration uses flickrapi module. The nodes are saved using imagefield module.

Basically I want kids to be able to use it, so it tries to be quite easy. I'm also planning on integrating onto drupal other html5 canvas apps (don't know what apps yet) but as I'm new to drupal and this is my first module, i'd like to have some feedback.

You can download the module at: http://dl.dropbox.com/u/1041436/paint.zip
Be aware that you'll also need to download paintweb from the http://code.google.com/p/paintweb/. So please read the README.txt file

Screenshots:
*Paintweb loaded: http://dl.dropbox.com/u/1041436/screenshot.png
*Import from flickr: flickrhttp://dl.dropbox.com/u/1041436/screenshot1.png

That's all, thanks for your time :)
Oscar

Comments

omllobet’s picture

Status:Postponed (maintainer needs more info)» Needs review
StatusFileSize
new15.66 KB
omllobet’s picture

StatusFileSize
new15.66 KB

New version with some minor modifications to make it works in sites with javascript performance option enabled and clean url.

I've removed paint from dropbox.

Thanks.

kiamlaluno’s picture

Status:Needs review» Needs work
Issue tags:+module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

omllobet’s picture

Status:Needs work» Needs review

Well, first of all it integrates paintweb: paintWeb is a Web application which allows users to draw inside the Web browser, making use of the new HTML 5 Canvas API. So you'll be able to draw and also to edit images in the browser. So this module comes with the editor and integrates the save button into drupal so when you hit the save button on paintweb a new image node is created/update if you have the rigth permissions (the image nodes uses imagefield module). Yo also have the ability to search and import images from flickr in a really easy way so you don't have to start drawing from scratch. It offers a edit button when you are on the edit node page. An you can also see an editing link when you're not on the teaser view of a node. And also the module has the ability to make a copy of a node if it has permissions.

Currently the modules for image edition that I found basically can crop or turn images. In http://drupal.org/node/769830 you can see some start code to integrate paintweb as a tinyMCE plugin, but it seems there was no more work on it and it was never released as a module.

I've set-up drupal in a free hosting here: http://osini.byethost31.com/drupal/

Thanks,

omllobet’s picture

StatusFileSize
new15.94 KB

Changed some permission names to fit in drupal style permissions. Also, made the README more clear.

Also, you can find a demo of paintweb (not within drupal) at

http://www.robodesign.ro/paintweb/trunk/demos/demo1.html

Thanks

omllobet’s picture

StatusFileSize
new16.14 KB

new version: use menu_get_object instead of arg

kiamlaluno’s picture

Status:Needs review» Needs work
  • This review is only partial.
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. Hook implementation comments should be like the following one:
    <?php
    /**
     * Implements hook_menu().
     */
    ?>
  2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how Drupal variables, global variables, constants, and functions defined from the module should be named.
  3. The module doesn't implement hook_uninstall().
  4. <?php
     
    if (strcmp($fieldname, check_plain($fieldname)) != 0) {
       
    form_set_error('paint_imagefieldname', t('The name of the field is not safe to save.'));
      }
    ?>

    I am not sure I understand what the purpose of strcmp($fieldname, check_plain($fieldname)) is.

  5. <?php
      $form
    ['serverid'] = array(
       
    '#type' => 'hidden',
       
    '#required' => TRUE,
        );
    ?>

    #required is not used for hidden form fields.

  6. <?php
    function paint_flickr_download_form() {
     
    $form['fid'] = array(
       
    '#type' => 'hidden',
       
    '#required' => TRUE,
        );
     
    $form['serverid'] = array(
       
    '#type' => 'hidden',
       
    '#required' => TRUE,
        );
     
    $form['id'] = array(
       
    '#type' => 'hidden',
       
    '#required' => TRUE,
        );
     
    $form['secret'] = array(
       
    '#type' => 'hidden',
       
    '#required' => TRUE,
        );
     
    $form['title'] = array(
       
    '#type' => 'hidden',
       
    '#required' => FALSE,
        );
     
    $form['#submit'] = array(
       
    '#type' => 'submit'
     
    );
      return
    $form;
    }
    ?>

    What is the purpose of a form that has only hidden fields?

  7. <?php
     
    if (!user_is_logged_in()) {
          return
    drupal_access_denied();
      }
    ?>

    That code should be part of the access callback.

  8. <?php
     
    if (!module_exists('flickrapi')) {
        return
    t('Flickr integration needs Flickr API module installed and enabled.');
      }
    ?>

    Why isn't the code calling drupal_set_message()?

  9. <?php
       
    if (fwrite($handle, $result->data) === FALSE) {
         
    drupal_set_message(t("We couldn't save the image."));
          exit();
        }
    ?>
    <?php
     
    if (!$comma) {
        exit(
    t('Error: data incorrectly formatted'));
      }
    ?>
    <?php
       
    else {
         
    drupal_set_message(t("You don't have permissions for creating an image"), 'error');
          exit(
    'Error: incorrect permissions');
        }
      }
      exit(
    'OK');
    ?>

    The code should not call exit() in such cases.

  10. <?php
     
    if (!empty($nid)) {
       
    $node = node_load($nid);
        if (
    $node != FALSE) { // update node
          // if it is the admin allow edit if the node is not anonymous, if its not, create a new node this way
         
    if ((user_access('edit own ' . $node_type . ' content') && $uid == $node->uid ) || user_access('edit any ' . $node_type . ' content'))  {
           
    $update = TRUE;
           
    $file = _register_file($filename, $imginfo[0], $imgdest, $uid);
           
    $node->title = check_plain($title);
           
    $node->field_image = _fill_field_image($file, $title);
           
    node_submit($node);
           
    node_save($node);
          }
          else {
           
    drupal_set_message(t("You don't have permissions for editing an image"), 'error');
            exit(
    'Error: incorrect permissions');
          }
        }
      }
    ?>

    The code should also check the node is published.

  11. <?php
          $text
    = t('Import an image from flickr...');
         
    $ret = '<a href=' . url('paint/flickr') . '>' . $text . '</a><br />';
    ?>

    l() is the right function to call in this case.

omllobet’s picture

Status:Needs work» Needs review
StatusFileSize
new16.3 KB

1.- ok, done.
2.- ok, done.
3.- ok, done.
4.- That's to inform the user not to put some html or strange chars in the name of the field and the other settings
5.- ok, done
6.- That form is used to pass some params to download the flickr image. So:
1.-The user selects the import flickr link.
2.-Searches a text. Submits paint_flickr_form()
3.-Then 12 or less thumbnails appear on the web. see paint_get_flickr_photos
It clicks one, and then the module fills with javascript this form and submit it (see flickr.js).
So now, the module can download the real image, see paint_download_image and this image is loaded into paintweb.
Is it ok? or do you prefer another solution like to put in every image a link like http://{domain_name}/?q=paint/flickr/download&farmid=2&id=18212&secret=999182
7.- ok, done
8.- ok, done
9.- ok, done
10.- ok, done
11.- ok, done

omllobet’s picture

StatusFileSize
new16.38 KB

use file_save_data
added some little javascript

mlncn’s picture

Status:Needs review» Reviewed & tested by the community

Hello omllobet,

Huge apologies that your module and CVS application has remained un-reviewed for so long. I can see you have made changes or addressed everything in kiamlaluno's very thorough partial review.

Another note:

  1. Code comments should end in a period, including the first line of documentation blocks that introduce a function. Inline comments also should be Sentence case and end with a period.

Everything else looks good and i recommend your application for approval, you can correct comments and anything else that comes up in code committed to drupal.org proper!

Thanks for this module!

kiamlaluno’s picture

Status:Reviewed & tested by the community» Needs review
omllobet’s picture

StatusFileSize
new16.34 KB

Comments corrected

mlncn’s picture

Status:Needs review» Reviewed & tested by the community
zzolo’s picture

Status:Reviewed & tested by the community» Needs work

@Daniele, thanks for the application and patience. There are still a couple thing I would like to clear up before approval. I am sorry for the delay but I feel they are pretty important.

The following points are just a start and do not necessarily encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

Neccessary for approval:

  1. What is the deal with the loading image? Is this your own image? If not, is it available under GPL? Why not use the one that is core?
  2. <?php
    function paint_uninstall() {
     
    variable_del('paint_typename');
     
    variable_del('paint_imagefield_name');
     
    variable_del('paint_paintweb_config');
    }
    ?>

    This function needs to be in a .install file.

  3. <?php
    function paint_admin__form_validate($form, &$form_state) {
    ?>

    This function is not named correctly and is probably not fired. (I have not actually tried to run the module so do not know). Should not have the double underscore.

  4. <?php
    function paint_form_alter(&$form, $form_state, $form_id) {
      if (
    $form_id == 'paint_flickr_download_form') {
       
    $form['#action'] = url('paint/flickr/download');
      }
    ?>

    Why are you form altering your own form? This seems unnecessary.

  5. <?php
      $ret
    .= drupal_get_form('paint_main_form', $title) . '<span class="paint-status">' . t('Loading...') . l(t('Refresh'), 'paint/' . check_plain($node_id)) . '</span><img id="PaintEditableImage" src="'. check_url($img_src) .'" alt="test"><div id="PaintWebTarget"></div>';
    ?>

    HTML output should be in a theme function. You could also just consider putting these parts into the form definition itself with the appropriate attributes and form types.

Suggested for a better module (IMO), but not necessary for application approval:

  1. Allow the path to the Google Paint library to be configurable. Consider utilizing the Libraries API module: http://drupal.org/project/libraries
  2. Javascript has Drupal Coding Standards as well, please follow these.
  3. <?php
    /**
    * Implements hook_uninstall().
    */
    ?>

    Should be the following (note the spaces):

    <?php
    /**
     * Implements hook_uninstall().
     */
    ?>
  4. I am not sure why you have such complicated permissions going on. Both the node module and CCK deal a lot with permissions and access to this sort of thing. Consider making this simpler and using the existing systems.
  5. Consider utilizing CCK and filefield better to where this would just be a widget for this field. This would mean that someone could simply just add a PaintWeb field to a node type.
  6. <?php
         
    '#attributes' => array('onclick' =>
           
    'if (confirm(Drupal.t("Do you want to leave and edit the image?"))) {
                window.location.replace("'
    . url('paint/', $options) . arg(1) . '");
                return false;
            }'
    ?>

    Inline JS is not cool for many reasons. Please consider moving this to your JS file and utilizing jQuery.

  7. <?php
       
    return t("We couldn't found any photos that match your criteria");
    ?>

    Not proper American English.

  8. Consider using the "file" parameter in hook_menu() to keep the .module file size down since it is included on every Drupal page load.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.

zzolo’s picture

Hi @omllobet, sorry for the name mix up above, the message was indeed intended for you.

Also, just wanted to say that you are very close and doing a great job!

omllobet’s picture

Status:Needs work» Needs review

Necessary
1.- ok, changed.
2.- ok, done.
3.- totally true, it seems I messed up in a revision.
4.- yeah, fixed.
5.- ok, done.

Suggested
1.- I'll take a look :).
2.- changed some function names and variable names to lowerCamelCase, some spaces corrected and changed the name of some vars to something more meaningful.
3.- ok, corrected.
4.- ok, I'll try to simplify it when it gets approved..
5.- Could you explain a little bit more how'll that work. You mean have like a imagefield field in a node but a paintweb field so people will see the paintweb app algon with other nodes and be able to create an image on the node creation right?
As a side note I'm planning to integrate paintweb into Wysiwyg so this work http://drupal.org/node/769830 doen't get lost. (already contacted with the code author).
6.- ok, now in my todo list.
7.- thanks, fixed.
8.- mmm I see, I'll split the optional flickr part into another file. Now in my todo list before a release is done.

I'd also like to thank the reviewers for your time and help.

Oscar

omllobet’s picture

StatusFileSize
new14.38 KB

ups, I forgot to attach the file.

zzolo’s picture

Status:Needs review» Fixed

Hi @omllobet, thanks for the application and patience. Congratulations, you have been approved. The following points are more recommendations for when you do put your code on drupal.org and develop it further; these are not requirements, simply suggested improvements based on my experience and knowledge of the existing Drupal community. Also, please read further about CVS access and resources.

  1. .loading {
      background: url('/misc/progress.gif') no-repeat center center;
    }

    This CSS assumes that Drupal is in the webroot which is not a good assumption. Unfortunately there is not a really good way to do this considering how one can't use variables in CSS. I would simply copy that image to your module, as we know its GPL.

  2. Concerning suggestion 5 above. Look into how to implement a CCK field. Basically a CCK field is two parts: 1) the Field which defines how its stored, such as a file in filefield. And 2) the Widget defines the interface for input (this is what imagefield is). So, you're module should simply be a widget for filefield. This would make your module a lot cleaner and leverage a lot more existing CCK infrastructure. This a good, albeit a little old, post on how to create a CCK field: http://www.lullabot.com/articles/creating-custom-cck-fields Also, looking at the imagefield module would be helpful, and there is a lot of documentation for filefield here: http://drupal.org/handbook/modules/filefield
  3. On the WYSIWYG integration, that should be fine and should not conflict with the point above. A more sustainable and robust way to build this project would to abstract out any wrappers you create around the PaintWeb API into a core paint.module, then have supplemental module for paint_cck.module and paint_wysiwyg.module (maybe even paint_flikr.module. This create a lot more file, but allows people to only use the functionality they need, reducing their code base and allowing Drupal to run more efficiently.

Please read the following resources to make sure you know how to use CVS and the specifics to the Drupal CVS infrastructure, as well as how to be a good module maintainer on Drupal.org. The Drupal community is very large and dynamic; we welcome you as a module maintainer and hope that you embrace and challenge the Drupal community and continue to contribute.

Thanks to the following people who helped review this application, it is very appreciated:

  • @kiamlaluno
  • @Benjamin Melançon

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.

Status:Fixed» Closed (fixed)
Issue tags:-module review

Automatically closed -- issue fixed for 2 weeks with no activity.