Rename "Picture" module to "Responsive Image" module

I actually think this is at least major. Image vs. Picture is terribly confusing. Especially if <picture> doesn't end up being the spec HTML5 goes with. Much better to abstract this out into the concept.

Btw, I think "Responsive Images" is the right name. We don't call Image module "Image uploads and image styles" because it's the concept that's important, not the implementation.

This'd obviously require some under-the-hood changes, and I'm fine signing off on those.

If I'm not mistaken this could be tackled by a new contributor!

Here is the patch which simply renames module.
Could you please explain which API changes it requires? Should this module be merged into "image" module? Should it be renamed then?

The patch did apply but it is actually called
drupal-picture-rename-2124377-4_0.patch

I have added screenshots of what changed: In the Extend Overview (admin/modules) the name of this Core module is now "Responsive Images" .
If this is what needed to be done: it's done.

The directory the module is in remains core\modules\picture
Not clear if that needs to change too.

Next thing: After you have enabled the "Responsive Images" module the word 'Picture' pops up everywhere:

Hover over configure next to the module: 'manage picture mappings'
Permissions page in admin/people/permissions#module-picture: 'Administer pictures'
Config page in admin/config/media/picturemapping: 'Picture mappings' 4 times on the page.

Should this be new related issues? Or is this part of this (2124377) issue...?

first attempt, not done yet at all, using this issue right now for Core Mentoring at Global Sprint Weekend Switzerland.

#Update:
Don't need this issue anymore for Mentoring.

Next thing: After you have enabled the "Responsive Images" module the word 'Picture' pops up everywhere:

Good find, I would say we should change this all, or it would be really confusing.

If the Module should be renamed, I'm not sure about this.

