Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suffix image sizer option causes focus change to not generate new variation #703

Open
Toutouwai opened this issue Sep 18, 2018 · 23 comments

Comments

Projects
None yet
8 participants
@Toutouwai
Copy link

commented Sep 18, 2018

Short description of the issue

Normally every time the focus area for an image is changed in Page Edit a new variation is generated and visible on the front-end (browser cache may need to be cleared to see the changed variation).

This can be seen in these front-end screenshots before and after the focus area is changed (browser dev tools are open to disable caching).

<img src="<?= $page->image->size(200, 300)->url ?>" alt="">

2018-09-18_213741
2018-09-18_213805

But if a suffix is passed in as an image sizer option then the variation no longer updates after a focus change.

<img src="<?= $page->image->size(200, 300, ['suffix' => 'test'])->url ?>" alt="">

Expected behavior

Suffix has no impact on focus change generating new variation.

Optional: Suggestion for a possible fix

It would be great if the focus area was included (perhaps in an encoded form) as part of the variation file name so changing the focus area would reliably result in a new variation being loaded on the front-end regardless of suffix and would take care of cache-busting.

Setup/Environment

  • ProcessWire version: 3.0.113
@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

When someone uses a suffix, the idea is that they've done so intentionally to create some kind of custom variation (via $options). At that point it's no longer managed by PW as an auto-generated variation, because it doesn't have the information necessary to re-create it.

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

At that point it's no longer managed by PW as an auto-generated variation

I'm not sure what you mean by "auto-generated variation". I thought the admin thumbnail was the only variation automatically generated by PW. Other variations are created manually/deliberately by size() calls in template files, etc, possibly with various image sizer options passed in that are expected to affect the resulting variation.

I think that between this issue and the other one relating to changing the focus area there is a bug. Which part is the bug depends on what is supposed to happen if a user changes the focus area of an image.

Changing the focus area of an image could be treated in one of two ways:

  1. The changed focus is an important change and should result in new variations being created and shown on the front-end. In which case unrelated image sizer options such as suffix shouldn't be a factor in whether this happens or not (there is already an image sizer option to ignore focus if that is desired). I'd prefer this option, but if the variation is to be regenerated as a result of a change in focus then it should be regenerated at the time it is requested by size(), where all the relevant image sizer options are known. At the moment it seems that PW is recreating (some) variations at the time the focus is changed, but I don't see how that is going to work well because PW cannot know the relevant image sizer options at that time.

  2. The changed focus is not an important change and new variations do not need to be created if a variation at that size already exists. In which case the situation described in the other issue shouldn't occur. I think this option would be confusing for site editors, because if they are experimenting with the focus area to achieve a particular crop framing on the front-end then they need/expect to see those changes on the front-end.

It seems like going with option 1 would be simplified if the focus data is reflected in the variation filename. Then a change in focus automatically results in a new variation, which I think is what users and devs expect. And it takes care of cache-busting on the front-end too. As I've mentioned in an earlier request, I think it would be good to make it configurable to include more of the image sizer options that have affected a variation in the variation's filename. This could be done via $config settings, but maybe a config screen would be nicer:
2018-09-21_105015
That screenshot is from an unreleased custom module but I think it would be better to include at least some of this in the core.

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

This issue is related: #702 (closed to minimise the number of open issues in the repo)

@teppokoivula

This comment has been minimized.

Copy link

commented Feb 16, 2019

My comment from the now closed related issue, for reference:

When focus change in admin triggers variation recreation, this happens without knowledge of settings that the developer provided via code when first creating that variation, and as such may result in a very different outcome than what was originally intended.

For this reason I think that we should either store every setting available via code somewhere so that we can reuse them for that new variation, or alternatively we should not attempt to recreate variations and rather let the (developer defined) code perform that task – otherwise we will end up with unexpected results.

@knoedelsalat

This comment has been minimized.

Copy link

commented Feb 25, 2019

This also seems to be an issue when using "cropping" => "false" in image size options. If I use

$img_large = $image->size(1200, 1200, $options = [ 'cropping' => false ]

in a template, the image will be generated within 1200x1200 px limits without being cropped, working as expected.

Then, if the focus point is being changed afterwards, the image variation will be instantly recreated as a 1200x1200 cropped version. Then on page call the cropped version will be displayed. Gave me a bit of a headache last night :)

