[D7] Media Pane

Code sniffer found a couple warnings but no real errors:

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
11 | WARNING | Line exceeds 80 characters; contains 140 characters
13 | WARNING | Line exceeds 80 characters; contains 211 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/media_pane.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
12 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
60 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
90 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
132 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
--------------------------------------------------------------------------------

I only took a quick look at it but I believe you're missing an uninstall hook and your readme.txt installation section is more advice than installation instruction.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Changed the git instructions to reflect new module branch

Hi Yanniboi!

Automatic Review

• PARaview threw up no errors so we're all good on that front.

Manual Review

• It think the Module page could be a little better. There are some grammar problems (comma's at the end of bullet points) but mainly I found it quite hard to know what the module was doing. A screenshot might be helpful?
• In .info I think you mean to depend on 'media' rather than 'mediafield'. IIRC mediafield has been deprecated for a while.
• In .install when creating the view modes field you use a function to generate a list of options. As these options seem to be dynamic (you want to add the view mode in whenever a view mode is added right?) I would recommend setting the 'allowed_values_function' setting.
• None of the field labels are passed through t() or st() (use $t = get_t() in the .install function to pick which one to use). Same goes for the descriptions.
• In hook_entity_info() you copy all of the view modes on the file entity onto the fieldable_panels_pane, from my understanding of the module this isn't what you want/necessary?
• An @return in the docblock for media_pane_get_view_mode_options would be helpful.
• I'm not sure the standard for docblocks on preprocess hooks. I would be inclined to use Implements hook_preprocess_file_entity() on line 60 and 90.

Other than that all looks good :)

manual review:

1. media_pane_theme(): I think your theme key should be prefixed with your module's name to avoid name collisions. I.e. fieldable_panels_pane should be media_pane_foobar or similar.

Otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to take a final look at this.

Given the code reviews above have passed, I'm now trialling as an end-user.

Coming in cold, I find the project page a little unclear as to what it sells and enables me to do. If we are all familiar with what fieldable_panels_panes are it may be a little more natural. A one-paragraph walkthrough on how to use it may help there. It's just a suggestion.

On the modules page (via the .info file) this should this be grouped with 'Media' or the 'Panels' section. Currently it's just in 'Other'.

On first install, (new clean site) I get death!

FieldException: Attempt to create a field of unknown type list_text. in field_create_field() (line 110 of /private/var/www/drupal7/modules/field/field.crud.inc).

This is probably because I chose to test on a 'Minimal' install and therefore didn't have some of your assumed core modules on. This is why I tested like this - to uncover undocumented dependencies. Though (maybe) rare, this is because you are invoking the field 'list_text' in 'media_pane_install' without confirming or requiring that the core list.module is available.
Quick fix is to add that to your requirements[]

After disabling and uninstalling this module, enabling 'list' and re-enabling, I seem to be up again.
Without a HOWTO on what happens next, I check the README. Still not immediately sure how to proceed as a first-timer. Probably less of an issue if you are already using panels and want to add this widget.

I see things coming up on the Panels admin, so something is working. Hm,

I'll enable Panels_node and see.

* Create a panles node and add content. I see 'media pane' as a thing I can add.
* It looks like it's available as a top-level or uncategorixed content element (not in a vertical tabs group). Thats ... probably OK for this one, though most pane providers do classify themselves as a 'Widget' or something.
* Using it triggers the 'Media' selector correctly, even though I haven't configured anything else. so that's as expected.
* I see the view mode coming up with the image styles, so now I'm understanding how this widget is handy.
But, um. hm, we have two very similar "View mode" selectors here.
- View Modes : "Select which view mode should be used to display the media"
- View Mode : "Select a view mode for this pane."
A little unclear here. may need better ordering or grouping. What is the second one doing in this context, is is just overlap with panels core options?

It looks like half the options are dependent on others. Though just a nice-to-have suggestion, I'd point you to the #states utility in Form API that would enhance that. Just FYI.

*And preview:
Hm. I was right about the UI being confusing. I thought by selecting the "view mode" image style I would be seeing the picture on the panel when it displays. Instead I'm getting the generic 'file attachment' icon.
Is this broken?

I'll stop here for now.
In general I now understand the idea of what it wants to do, and it's sure useful for that. +1 worthy there!

* must list the required module to avoid death. REQUIRED
* could be clearer on the project page, but you can do that in your own time. SUGGESTED
* should be clearer on the form UI - sort out the weighting, maybe group a little or use dependent visibility. RECOMMENDED
* does not actually work for me! REQUIRED. If I'm using it wrong (which is likely), then it's documentation:required.

Yeah, we all get a little to close to making things *work* to be able to communicate what the original intent was sometimes.

That sounds very cool.
I've come in deliberately ignorant of the intent, so used my fresh eyes for a "smoke test".

The "view modes" confusion makes some sense when explained, though I would not like to explain that to an end user. Improved UI labels will certainly solve most of that.

I took the direct route (this time) to installing just this module and following the prompts (if any) it gave me to get a result. I did suspect I'd missed a step to get the display actually working. Not having used the current version of Media a lot yet, I didn't have the view displays pre-configured in any way.
Maybe it's an oddity in Media itself, but I did recognize the image derivatives offered as being displays that the system should already know how to render, so was surprised when the using the options had no effect. Seeing your screencast, I appreciate that you were letting Media module do what it does, and so avoiding duplication.
Boosting the project page and README with explanations of this is a win for everyone.

I really enjoyed the screencast, and I think you should be proud of the front-end UI you've integrated there. I tried it 'minimal' so didn't get to take advantage of the overlay and in-place editing that really enhances how useful this module can be for this use-case.

So, without needing to re-test, as my issues were largely about the UX, I'm totally happy to +1 and promote this one today.

yanniboi is now a "vetted" user, Thanks for your contribution, yanniboi!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

You should find yourself with the ability to "promote the (sandbox) to a full project. " - it's a tab on your project edit page.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.