This project issue has been closed - I don't have the time to pursue it at this moment. I've realized I should have started with a much smaller module for my first Drupal Project.

Sandbox page: https://www.drupal.org/sandbox/mtnguru/2272169

PAreview: http://pareview.sh/pareview/httpgitdrupalorgsandboxmtnguru2272169git

Git clone command:

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/mtnguru/2272169.git imager
cd imager

This module is for Drupal 7. Once I get through the Project Application process I'll start looking into porting this to Drupal 8.

Overview

The Imager module provides four major capabilities for images:

  • View Images: User clicks on a thumbnail or image on any Drupal page, Imager pops up an image viewer dialog - user can pan, zoom, enter full screen mode, select previous/next image.
  • Edit Images: While in the viewer the user can edit the image - rotate, crop, brightness/contrast, hue/saturation/lightness
  • Save, Download, email, print image: Image can be saved back to Drupal, downloaded to local computer, attached to an email, or printed.
  • View/Edit File Entity fields: While viewing an image the user can popup a rendered view of the file entity. From that popup the user can select a field to edit. The form widget for that field is popped up, the user edits the value, and upon closing the popup the value is saved using AJAX.

Demonstration Site

Click here to see a demo. Left click on a thumbnail, the Imager Viewer pops up displaying the full size image. Double click on that image to close the viewer. Use the thumbwheel to zoom, the zoom is centered on the cursor location. Click and hold the left mouse button to drag or pan the image. Click the small 'i' icon under the View Buttons to see the rendered file_entity.

I like the first picture, a composite of many nebulae. It shows off the panning and zooming capability. It's also interesting to play with contrast, brightness, hue and color saturation - this highlights the filamentation in the nebulae. This is a very large image - over 30MB - be patient when it loads.

Comments

mtnguru’s picture

mtnguru commented
Title: Imager - Image viewing and editing module » Imager - Image viewing and editing module - Drupal 7
Issue summary: View changes
PA robot’s picture

PA robot commented

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

mtnguru’s picture

mtnguru commented
Status: Active » Needs review

I think I'm ready, here goes ... Changing status to "Needs Review"

Today I'm working on documenting functions. Don't spend too much time on documentation. I know getting this module approved will take several iterations. I'm working on it. However any comments about format are appreciated.

The last couple days I have:

  • Checked code with pareview and coder - everything looks clean there.
  • Added the t() function to string literals.
  • Added check_plain where imager.ajax.inc uses $_POST.
  • Put in lots of DocBlocks - still working on content.
  • Stopped using drupal_load_js and drupal_load_css. Now using #attached.
mtnguru’s picture

mtnguru commented
Issue summary: View changes
sajiniantony’s picture

sajiniantony commented
Status: Needs review » Needs work

Hello,
I installed the module and tried creating a content with image field.Please find the notice fired.
Undefined index: css_class in imager_views_pre_view(). (line 117 of imager.module).
Please check the function imager_views_pre_view() of imager.module file.
In the hook_init() funtion in imager.module file a return statement is shown. The init function is called in all pages. If not required remove the same.

mtnguru’s picture

mtnguru commented
Issue summary: View changes

Added info about Distribution

mtnguru’s picture

mtnguru commented
Issue summary: View changes
mtnguru’s picture

mtnguru commented
Status: Needs work » Needs review

I'm putting the status back to Needs Review. The loading problems have been fixed.

sajiniantony - Thanks for looking at my module. The module wasn't designed to work with images attached to a content type. This shouldn't be difficult but the code isn't written. It was designed to work with File Entities that are output in a View.

To make installation easier I've created a very basic distribution. Instructions for installing the distribution are on the project sandbox page.

I know this module will take several tries to get through the Project Application Process. It's not a simple module.

For now please look at the coding, format, structure, security, etc. I've made a lot of progress on documentation but still have work to do. I just read the Drupal JavaScript documentation guidelines - yikes!

mtnguru’s picture

mtnguru commented