Replaced all occurrences of the word 'picture' to 'responsive image' , everywhere it showed up on screen:
- in Extend (admin/modules)
- in Help (admin/help/picture)
- in Permissions (admin/people/permissions#module-picture)
- in Configuration (admin/config)
- In Configure responsive image mappings (admin/config/media/picturemapping)

Nothing has changed in the functionality.

When testing patch 8 in #9 in simnplytest.me I found some unchanged 'picture' words that were actually coming from commented code.
Included these changes in patch 9.

There is one occurrence of the word picture though that I cannot find inside the module:
It's visible on an important screen: admin/structure/types/manage/article/display
Where you can change the format of the image field
I have attached a screenshot. Could it be the module name it's showing?

Replaced all occurrences of the word 'picture' to 'responsive image' , everywhere it showed up on screen:
- in Extend (admin/modules)
- in Help (admin/help/picture)
- in Permissions (admin/people/permissions#module-picture)
- in Configuration (admin/config)
- In Configure responsive image mappings (admin/config/media/picturemapping)
- In Test

Nothing has changed in the functionality.
Solved simpletest errors found in earlier patches.
If patch 10 passes the test the only important question left is this one:

There is one occurrence of the word picture though that I cannot find inside the module:
It's visible on an important screen: admin/structure/types/manage/article/display
If you select the format of the image field it shows up.
Could it be the module name it's showing?
Not sure how to fix this.
See screenshot.

Re @ellishettinga: Awesome investigative work! :-) I think the remaining "Picture" you mean is caused by the PictureFormatter's annotation, see https://api.drupal.org/api/drupal/core!modules!picture!lib!Drupal!pictur...
i.e. where it says
label = @Translation("Picture"),

+++ b/core/modules/picture/picture.local_actions.yml
@@ -1,5 +1,5 @@
\ No newline at end of file

Can we fix this while we're at it? :-)

Thanks! @tstoeckler
I'll have another look

That did he trick !

Patch 11 replaces all occurrences of the word 'picture' to 'responsive image' , everywhere it showed up on screen:
- In the selectbox of image fields in node display screen (admin/structure/types/manage/article/display)
- in Extend (admin/modules)
- in Help (admin/help/picture)
- in Permissions (admin/people/permissions#module-picture)
- in Configuration (admin/config)
- In Configure responsive image mappings (admin/config/media/picturemapping)
- In Test

1. +++ b/core/modules/picture/picture.local_actions.yml
   @@ -1,5 +1,5 @@
   \ No newline at end of file
   

   Still would be cool to fix this :-)

2. +++ b/core/modules/picture/picture.module
   @@ -2,7 +2,7 @@
   - * Picture display formatter for image fields.
   + * Responsive Images display formatter for image fields.
   

   Images -> images

3. +++ b/core/modules/picture/picture.module
   @@ -41,8 +41,7 @@ function picture_help($path, $arg) {
   -      'description' => t('Administer Pictures'),
   

   Good call on removing the needless description!

Still have to apply this and click around a little bit but apart from these minor issues the patch looks great!

Will adapt code following comment #22 by @tstoeckler

Hmm... seems config_translation contains an explicit assertion for the message that appears when saving a picture mapping:
Raw "Picture mapping <em class="placeholder">xes5qbUY</em> saved." found.

ConfigTranslationListUiTest.php, line 375
Drupal\config_translation\Tests\ConfigTranslationListUiTest->doPictureListTest()

Adapt code following comment #22 by @tstoeckler.
Patch 11 failed testing because of a translation issue in Drupal\config_translation\Tests
I added the change required to pass this test (at least it did locally) to patch 12.
Not sure though if this should be done here.
Let's see what the bot says...

Read through the patch again and clicked through the UI a bunch and couldn't find anything to complain.

Awesome work!

Looks like webchick was here early, but giving her a chance to look again before this goes in.

I'm not sure why we didn't go the whole hog and actually rename the module and API functions too here - is that a follow-up?

Well since is still exposing a UI for managing the HTML <picture> element, the (sort-of) consensus above was that for developers the "picture" naming actually makes most sense.

Well it's supporting both picture and at least partially relying on srcset too. If browsers end up implementing srcset but not <picture> we might change that internally no?

I have mixed feelings on this.

Responsive images is probably a friendlier title for this.

It does make make me a tad uncomfortable, because the picture element are just one element of the larger field of responsive images. Picture is probably the best solution for raster images, particularly in the content area of a page.

However, using SVG graphics for vector art like logos is also an example of responsive images. Using media queries within CSS for switching out sidewide raster images is another example of responsive images. Icon fonts. Unicode graphics. All of these are different types of responsive images.

The picture module is where you go to set up images styles for the picture element, not any of those other responsive image solutions.

Srcset is a special case, as it works in conjunction with picture (although it can stand on its own for retina image switching).

When it comes to field formatters, as ellishettinga demonstrates above, I do think having the option say Picture rather than Responsive Image is probably more accurate.

It's a balancing act between accurately describing what this module does and ease of use/site builder education. There's probably challenges with each: if the module is named Responsive Images, there could be education needed to explain that there are other responsive image options for different situations. But for most site builders, responsive images in the content area or in blocks is probably going to be the biggest need, so maybe that label does make the most sense.

I do think if we are going to go with Responsive Images as the module it would be best if everything had that label, including the module name. It's going to be confusing if the interface says Responsive Images module, but then within Drupal core folders, the module is labeled picture.

Mozilla is implementing picture support in the first quarter, and Blink is developing picture support right now. The chance of the picture element being discarded at this point is pretty unlikely (although it's been quite the saga to get to this point).

I think I'm leaning towards labeling everything Responsive Images (including the field formatter, which could be labeled Responsive Image).

In any case it seems we could commit this and handle the full rename in a follow-up then?!

28: drupal-rename-picture-2124377-12.patch queued for re-testing.

I hit "retest" here because I just noticed this issue and I had recently committed a change to the hook_help() for this module. It probably conflicts with this patch, and even if it doesn't, you might want to look at the new help and make sure the text in the help is still all consistent with the new name and UI in this module.

28: drupal-rename-picture-2124377-12.patch queued for re-testing.

re-rolled patch 12

Thanks for the reroll! There's one additional change needed in the hook_help() [file picture.module, function picture_help()], where it still says "picture mappings" instead of the new "responsive image mappings":

... click the Edit icon and select one of the picture mappings that you have created. ...

Here's the fix as suggested in #41.

There are still a ton of mentions of picture with module names, file names, etc. I think if the name of the module changes, most of those code references should change too for consistency. I think it's confusing to have the module named one thing in the user interface and something else entirely in the codebase.

Doesn't the 'assigned to' status mean anything?

Sorry, I had started on the patch before the assigned status was set. Saw that when I went to upload, but I already had a completed patch. Just trying to help. Sorry.

The changes to the hook_help() look correct to me probably... It would be helpful if someone would apply this patch, turn on Picture... er... I mean turn on the Responsive Images module and the Help module, go to the help page for the module, and verify that everything that is in the new help text matches what you see in the new user interface (page titles, field names, select list options, etc.).

...module names should be Proper-Cased ;)

44: drupal-rename-picture-2124377-43.patch queued for re-testing.

Because the patch failed when applied locally.

About the casing: "Responsive Images" is used for the name of the module and in the text when the name of the module is mentioned. "Responsive images" is used everywhere else in the text when the images itself are targetted.
Is that not good?

re-rolled patch 43 according to this excellent explanation https://drupal.org/patch/reroll
see what happens

In the sandbox I saw yet another occurrence of the word 'Picture' in /admin/structure/types/manage/article/display.
Working on it.

Again, to make the reviewing easier:

This patch applies to the word Picture:
- In the selectbox of image fields in node display screen (admin/structure/types/manage/article/display)
- In the selectbox of image fields in node display screen (admin/structure/types/manage/article/display) after you have chosen Responsive image in Format click configure too!
- in Extend (admin/modules)
- in Help (admin/help/picture)
- in Permissions (admin/people/permissions#module-picture)
- in Configuration (admin/config)
- In Configure responsive image mappings (admin/config/media/picturemapping)
- In Test

This patch is looking pretty good, thanks for finding that additional spot that needed modification!

I went over it carefully and found a couple of things to fix:

a) I went to core/modules/picture and did
grep -R Picture

Most of them were class names, but this was missed:

config/schema/picture.schema.yml:# Schema for the configuration files of the Picture module.

b) I also noticed a couple of minor issues in the hook_help() last topic (on assigning to image fields) -- and these were problems in the original help, not introduced by this patch, but it would be nice to fix them:

b.1) The field type is called "Image" not "image". This is capitalized inconsistently.

