Reorganize effects

You are quite right that the grouping is historical and not at all planned.
At the time, the concern was to
* avoid a huge 5000-line file with everything in
* avoid bootstrapping all that code that would only get invoked less than 1% of the time
* Add some effects 'brand new' without disturbing existing code - autorotate was a stand-alone contribution that was adopted into the family.

Way back then, Drupal didn't have a history of using lazy-loaded plugin libraries - modules generally included all their own code and the idea of splitting things into inc files was new.
Some of the grouping was (as you've observed) to get a little code re-use inside a library - but thats just architectural choices for conditions that are no longer valid. A number of those needs have been ironed out, and I don't THINK there are any actions left that sneakily call each others routines (the overlay subroutine used to be a sneaky workhorse for a few other effects - but it's now being called through the API in all cases I think).
I didn't used to have a central imagecache_actions-core library of re-usable stuff then either.

TODAY, I think your suggestion is great - but we if refactoring, we may as well take it further.
When we can't find a good name for a group of actions, or clear reasons why something should be one place or another - it means that support & docs cannot be intuitive, and the grouping or distinction is probably unnecessary.
SO: Why not do it properly and

• Bring all the action declarations back into a central module
• split all the inc files off into separate libraries

I think the implications would be in how the existing image effects remember the module name that provides processes, so there will be a hook_upgrade needed there. This would be a major point release, so refactoring should probably be done in 7.x-2.x

I'm keen for this

Bah. Another reason to hate that dimensions callback.

I say no. We don't need another layer of plugin callback stuff just to deal with the *three* phases of effect hooks.
I see where you are going with this, but I feel that for the code we have here, the added overhead (and mental complexity) would be greater than what we save by trying to push things out into additional inc files.

I'd have to refresh my memory, but if the callback file can be defined adequately in the hook_image_effects_info, then we are probably OK still.
Edit: Damn, it's not. I must have been thinking of another thing.

Most of our effects - the actual processing part - are not more than a dozen lines ling. The admin forms for them are often quite a lot bigger than the job itself.
It's rare that the processing phase requires too much extra code - the alpha-pixel-blend routine is already in its own lazy-loaded lib - partly for efficiency, partly to split off externally-sourced contrib code.
And it will be really annoying if we had to maintain the dimensions callback in a separate place from the process callback. It's not worth trying to split those off.

> Thus, if the latter makes that all effect code is loaded on "every" page request we miss the goal of lazy loading.

I don't mind if the *process* code for one effect is loaded if it's required to get the *dimensions* calculation. The lazy-loading I care about is avoiding the process code for every available effect, even the unused ones being loaded all the time.
We have 20 effects but the site uses only 3. It's OK that all the related code for those three effects is bootstrapped, it's not great if all 20 are.
... and if we do proceed with splitting each effect into its own file - rather than the groups of 3 or 4 we have now - we will be winning there already! The greater granularity of individual effects will have better payoffs than trying to split the parts of each effect into phases. (I predict)

So, my attitude is
* The perfect is the enemy of the good.
* Avoid premature optimization (unless you are prepared to benchmark it)
* Don't sacrifice readability, maintainability, brevity or sanity for lazy-loading tricks

So if we can avoid doing our own plugin layer, lets leave it out. Ideally it should be driven from core (where the hook_info could define callback files).
As it is, I guess we still have to put some sort of delegator into the .module file to catch the requests coming from core and pass them on to the inc files. As long as the incoming arguments include the name of the effect somewhere, that can probably be done pretty cleanly.
Damn, I really was thinking that the core info would allow us to specify the code file...

Yes, any other cleanups can be done in the current branch first - I like to do housekeeping sort of stuff at this point also.
Changes that will require a proper hook_update - and that certainly will include renaming or moving filenames around - should be the next branch.

Hm. It's a bugger that the name of the effect isn't immediately available. I guess we could insert it into the data consistently when settings are returned and pull it out later. Messy.
Your hook_image_styles_alter() idea would probably be a little cleaner than that.

To go to the other extreme, the system
* as it stands right now,
* and as it was initially set up in the imagecache days,
* and the way in which it is simplest to add just one more action any time you want ...

Is to have each effect in its own module. :-)

I know we all have an aversion to adding too many modules to the list ... but with the D7 registry cache, it's possibly even more efficient to have a dozen trivial modules than to try and force it all into one and then add our own autoloader abstraction into it anyway.
It could be that trying to group them into bigger packages at all is premature optimization with no demonstrable performance advantage.

Conceptually - one module : one effect - is clearly the easiest to develop and extend. It might be worth stepping back and giving that another think about.

fair enough

I know what you mean. Yeah,it could be worth a try, though I don't know what shape D8 plugins are.
If it means a big OO rewrite, it could be scary. But I agree, if there is any refactoring, it should be to move in the direction of what it has to be in the future