I've been doing a sprint on the code all weekend and just committed significant changes:

  • Improved in code documentation - needs a lot of work still, especially JavaScript.
  • Created configuration page admin/config/media/imager - this eliminated all? hardcoded paths and strings.
  • Fixed download, email, view options. Most issues were related to hardcoded paths that are now configurable.
  • hook_imager_modify() - Created a hook so developers using Imager can update changes to the thumbnail page when images or file_entity fields are edited.
  • JavaScript - broke init and attach functions apart. Init is done once, attach is done as needed with Drupal.behaviors.
  • Added an imager_example module - this was need to test hook_imager_modify.
  • Now using rendered file_entities in email text, and when bringing image up alone in simple html page.
  • Plus many minor fixes
PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmtnguru2272169git

I'm a robot and this is an automated message from Project Applications Scraper.

mtnguru’s picture

mtnguru commented
Status: Needs work » Closed (won't fix)

I'm withdrawing this project from the Project Application process. At a recent DBUG meeting they discussed upcoming changes to the Project Application process and proceeding on with this will take a lot of time. It will take me at least 8 months to get through the process, whereas the expected changes will be in the next couple months. Once the project is findable, I suspect someone will come on board to help.

The proposed changes are to get rid of Sandboxes. Instead every project starts out as a development project such as 7.x-dev. These projects will be clearly marked as under development and not having been approved. To become a full release - 7.x-1.x - ALL projects must go through the Project Application process.

Update May 13 I must been having a bad day, I'm not going to change the above wording, but wouldn't write that today :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I'm sorry, but I think you misunderstood. Proposed changes: #2453587: [policy, no patch] Changes to the project application review process.

We keep sandboxes and we also keep the project application process. Users will be able to release 1 full module as unstable project, but they will still have to go through this process to publish more projects.

mtnguru’s picture

mtnguru commented
Status: Closed (won't fix) » Needs review

Discussions at DrupalCon have prompted me to put this back to "Needs Review". It's going to take months to implement changes to D.O. to implement the new Project Application process and I want to get this module approved long before then.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work

http://pareview.sh/pareview/httpgitdrupalorgsandboxmtnguru2272169git-7x-1x

Several thousand errors. Might be a good place to start.

mtnguru’s picture

mtnguru commented

heddn: I ran the tests a month ago and thought I had eliminated most errors. I must have run the tests wrong, You are right, there are 1000's. I switched this week from Netbeans to PhpStorm and it fixed a bunch of errors but I still have at least a 1000.

Lesson learned, if you plan to write a contrib module, learn the coding standards first.

mtnguru’s picture

mtnguru commented
Status: Needs work » Needs review

Yay! pareview passes - I'm putting the status back to "Needs Review". The only remaining errors are in JavaScript libraries I'm using - imgareaselect.js, fullscreen.js and hoverintent.js. Are these supposed to be placed into a library with drupal_add_library?

Review the 7.x0-dev-dialogs branch. Review the 7.x-1.x branch.

Do not attempt to install on your system, I need a couple days to ensure everything works correctly first.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: mtnguru » Unassigned
Status: Needs review » Needs work
PAReview: 3rd party code
hoverintent.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Please merge all changes back into the main 7.x-1.x branch so that there is no confusion what to review.

mtnguru’s picture

mtnguru commented

Ok, I thought phpcs was similar to pareview.sh. It isn't. The code fails the online pareview.

My previous message should have said "phpcs" passed.

Looks like klausi beat me to putting it back to "Needs Work".

mtnguru’s picture

mtnguru commented
Status: Needs work » Needs review

Moving back to "Needs Review".

Made primary branch 7.x-1.x as recommended by klausi.
Removed JavaScript vendor files as recommended by klausi.
Removed icon images as recommended by kreynen.

Note: The above changes made the installation fail. Please contact me if you intend to install this code.

Ran phpcs, phpcbf and pareview a billion times as recommended by heddn.

Remaining errors are primarily commented out code that doesn't meet commenting requirements.
I've deleted a lot of code like this in order to pass the review, I wish there were a better way. The remaining few lines I want to keep even though it causes pareview errors.

Is there a way to leave development code commented out without throwing errors with pareview?

ajalan065’s picture

ajalan065 commented
Status: Needs review » Needs work

Hi mtnguru,
I have not gone through the functionality of your project, Just was having a look on it and here is what I found.

1. Please change your module name and keep it as the convention goes.
[D7] module_name

2. Please add the git link of your project in the project page. Reviewers face difficulty in going into your sandbox and searching for links.

3. In your .module file, hook_init() is empty. So remove it from the code if not required.

4. Dont keep the gif files in css folder itself. Make a separate folder for the images.

5. In your imager.popups.inc, you have multiple lines where you have included your js and css. Why dont you instead declare the path to js and css in a constance and use it in all places. It looks better.

6. You have commented out a large portion of your imager.popups.inc, 

  // 'viewer'     => _imager_build_viewer(),
  // 'brightness' => _imager_build_brightness(),
  // 'color'      => _imager_build_color(),
  // 'filesave'   => _imager_build_filesave(),
  // 'status'     => _imager_build_status(),
  // 'info'       => _imager_build_info(),
  // 'edit'       => _imager_build_edit(),
  // 'messages'   => _imager_build_messages(),
  // 'canvasorg'  => _imager_build_canvas(),
    'busy' => _imager_build_busy(),
  // 'map'        => _imager_build_map(),
  // 'form'       => _imager_build_form(),
  // 'confirm'    => _imager_build_confirm(),
  // 'config'     => _imager_build_configuration()

.
Remove this peace when its not required.

7. You have already declared the variable $_imager_enable as global in imager_views_pre_view(). So no need of declaring it again imager_page_alter() as global in your .module file.

8. Keep it in a variable drupal_get_path('module', 'file_entity') . '/file_entity.js', and use it wherver required.

These were a brief review from my side.
Correct them and commit again. Would be happy to review your project.
Comment me if I am wrong.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review

re #20: these are all fine feedback. However, none of this is a release blocker. Moving back to needs review.

babusaheb.vikas’s picture

babusaheb.vikas commented

1) No need to use quotes in *.info file
name and package should be
name = Imager
package = Media

2) I have found below error when going to install your module.
Parse error: syntax error, unexpected ',' in D:\xampp\htdocs\responsivewebsolutions\sites\all\modules\imager\imager.install on line 52

