picture size limit to define

not yet as far as I know Desparately waiting!

The best way to make sure they don't forget is by adding an RFE, though it has been said that they plan on releasing G2 with nearly all or all the features of G1. So its probably that this feature simply hasn't made it in yet.

virshu has volunteered to work on this feature.. thanks, virshu! I'll help out by providing design and technical guidance...

virshu, let's start with creating a new module.. we can merge into core later if we decide that's where it belongs. So start with a new module.inc that registers a new ItemEditAlbumOption (see randomhighlight/module.inc). The ItemEditOption should offer controls for:

• maximum image size by dimensions; can be a picklist like G1, but let's have it default to NxN but also offer to specify both dimensions.
• maximum image size by file size (in kb)
• checkbox for whether to modify original file or create preferred derivative

The option can save these settings in the PluginParameterMap (using the itemId of the album).
I'll provide more detail later on the next steps (event handler so that new albums will inherit the settings from its parent, and itemaddoption to apply the settings to newly uploaded images).[/]

wow virshu, fast work
looks good so far.. my comments/questions:

1. is the "default" choice needed? i don't think so.. what would it mean?
2. any ideas on a nice way to present the dimensions entry form to make it clear you can enter one dimension for square (just enter 800 and you get 800x800) or both dimensions for rectangle? one way would be a checkbox to the right that says "square" and when you check it then anything you type in one box gets duplicated in the other.. but if there is an easy-to-understand way that doesn't require any fancy javascript that would be good too. (I ask because we'll use the same type of form element when we change the image resizes to allow entering both dimensions instead of just one as it does now... there's a separate task for that)
3. why is jpeg/png specifically mentioned in the file size limit section? is this because the toolkits (imagemagick, etc) will only support changing the quality level to get a particular file size for these mime types?
4. i think the type check in isAppropriate isn't needed.. you registered as an ItemEditAlbumOption so it should only be used for albums.
5. perhaps use fetchAllPluginParameters to save a few getPluginParameter calls in loadTemplate.

Next step: event handler so new albums inherit settings from parent.

1. Update from cvs.. i made an api change in event handling today.
2. Take a look at useralbum/module.inc; add an event handler in your module.inc for GalleryEntity::save event.. when a new album is created (check entity type and check for STORAGE_FLAG_NEWLY_CREATED) then fetchAllPluginParameters for the parent album and save the same values for the new album.. skip this if the album has no parent (shouldn't really happen, but doesn't hurt to check..)

Finally, make it happen: take a look at exif ExifDescriptionOption.inc.. you'll want an ItemAddOption like this (with no UI, ie empty template). When your ItemAddOption is called then load the sizelimit prefs for the parent album and apply them to the newly added image (make sure it's a GalleryPhotoItem).[/]

[/]

as far as isAppropriate() is concerned, I think it is not appropriate to remove it... The call falls back to ItemEdit:isAppropriate which returns false, and the snippet doesn't come up...

I was just thinking about two approaches to "default" implementation ('traverse' and 'copy')... I think both approaches have some pros and cons... 'Traverse' is more dynamic - which may be good or confusing. Would 'copy' be consistent with other components in the gallery? That sure would tip the scale!

my biggest concern with 'copy' approach is applying these settings to existing albums; specifically, migrated from G1. Do you think, it will be possible to apply the settings from G1 during migrations (in other words, do you think, jmullan could retrofit the migration module to carry this setting over?)

In short, I am not married to one approach over another... If you feel that copying is better - so be it (it sure is easier to implement )

I got so far as to handleRequestAfterAdd. But I have no idea how different toolkits actually do the conversion... OK, tomorrow!

ok, here's one idea for the dimensions UI.. try it out
what do you think? it copies the width val to height after a delay.. but if you change the height to be different then it won't overwrite when you change the width.

I like "copy" with site-wide deault the best (if you remember, that was my original implementation of "new items" as well). Anyway, I'll do "copy" for now; it won't be a big deal to add site-wide default later. Moreover, my original concern about migration isn't relevant - now that I am thinking clearly. If we subclass addAlbum functionality, then I can set up the preferences for the root (or other parent) album, and these settings will be copied to the migrated album automatically, right?