b.2) "...click the Edit icon and select one of the responsive image mappings..." Should be a comma before "and".

c) In hook_permissions, the user-facing name of the permission is "Administer Responsive images". This either needs to be "Administer responsive images" or "Administer the Responsive Images module" (the first seems more sensibe).

Thank you @jhodgdon for reviewing.
I changed the patch according to your comments except:

b.1) The field type is called "Image" not "image". This is capitalized inconsistently.
It is now more consistent: I changed the one loose "Image" in Help to "image". I guess it's up to a native english speaker to decide where "image" should be capitalized. I cannot do it/don't see it, for me it looks good as it is.

b.2) "...click the Edit icon and select one of the responsive image mappings..." Should be a comma before "and".
Because comma and "and" have the same function in a sentence and should not be used in combination (so I've been told) I changed it to ", after that"

Thanks for the new patch!

Regarding b.1, in the user interface, the field type is called "Image". We want hook_help() to match what the user sees in the user interface, so the field should be referred to as "Image". So for instance it should say "... in the display settings for your Image fields...", etc.

Regarding b.2, the wording you chose doesn't sound very good to a native English speaker. The sentence in question is:

Choose the format Responsive image, click the Edit icon, after that select one of the responsive image mappings that you have created.

It should be

Choose the format Responsive image, click the Edit icon, and select one of the responsive image mappings that you have created.

The other changes look good to me.

@jhodgdon Thank you for your explanation. Is it OK now?

Looks good to me, thanks! I think the patch is consistent now. I am not going to mark it as "reviewed and tested by the community" because I am not sure that the change has been fully decided on, but I think that the patch is good, assuming that the ending name of the module is expected to b e"Responsive Images" and that we are leaving the machine name as "picture" (which seems really confusing to me).

I could go either way on whether the module is named Picture or Responsive Images. The latter might be more user friendly for those new to the topic. I want to see people actually use this with their sites, so if naming it Responsive Images helps with that, then probably a good change.

I still don't understand the reasoning for making the change in the UI but not changing the name of the module itself. I agree, that seems confusing. What's the reason for not changing the module name? Does anybody know?

Re-reading some of the comments above I'm going to be bold and say that there is sufficient consensus on the rename.

It has also been suggested to rename the entire module name (i.e. the machine name), and on that I'm not sure there is consensus (although I'd certainly be for it), but I think most people agree that this fixes a WTF for many users.