3) Several errors on Pareview. Please review and fix it.
http://pareview.sh/pareview/httpgitdrupalorgsandboxmtnguru2272169git

smakisog’s picture

smakisog commented

@mtnguru: Hello, you wrote

What I'm looking for now
This is a fully functioning module, it is in use by my customer.

At first glance, this module a rather crude

Even on first part - installation there are imager.install on line 52 error

$requirements[$key]['description'] = 'The %name dude plugin is installed at %path', array(
            '%name' => $requirements[$key]['title'],
            '%path' => libraries_get_path($name),
          );

i think should be

$requirements[$key]['description'] = t('The %name dude plugin is installed at %path',
  array(
    '%name' => $requirements[$key]['title'],
    '%path' => libraries_get_path($name),
  ),
);

Then you use libraries hooks, so plz add dependencies to *.info file

Also, i've installed jquery.imgareaselect library, setup imager settings but nothing happens. Please provide more information on how to init and setup your module.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs review » Needs work

The demo site no longer works.

Plenty of errors from http://pareview.sh/pareview/httpgitdrupalorgsandboxmtnguru2272169git - some may be false positives and some just cosmetic, but bad coding styles make manual code review harder than it need to be.

I also think the major problems pointed out in #22 and #23 blocks promotion until fixed.

Changing status to "Needs work".

mtnguru’s picture

mtnguru commented
Issue summary: View changes
mtnguru’s picture

mtnguru commented
Status: Needs work » Needs review

Putting the status back to "Needs review".

Thanks to those who have looked at the module. I apologize for the issues. I haven't had a lot of time to look at it. But that's changing. I want to port it to Drupal 8 and the first step is to get it up as a contrib module on Drupal 7. I have some time for the next 3 weeks before I start a new job.

I fixed the demo site.

I have fixed all the pareview errors that it makes sense to fix. There are a few remaining errors but I see little value in fixing them. IMHO - pareview makes it very difficult to comment out code temporarily and it puts severe limitation on how comments are formatted. And what's up with limiting a line to 80 characters. I have a 3440x1440 display. This should be increased to at least 160 chars. In the end I ended up taking out hundreds of valuable comments just to get the pareview errors down to where they are now.