I love the dimension UI implementation... Can I take it as is? Who do I need to credit/thank/bribe?... Oh, never mind - I just read your latest message. I think there is a delay for anonymous checkout, right? which module should I be looking for specific version?

I'll play with toolkit operations today and tomorrow (my teenage son took my car; can't go anywhere anyway :roll: ) I will probably have questions after that.

Somewhat off-topic: it seems that ItemEditOption:IsAppropriate and ItemAddOption:IsAppropriate are implemented very differently. ItemAdd is static function that doesn't have access to the $item... I could really use it! Is that by design or by accident 8)

Alright; back to work now!

Here is the code or the easy stuff (that is, without actual conversion... And I don't have the new template yet...

Just to clarify - I am planning to maintain the ratio (if I figure out how) - that is, if the values are 800x800 - it means that neither of the sides will exceed 800 pixels; not that my 3264x2448 rectangle will become 800x800 square (it will become 800x600, maintaining the ratio).

Also, I just realized something... when I have "default" sort order in subalbum - it means the global setting; not the parent album! I thought the opposite - that sort order traverses to the parents, and only if all the parents are "default" - it will go to the global setting. OK, my bad. That solves "traverse" vs. "copy" dilemma (at least in my mind). So, at some point it would be nice to set these global settings...

awesome! I was already panicking a little :cry: I think I am pretty much on my way for dimensions. The only remaining thought... if you specify 600x800 ('portrait'), and the picture is 1024x780 (landscape) will resize understand? or it will force 1024 into 600? Well, once it works - I can try, of course!

As far as filesizes... I remember in G1 it was like "trying 95% - size nnn; too much. trying 90% - size mmm. OK" Is this how it should be done?

1. For some resons dimensions.tpl still doesn't show in anonymous cvs; but it is in the nghtly; so I will just use that one... One thing I noticed. When I keep the original - I do it through "resize". Now, if I go to "edit photo" Rotate&Scale only recognizes preferred derviative for "rotate" and "scale". I realize that "resize" is used for generating thumbnails, so it is not as trivial as to add "resize" to the comparison, but it would be nice to note that there is an original, but we are dealing with derivative here. I hope I am clear here - if not, I'll try to elaborate. :-?
2. dimensions resizing are done; I was waiting to make UI changes before throwing it again at you
3. OK, G1-style trial and error it is until somebody suggest something better
4. hint is well taken... I was thinking about it except I have no idea how to write them. Is there some semi-automatic way ala localizations? :o

OOOHHH! It's my 100th message! I am an artiste now! Exciting![/]

1. good point.. i'll make the dimensions change in that view too.

note that {g->dimensions} now also takes width= and height= parameters for the initial values to place in the input boxes.

One other question/problem/enhancement request:

Is it possible to pass it "disabled"? If the current value is missing then I enable "none" and disable the text fields. Then I have some JavaScript that responds to clicks and toggles the fields. The toggling part works; but the initial state is always enabled...
Thanks

I have javascript to disable/enable on click (that is, when the user switches between "none" and "explicit" radio buttons). But I can't see the way to set the initial state (it's either in the field, or "body onload=" - I don't know any other way...

done... I forgot about this way :oops:

let me know what you decide about the toolkit!

try setting "no limit" for both settings and then adding a photo to the album.. i get an error.

haha.. but don't forget the toolkit changes will have unit tests too.. not like i'm getting off easy. though after 9000 lines of unit tests, they do get easier to write.

oh, sorry! lib/tools/phpunit/index.php is what you're looking for. you can enter sizelimit in the filter box to run just your tests (once you start writing them).
create modules/sizelimit/test/phpunit directory and put *.class files in there for your tests. each *.class can have a setUp() function to create any test data used in the tests and tearDown() to do any needed cleanup. each test is a function with the name starting with "test".
try copying modules/randomhighlight/test/phpunit/RandomHighlightOptionTest.class to start out your ItemEditOption test. you'll see how it calls handleRequestAfterEdit with various inputs.. and the basic checks are $this->assert(boolean, message) and $this->assertEquals(expectedval, actualval, message). you'll have more checks than this class because your form has more than a single checkbox. also don't put too many tests in a single test* function.. for example, tests for error conditions (user enters a blank or negative width/height/size) is usually a separate test from testing valid conditions.

i hadn't seen the bug report, but i'll take a look. i'll also test Gd with resize operation.. using imagemagick it seems the aspect ratio is maintained.. Gd should do the same.

ok, changed the operation to scale.
also added the compress operation to all 3 toolkits.

you should code your module to check for toolkit support of the compress operation.. if there aren't any then don't show the filesize controls.. if there is support you can customize the message for which file types will work.. maybe get the mime types and strip "image/" off the front of each and show the list. Right now imagemagick supports jpeg and png, netpbm is jpeg and pjpeg and gd is just jpeg.

Here is the culprit:

<html>
<head>
<title>Image Size Create From JPEG </title>
</head>
<body><p>
<?php
	$result = imagecreatefromjpeg('source.jpg');
    $ret = imageJpeg($result, 'dest.jpg', 99);
    echo "Size of first JPEG: " . filesize('dest.jpg') . "<br>\n";
    $ret = imageJpeg($result, 'dest.jpg', 70);
    echo "Size of second JPEG: " . filesize('dest.jpg') . "<br>\n";
?>
</p></body></html>

will return the same number twice despite the fact that the second time is obviously smaller...

Real men don't read manuals :evil: Which caused me better part of New Year's eve of wasted time and frustration :oops:

I noticed one other thing that I would like to change... I see that ItemAdd expects plugins to return status among other things:

list ($ret, $error, $status) = $plugin->handleRequest($form, $item, $optionInstances);

However, the plugins (ItemAddFromBrowser, ItemAddFromServer, ItemAddFromWeb) don't allow options to return status, but only errors: list ($ret, $optionErrors) = $option->handleRequestAfterAdd($form, $newItem);I think I could use status to show warnings in the ItemAddConfirmation screen. So, how about something like this:

list ($ret, $optionErrors, $optionStatus) = $option->handleRequestAfterAdd($form, $newItem);
$error = array_merge($error, $optionErrors);
status['addedFiles'][] = array('filename' => ..., 'id' => ... ,  $optionStatus);

Whadayathink?

One other observation... Per mindless advice I started reading the manual, and I think the way we are compressing the pictures (95, 50, 75, 62, etc.) is not appropriate for IM+PNG situation:

So, if I am reading it correctly, the two digits are evaluated independently. The tens affects the size (at the expense of de-compressions peed; not quality like in JPEG) and the singles define filter-type (which I have no clue about). But if the goal is to target certain size - we must treat the quality for PNGs differently...

Ok! I found some time to review this. In general, it looks like good work. I think that you've covered most of the cases that need covering. However, in order to get it into the codebase there are a lot of things (big and small) that need to be fixed up. The list is long, but for the most part it's pretty cosmetic stuff that won't take you too long to resolve. Since this list is so long, we should do another round or two of reviews after you fix all this stuff because I'll probably find more issues (it's hard sometimes to find all of the issues when there are a lot of little things).

This is pretty standard for a first round submission of code to G2. I'm being very, very stringent about this code base As is usual for me with the first round, I haven't tried to run the code yet -- I've only done a full read through. Next round I'll start playing with the functionality.

----------
General comments:
- use /* */ instead of // comment style
- use longer variable names instead of abbreviating (dimensionNone
instead of dimNone, etc)
- your tests look fairly complete and seem to cover the appropriate
cases -- good work!

SetSizeOption.inc:
- I changed it so that you can return a null template for loadTemplate
and it won't load anything, so you can get rid of Empty.tpl
- The underscore in Sizelimit_helper::_applyLimits and _buildDerivativeWithLimits
implies that they are private methods, so you should not be calling them from outside
the helper class, unless you want to make them public and remove the underscore.
- You don't check the error result from either of those calls (4 places)
- "x >> 10" is fine, but please add a comment explaining what it means

SizeLimitOptionTest.class:
- testTrivial() is unnecessary (unless I'm missing something)
- testNone() - form variables like 'rbDim' and 'rbSize' are unnecessarily
short -- why not just spell out what they mean, like radioButtonDimension
and radioButtonSize. When we spell it out that way, is it really necessary
for the code to know that these are from radio buttons? maybe dimensionChoice
and sizeChoice might be more appropriate names.
- avoid using print_r(..., true) because it requires PHP 4.3.0. For
assertEquals() you don't need to print out the result; it will do
it for you (and tell you where they differ).
- line 80: put spaces around => in array assignments
- testDimWidth() it's easier to do:
$this->assertEquals(array('x' => 1024, 'y' => 1024, 'keepOriginal' => 0), $params);
because assertEquals will display them nicely and tell you exactly what's different
- use single quotes instead of double quotes around any strings that don't
contain variables or control characters (\n, \r, etc) so that PHP doesn't
bother trying to do variable expansion
- the last few things are repeated in all of your tests

SizeLimitOption.inc:
- You should be storing your data in $SizeLimitOption not $SizeOptions; that way
your namespace matches your class name (and you won't conflict with another
class's template variables)
- handleRequestAfterEdit(): your GalleryCoreApi::xxxPluginParameter calls don't
check the return code
- in form[error][sizelimit][size][invalid], the convention is that the [size] part
matches the name of the form variable (so in this case it should be [rbSize]).
See my general comment about variable naming
- the form should be form[SizeLimitOption] to maintain namespacing (see my
comment above)
- <dimNone vs. dimYes> and <sizeNone vs. sizeYes> -- these don't really speak
to me. It's already in form[sizelimit][rbDim] so you don't really need the
dim prefix. For example, how about:
form[SizeLimitOption][dimension][unlimited]
form[SizeLimitOption][dimension][limited]
these make more sense to me.

SetSizeLimitOption.tpl:
- our templates are indented in steps of 2, so for example it should be:

  <div> 
    <script> 
    function foo() { 
    } 
    </script> 
  </div>

- all of your Javascript functions should have your namespace; remember that
you're potentially sharing a page with other Javacsript. Eg:
SetSizeLimitOption_toggleXY
- You can't count on document.forms[0].elements['g2_dims[width]'] because
we may not be using g2_ as our form prefix in all cases. You can use the
{g->formVar} there also, but you're really better off using a namespaced id
instead, so your input would be:
<input id="SetSizeLimitOption_foo">
and your Javascript would be:
document.getElementById("SetSizeLimitOption_foo")
look at WatermarkOption.tpl to see a clean way to do this
- "No Limits" and "Keep original image?" are not in {g->text} blocks

AddPhotoOptionTest.class
- Laima.jpg appears not to be part of the tarball. Is it required?
- tearDown(): You should not need to remove the item specific plugin
parameters; the framework will take care of that for you
- 112, 146, etc.: the assertEquals should be in the comment (ie, have a "* " prefix)
- 141, 187: youdon't check the return status on rebuildCache()

Empty.tpl:
- no longer required (see comment above)

module.inc:
- should be a blank line between performFactoryRegistrations() and handleEvent()
- 100: should be indented under the left paren
- in handleEvent() if it's not a save, you return a $ret->wrap() but you have
no ret. I prefer to structure it like this:

  if ($event->getEventName() == 'GalleryEntity::save') { 
    /* do all album save event processing here */ 
  } 
  return array(GalleryStatus::success(), null); 

Sizelimit_helper:
- this code looks very familiar Once your stuff is in and tested
we should consider refactoring this and the ItemEditRotateAndScalePhotoPlugin
code together in some fashion so that we don't have excess duplication.
- line 33 is missing a semicolon -- PHP should barf on this
- This class should be renamed to SizeLimitHelper in accordance with our
naming convention
- the function should have phpdoc
- 54: if you use $gallery->i18n() you're saying that you're returning an
internationalized string that will be localized later. However, I believe
that we pass those warnings directly out to the browser so it must be
localized at this time. You need to load the sizelimit module and then
call $module->translate() on your string before returning it.
- I don't see any tests for this code. You can probably adapt the ones from
ItemEditRotateAndScalePhotoPluginTest to get yourself started, though...

That is a terrible API! But what can ya do. We can extend the imagemagick module to have a separate compression and filter settings for PNG, but nobody is going to take the time to figure out what's the right filter to use. I changed it for now to hardcode the filter to "adaptive" for PNGs, but retain the compression setting.

Later on if we want to do it right we can let users specify both settings...