Needless to say, I can certainly be overruled.

I'm +1 on the rename ("Picture" vs. "Image" is very silly), though I also believe we should do the module files and the module rename wholesale in this patch. Without doing that, there's a risk that we ship Drupal 8 with this inconsistency, and that'll be extremely confusing trying to explain this to people.

So I think we can move forward here once the patch is re-rolled to take that into account. I'm also alerting attiks to this issue in case he has any last minute concerns, since he co-authored this code and is the maintainer of the D7 module. However, given the domain name of http://picture.responsiveimages.org/ which is referenced there, I don't forsee a problem with the name chosen. (Clarifying title for people who haven't been following this.)

I have to agree, naming the module 'picture' was not a good move, it gives all kinds of problems with translations (in Dutch both are translated to 'Afbeelding'), the only thing that concerns me a bit is that we will break all other patches, so when this patch is ready it should be committed as soon as possible to avoid that people have to re-roll other patches all the time.

Asking Jelle_S to take care of the rename

I guess the module has to be named "responsive_image" since all module names are singular.

All occurrences of picture (except where referencing the HTML5 picture tag or picturefill) should be replaced with this patch, which is based on the patch from #62. Let's see what the testbot makes of it.

FYI: I also replaced https://drupal.org/documentation/modules/picture with https://drupal.org/documentation/modules/responsive_image so if this patch goes in, that probably should be moved as well. Also, that page applies to the D7 version of this module, and has a link to documentation on how to add breakpoint groups through the UI, which can't be done (without a contrib module) in D8.

44: drupal-rename-picture-2124377-43.patch queued for re-testing.

New patch addresses the failing tests.

FYI: the component select box on this page needs to be updated once this is committed

MAINTAINERS updated as well

This patch has a mixture of calling the module "Responsive Images" and "Responsive Image". Which is it going to be?

Examples:
MAINTAINERS.txt - Responsive Image
responsive_image.schema.yml - Responsive Images

There are quite a few examples of both in the patch. Pick one. Stick to it. (I would actually think that since we have modules whose human-readable name is "Block", "Image", and "Menu", this one should be called "Responsive Image" not "Images".)

Also, the entity class is called ResponsiveImageMapping, but the comment at the top calls it a "ResponsiveImage" entity. That seems wrong? Related to this, I think some of the UI text is confusing. For instance, in the ResponsiveImageMappingDeleteForm class, the confirm message is "Are you sure you want to delete the responsive image %title?"? In the current code, the word "mapping" is in that message, which I think makes it clearer that you are just deleting this configuration entity item and not an image file.

And as an unrelated note, there is a "@var BreakpointGroup" in the ResponsiveImageMapping class file, but no "use" statement for that class.

It should be "Responsive Image"
Missing "mapping" added

Others are unrelated to this

Better!

A couple of things still to change, and I do not think they are out of scope, since they are lines that are changing in this patch:

a) in ResponsiveImageMappingListController:

/**
- * Provides a listing of Pictures.
+ * Provides a listing of ResponsiveImages.
  */
