From: Russell <zi...@zi...> - 2006-07-30 23:52:06
|
The original patch can be found here: http://tools.gallery2.org/pastebin/684 Termite: Could you give me a brief explanation of what your changes do and why a user would want them. This will help us evaluate the usefulness of your changes as well as make other suggestions. From my understanding of what it does, it provides an alternate way of displaying the items in the cart by listing them all as links. Line numbers are based on the full file after adding the changes in. Developers with access to Kompare (KDE) can run "kompare -b modules/cart/ cart-changes.diff" to see the changes in-line. General Gallery uses a indent-width of 4 spaces and a tab-width of 8. If your editor does not support this then please use a consistant style of just four spaces for indents; we can change it from there. Gallery wraps lines at 100 characters. Lines and strings should be broken up into multiple lines per the Coding Standard: CartAdmin.inc If this file is for SiteAdmin then it should be renamed CartSiteAdmin.inc, as there is also a UserAdmin and ItemAdmin interface. 3 - 5: I don't think we put those there anymore. $Revision$ should be put in @version on line 25. Example: @version $Revision$ $Date$ 30: You should have a little phpDoc tag above the class name that gives a little description, @package Cart, and @subpackage UserInterface. See another module's .inc file for an example. 39: No space in "array();" 44: The // comment tag isn't allowed in Gallery. You'll have to change it to /* Save settings */ 48: Please do some input validation before saving the form value. If you only expect thins to be two or three values then do an array check against those values. We don't want a malicious site admin to craft a setting that might expose anything up- or down-stream. 48: You also need to check for $ret = with each call to setPluginParameter. 51 & 53: Since the only change you're doing to $status is to set it to 1, go ahead and do $results['status']['saved'] = 1; 73: There should be a space in list ($ret, $param) 75: With Gallery 2.2 (svn), we no longer need to $ret->wrap() the return. You can simply return array($ret, null); now. 78: Space in "foreach () {" 79: I've been talking with virshu and we can't decide if this method of assigning variables to the template is appropriate (or what the difference is). The CartAdmin.tpl file is missing for the most part so we can't tell what it is doing with the variables to figure out if $template->setVariable should be used instead or not. ModifyCart.inc 55: Brackets are required for all language statements. The first bracket always goes on the last line of the statement and the last bracket always goes on a new line. This will also keep this line from going over 100 characters. module.inc 144: Space in "list (" 144: Since you're only using one parameter in this section, consider changing it to GalleryCoreApi::getPluginParameter() for the one specific parameter instead of all of them. 160: The second part of the if statement should use a == to compare instead of set $param. You shouldn't need the parenthesis around that part of the statement either. "and" should be "&&". The line is also longer than 100 characters so you'll need to wrap it on the next line, with the && on the second line as well. 162: Because you're translating a static string you only need $this->translate(). $this->_translate() is used when the translated string may contain other values (like on line 156). ViewCart.inc 48: Only get the 'LinkType' parameter if that's the only one you're going to use here. templates/CartAdmin.tpl This file seems to be missing. 2: Simply use $Revision$ here for new files. templates/ViewCart.tpl Could someone that has more experience with templates look at these changes for me? Thanks. |