In this case I actually resorted to using a suffix to prevent PW from recreating my image variations for that particular template

@reminders reminders bot added the reminder label Feb 26, 2019

@reminders

This comment has been minimized.

Copy link

commented Feb 26, 2019

@netcarver set a reminder for Mar 12th 2019

@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@Toutouwai @teppokoivula You've both mentioned including every option in the filename at some point, and I think that would resolve what @knoedelsalat mentioned as well. However, I can't just introduce that because it would potentially wreak havoc on existing installations. What I could do though is make that behavior another option to the size() call, and/or a $config option, and/or make it enabled for new installations automatically. I'm thinking most probably don't need this, and it might also result in some pretty verbose filenames, but I think it'd be a useful option to have so long as one can choose to enable it or disable it.

@reminders

This comment has been minimized.

Copy link

commented Mar 1, 2019

@netcarver set a reminder for Mar 15th 2019

@teppokoivula

This comment has been minimized.

Copy link

commented Mar 1, 2019

@ryancramerdesign, while I think that including every option in the filename would be a possible solution, I'd like to highlight one part from my comment:

... alternatively we should not attempt to recreate variations and rather let the (developer defined) code perform that task ...

What do you think about this – does it make any sense? Personally I think that this re-creation feature is a nice touch, but also not necessary – so unless there's an easy way to make it aware of all the options, it would be much better to simply disable it. I'm not sure if that's currently doable, but if not completely, at least we could provide a toggle for it, so that it's disabled by default in new setups.

If it was indeed configurable, it would be relatively safe to assume that if the developer has switched it on, they should be aware of potential side effects.

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Mar 2, 2019

I agree with @teppokoivula that regenerating variations at the time the focus is changed is not necessary - the variations should be recreated next time they are requested via a size() call.

I like the idea of a $config option to include more/all sizer options in a suffix to the variation name because as a general rule if I change any size() option I want to see that change reflected on the frontend. I already have my own solution for this (the module I mentioned above) but would welcome seeing it as an option in the core.

As for the filenames being overly verbose, my module produces a suffix like the one below which I don't think is excessively long:

frog.400x400-u0i1s1q90f1t420l710z0.jpg

That suffix would decode as:

  • upscaling: false
  • interlace: true
  • sharpening: soft (I assigned an integer to represent each sharpening option)
  • quality: 90
  • use focus: true
  • focus position top: 420
  • focus position left: 710
  • zoom: 0
@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@Toutouwai Nice job with the module. I think this is good having this solution available as a module like this for the people that need it. If it turns out over time that the audience is significant then I'd suggest we copy your addVariationSuffix() method into Pageimage/Pageimages directly if you are open to it (enabled by another option or config('installed') date). Except that rather than using a selectable set of options to affect the suffix, it would just use any option that differs from a defined set of core defaults, or any in config('imageSizerOptions') that differs from those defaults.

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

@ryancramerdesign, if you later decide to add something like this to the core it would be fine with me.

But if you do that I think it would still be good to somehow give the user the option to ignore certain ImageSizer options from the suffix. For example, a user might be undecided about what default sharpening level, quality or interlace option they will settle on. They might want to change these later without causing all existing variations to be regenerated.

Is the general consensus that this issue should now be closed and anyone wanting to change the core behaviour on this (and the other related issue closed previously) should install my module?

@teppokoivula

This comment has been minimized.

Copy link

commented Mar 5, 2019

Is the general consensus that this issue should now be closed and anyone wanting to change the core behaviour on this (and the other related issue closed previously) should install my module?

Honestly I still think that this behaviour should be disabled by default until it can be guaranteed to work as expected – i.e. take the settings provided by developer into account –  in as wide range of cases as possible.

@ryancramerdesign, if you could perhaps provide some feedback regarding aforementioned idea, or the alternative that maybe there could be a setting for enabling / disabling the regeneration behaviour completely on a case by case basis, I'd be happy to hear it :)

@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@Toutouwai @teppokoivula I think the best bet is to monitor usage of the module over time. Toutouwai can let us know if it seems like usage is high enough to warrant inclusion of the option in the core down the road. Even if it were to be added to the core at some point then I agree it should be disabled by default, and perhaps one could enable and configure the behavior of it from the config.imageSizerOptions or one could also specify an option Pageimage::size() method to use it.