-class PictureMappingListController extends ConfigEntityListController {
+class ResponsiveImageMappingListController extends ConfigEntityListController {

There is no such thing as a "ResponsiveImage". This class actually provides a listing of "ResponsiveImageMapping" config entities. If you want to argue that this is not supposed to be a class name, then you would need to use plain English, in which case "ResponsiveImage" is not a word, so you could say "Provides a listing of responsive image mappings.".

b) Similarly in ResponsiveImageMapping:

 /**
- * Defines the Picture entity.
+ * Defines the ResponsiveImage entity.

There is, again, no such thing as a ResponsiveImage. The class is called "ResponsiveImageMapping" or in plain English, you could say "Defines the responsive image mapping entity.".

c) in the .module file:

+function responsive_image_permission() {
   return array(
-    'administer pictures' => array(
-      'title' => t('Administer Pictures'),
-      'description' => t('Administer Pictures'),
+    'administer responsive image' => array(
+      'title' => t('Administer responsive image'),

I think the human-readable text for this should say "Administer responsive images" or "Administer responsive image mappings". If you want to argue that this is the module name, then it needs to be "Administer Responsive Image", but otherwise, I think it should follow the pattern of the Block module and others:
Block ==> "Administer blocks"
Comment ==> "Administer comments and comment settings"
Image ==> "Administer image styles"
etc.

d) In responsive_image_mapping_load() docs:

- * @return \Drupal\picture\Picture
+ * @return \Drupal\responsive_image\ResponsiveImage

No such class. Should probably be ResponsiveImageMapping or ResponsiveImageMappingInterface here?

e) Unrelated, but OMG does this module still have theme_* functions in it??!?

Thanks for the feedback, Jelle is going to have a look at it

a/ true
b/ true
c/ "Administer responsive images" sounds good
d/ true
e/ see https://drupal.org/project/issues/drupal?text=theme_picture

New patch (and interdiff) with remarks from #79

Should have checked the interdiff first...

Looks better, thanks!

I think the "Needs manual testing" tag is good here. Someone really needs to go through the UI for this module (including the Help page) and make sure everything is consistent.

Tried to install on recent clone of D8. Appeared to apply cleanly but blew up when I tried to enable module. Appears module renames weren't done.

That error in #84 looks like you installed without the patch, or applied the patch after you installed, because it was looking for a file in core/modules/picture, which should not exist after applying this patch... at least, I would hope it would not exist after applying this patch? Let's see...

No, when I apply the patch, I don't have a directory core/modules/picture any more.

I think maybe you should try again dcrocks?

Tried again. Did a 'git clone' of d8, then did a 'patch -p1 < ...', only thing in responsive_image directory was the local actions yml file.

Um. That is not happening when I do patch -p1 < (patch file). I get a bunch of files and subdirectories, and core/modules/picture goes away. Did your patch command succeed? Do you perhaps need to re-download the patch in #81 (not #82 which is just an interdiff file)?

I did reload patch 81. I didn't see any error on the patch command but I will try to see where my went wrong.

ps. I am on OS X 10.6 installing into my sites directory.

Just an idea, but maybe try git apply vs. patch -p1?

That worked. Patch, for some reason, was ignoring the renames(and the delete) and applying the patches to the picture module files. Don't understand it.
Apply and install went ok, also enabled the responsive_image module. There are currently 2 themes, Adaptive and civi_bartik, that will have to rename there mapping yml files though. I've only done a quick check of the ui so far.

Fixed issue summary to be inline latest patch.

@dcrocks it applies just fine for me - and the patch looks code from a code perspective. All mentions of picture in the module relate to the picture html element as expected. Everything else has been change to responsive image.

'git apply' cmd worked for me, 'patch' cmd didn't. Don't know why.
Just noted that there are 2 themes out there using this and need to make appropriate changes after this is committed. Already added to their issue queues. Patch looks fine to me after light testing.

Perhaps should move this along.

All right, committing this while it's hot! :)

Committed and pushed to 8.x. Thanks!

Only relevant change record I could find was https://drupal.org/node/2164623 and I updated the table with this change, so I think we're good to go.

Edited title to correct new name of module.

looked into names while reviewing #1898442: responsive_image.module - Convert theme_ functions to Twig.
opened a novice #2222431: Project name clean up after Picture module renamed Responsive Image for some things I noticed.