The module is ready for you to install. Today I did a clean install of Drupal 7, enabled the File Entity, Entity, Views and Imager module. I added some images (files) and created a View as specified on the Sandbox page. It all worked quite spendidly. So go ahead and install the module and give it a try.

If you know how to make this into an image formatter please contact me. I'd love to talk and get a sense of what it takes.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Issue summary: View changes

You've named the file where you define your API hook definition imager.api.inc. This is wrong and confuses PAreview. It must be named imager.api.php. See: https://www.drupal.org/coding-standards/docs#hooks

IMHO - pareview makes it very difficult to comment out code temporarily

There should be no commented out code in a project you share with others. If you want this for your own purposes, have a private development version that you work on outside git, and when you are ready to push to the shared repo, copy this to your local repo and remove all commented out code before pushing. Commented out code usually makes no sense to other people and just makes the review task more difficult.

And what's up with limiting a line to 80 characters. I have a 3440x1440 display.

And to review your code, everyone should get a 3440x1440 display? Please understand that when you submit your code here for review, it should be readable for everyone - not just you. Some reviewers may use a much smaller screen than your. Some may have a different workflow. For instance, when doing a full code review, I prefer to print the source code on paper in a fixed font that prints 80 characters across the sheet, in order to use yellow highlighter to mark the sections that I need to focus on. Lines exceeding 80 characters makes a mess of the printout.

Code style issues and file naming conventions are not blockers, so I'll leave this at "Needs review". However, by ignoring these things, you make your project look very unpolished. There are today around 180 applications on the waiting list. As a reviewer, I prefer to work on polished code rather than crude stuff, so I will probably not look at your code again. I'll leave it to someone who cares less about these things to do the full review.

mtnguru’s picture

mtnguru commented

gisle: Thanks for your comments, this is one of the more useful reviews I've had. I really wish I had developed a nice short little 500 line module as my first Drupal project. Imager is 2500 lines of PHP and 3600 lines of JavaScript utilizing many different Drupal concepts. It is not an easy project to review. I appreciate you taking your time.

Thanks for pointing out my incorrect name on the imager.api.php file. I've tried to figure that out several times.

I agree finished code shouldn't have a lot of commented out code. When I look at this module I really don't see that many lines that are commented out. However this code is still under development, it is still changing. I think commenting out code temporarily is a valid way to develop software.

I think if you look at the code you will see it is far from crude and I'm not simply ignoring the standards. I've always been very particular about how I format code and name things. I initially made the mistake of writing most of this module without first familiarizing myself with Drupal standards. That resulted in a lot of errors, I won't do that again.

One of my frustrations is that pareview is a moving target. I've fixed all problems several times and put it back to "Needs Review", then no one looks at it for a couple months and by the time they do pareview has changed and there are a bunch of new errors. Almost all of them cosmetic and related to comments. They don't effect how the code works, or how secure it is, or whether I was following Drupal concepts. I don't violate coding standards often but when I do it's with good considered reason. I think it is more important my code be understandable and easy to follow then that it be formatted exactly to a standard.

As an new and as yet unapproved contributor I'm being help to a much stricter standard than more experienced contributors are. If you run the media module through pareview you'll see thousands of errors that are much more egregious then what I've done. Media has over 1.5 million downloads. Yet I as a new contributor to Drupal must write better code then that in order to get any traction with it.

I really am at a quandary on how to get this module through the PA process. I've thought many times of withdrawing it and creating a simpler module just so I can get myself out of PA prison. Then once I get out of PA Prison I can post virtually any module with little oversight.

I just fixed all but 3 pareview errors. All 3 are OK and should not have been flagged by pareview. 2 are due to a bitwise shift operator that pareview does not like '<<'. The other error is that some JavaScript library is using the string EAHC and pareview thinks it is a misspelling.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Title: Imager - Image viewing and editing module - Drupal 7 » [D7] Imager - Image viewing and editing module
mtnguru’s picture

mtnguru commented
Status: Needs review » Closed (won't fix)

I decided to withdraw this from the project application process. I don't have the time to work on it and the process is very frustrating.

mtnguru’s picture

mtnguru commented
Issue summary: View changes