Last modified: 2012-02-23 16:33:26 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T36411, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 34411 - [SRF] 1.7 [patch]: SRF_Gallery.php introduce new parameter galleryformat for jcarousel plug-in
[SRF] 1.7 [patch]: SRF_Gallery.php introduce new parameter galleryformat for ...
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
SemanticResultFormats (Other open bugs)
unspecified
All All
: Unprioritized normal (vote)
: ---
Assigned To: Jeroen De Dauw
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-15 07:36 UTC by MWJames
Modified: 2012-02-23 16:33 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
SRF_Gallery.php patch (463 bytes, application/octet-stream)
2012-02-15 07:36 UTC, MWJames
Details
Patch on SRF_Gallery.php (13.31 KB, application/octet-stream)
2012-02-21 10:57 UTC, MWJames
Details
SRF_Gallery.php Patch 0.1.2, includes 0.1.1 plus RTL support (13.68 KB, application/octet-stream)
2012-02-21 12:34 UTC, MWJames
Details
Fix indentation (13.43 KB, application/octet-stream)
2012-02-22 07:34 UTC, MWJames
Details
SRF_Gallery-Carousel-0.1.3.patch for multi instances display (13.83 KB, application/octet-stream)
2012-02-23 13:15 UTC, MWJames
Details
Version 0.1.3, patch against r112205 (4.24 KB, application/octet-stream)
2012-02-23 14:34 UTC, MWJames
Details

Description MWJames 2012-02-15 07:36:05 UTC
Created attachment 10014 [details]
SRF_Gallery.php patch

SRF_Gallery.php produced the following error:

Notice: Undefined property: StubObject::$mOutput in ...\extensions\SemanticResultFormats\Gallery\SRF_Gallery.php on line 166

Fatal error: Call to a member function addImage() on a non-object in ...\extensions\SemanticResultFormats\Gallery\SRF_Gallery.php on line 166

Patch tested on:
Semantic Result Formats (Version 1.7); Semantic MediaWiki (Version 1.7.0.2); MediaWiki 1.18.0; PHP 5.3.8; MySQL 5.5.16
Comment 1 Jeroen De Dauw 2012-02-20 18:53:46 UTC
I applied this as part of your patch at https://github.com/mwjames/smw-srfgallerycarousel

This looks a bit odd to me though: !$imgTitle->getDBkey() == null

I read that as: if the negation of $imgTitle->getDBkey() is 'sort of equal' to null. Is that what you want it to do?

Please use is_null().
Comment 2 MWJames 2012-02-21 10:57:19 UTC
Created attachment 10058 [details]
Patch on SRF_Gallery.php

Hope this works, the patch includes: 
* Use is_null() instead

and since I'm bit lazy I just put in some other changes related to SRF_Gallery.php

* Tweak css so vertical display is similar to horizontal css
* Use perrow parameter as indicator for the amount of visible and scrollable elements
* Prepare other optional paramters that are not implemented yet
* The jcarousel developer suggested to use wrap=both instead of wrap=circular as default
Comment 3 MWJames 2012-02-21 12:34:26 UTC
Created attachment 10061 [details]
SRF_Gallery.php Patch 0.1.2, includes 0.1.1 plus RTL support

This patch includes all above plus 
* RTL CSS support
Comment 4 Jeroen De Dauw 2012-02-21 22:03:51 UTC
Can you please use tabs to indent like done in these files already? I fixed the indentation last time, but am not going to keep doing that.

Looks good apart from that.
Comment 5 MWJames 2012-02-22 07:34:35 UTC
Created attachment 10065 [details]
Fix indentation

OK, this should fix the indentation for all required files including the latest change from r112069
Comment 6 MWJames 2012-02-23 13:15:56 UTC
Created attachment 10079 [details]
SRF_Gallery-Carousel-0.1.3.patch for multi instances display

Ah, hope this is the last patch but I finally got the multi-instance display working which means that with 0.1.3 more than one carousel can be displayed per page, the display of a standard gallery combined with gallery carousel works as supposed to be.

All previous changes are included and tested with Chrome and Firefox
Comment 7 Jeroen De Dauw 2012-02-23 13:21:42 UTC
Great. Applied in r112201, and tested with trunk :)

Some comments though:
* Please provide patches against trunk. If someone makes a change on SVN and you just incorporate it in your patch, merging will result in a conflict since the code is already there.
* You where doing some false selection in the JS :) 
* If you have a dictionary of stuff in JS (ie { stuff: ... }), then don't put a comma after the last element, this makes IE cry (yeah, really >_>)
* Avoid aligning assignments in most cases, can be useful in setup files with long lists of similar assignments, but rarely in other places.

One improvement possible in the JS is getting rid of all the var and just assigning directly in the dictionary, like this:

( '#carousel' ).jcarousel( {
    parseInt($( '#carousel' ).attr( 'scroll' ) ),
    ...
Comment 8 Jeroen De Dauw 2012-02-23 13:22:41 UTC
Ahhh! Bad timing :)

Can you provide a patch against trunk? :/
Comment 9 MWJames 2012-02-23 14:34:13 UTC
Created attachment 10080 [details]
Version 0.1.3, patch against r112205

Let's see if we hit the bullseye this time
Comment 10 Jeroen De Dauw 2012-02-23 16:33:26 UTC
r112215

* Please don't prefix stuff with 'm_' or 'm'
* Code style convention for both JS and PHP is to have spaces between round brackets if there is something in between, so .attr( "id" ) rather then .attr("id")

I made some changes to the PHP (since the merge failed anyway... looking foreward to git :). The style mismatch is really trivial, no need to make another patch for that.

This script allows you to mass fix style issues btw: http://svn.wikimedia.org/viewvc/mediawiki/trunk/tools/code-utils/stylize.php?view=co

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links