@knoedelsalat

This comment has been minimized.

Copy link

commented Mar 5, 2019

I think what @teppokoivula means is that right now all variations auto-regenerate on a focus change, producing unexpected results in some cases, and if it's a possibility to disable that for the time being.

The only problem I see with that is that PW would need to delete all variations on a change, and then only regenerate them on size() calls. But this would probably also affect variations from within RTEs..

@teppokoivula

This comment has been minimized.

Copy link

commented Mar 7, 2019

@knoedelsalat, correct. I'm getting the feeling that we're talking about two different things here :)

  1. I'm saying that the auto-generated thumbnails are currently unreliable, so this feature should be either disabled (by default) for the time being, or at the very least the core should provide an option to disable it manually (via config, or via settings on module or field level).

  2. The module, and adding all those variables in file name, is another solution for this issue. I do think that it, or something similar, should be in the core – but until then, we should have some way of dealing with this issue (see 1.)

My main point is that until the re-creation feature is reliable enough, it probably shouldn't be an (enabled by default) core feature. Currently it can cause issues that are hard to figure out, and may go unnoticed by content editors and admins while still lowering the usability or UX of the site.

Anyway, I don't want to drag this issue on. If Ryan feels that the feature is solid enough, we'll just have to agree to disagree about that – whatever his decision is, we'll roll with that. Currently I'm just not sure that we're actually talking about the same thing :)

@szabeszg

This comment has been minimized.

Copy link

commented Mar 7, 2019

the idea is that they've done so intentionally to create some kind of custom variation

I would like to point out that the docs does not help us to figure out if there is an "idea behind" the usage of the suffix or not. All we are informed is: "Suffix word to identify the new image..."

Needles to say, the docs should be clear on what the suffix is for and what are its limitations.

I also started to use the suffix in order to identify the variations generated by the admin or by any other source. In config.php I include 'suffix' => 'thumb' in $config->adminThumbOptions and on the frontend I also add the same suffix so that I use the same variations for a gallery. This is to save on the number of generated variations (eg. storage space / file nodes).
My other intention was to use the suffix to identify where a given variation is used.

@reminders reminders bot removed the reminder label Mar 15, 2019

@processwire processwire deleted a comment from reminders bot Mar 15, 2019

@horst-n

This comment has been minimized.

Copy link

commented May 4, 2019

@ryancramerdesign , @teppokoivula , @Toutouwai Sorry, I missed this one, so be a bit late to it.

A) I think automaticaly recreation when changing the focus point should be optional, means should be possible to enable/disable (maybe on image field basis, or site wide?).

B) What about storing the complete final options array, that was applied for variation creation, into a reserved IPTC tag? (serialized) It is used with the CroppableImage settings too. And we already have the code in imagesizer for more than 5 years. The options array get stored with the variation file, so if it exists and need to be automatically recreated, it holds all relevant data by itself, regardless of filename parts and suffixes! :)

@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@horst-n

  1. Makes sense, I agree.
  2. I like the idea of using an IPTC tag (JPG/PNG) but my understanding is that it's not universally supported in all image formats? For instance, I don't think GIF supports IPTC tags?
@horst-n

This comment has been minimized.

Copy link

commented May 4, 2019

@ryancramerdesign

  1. Ah, damn!
    Yes it is only supported with JPEGs. :(
@horst-n

This comment has been minimized.

Copy link

commented May 4, 2019

@ryancramerdesign

  1. Sidecarfiles? Not as nice as IPTC storage, but maybe a combined solution:
    IPTC for JPEGs and sidecarfiles for PNG & GIF
@matjazpotocnik

This comment has been minimized.

Copy link

commented May 4, 2019

Why not sidecarfiles for all?

@horst-n

This comment has been minimized.

Copy link

commented May 11, 2019

@matjazpotocnik Storing the data physically within the imagefiles is more robust then sidecar files. And this functionality is already included into the imagesizers. Therefore I think it is a good solution to use IPTC where applicable and sidecar files where IPTC is missing.
And I believe that in most sites the amount of JPEG files is multiple times of that for PNGs or GIFs. What would keep the amount of sidecar files low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.