Library 7.x-2 introduce a new API. We should modify views_slideshow_cycle to use it.

The documentation of Library 7.x-2: https://drupal.org/node/1342238

Comments

DYdave’s picture

Hi gagarine,

Thank you very much for reporting this issue.

Indeed, there has been a great need for moving towards managing JQuery Cycle through Drupal 7 Library API.

It seems quite a lot of work, with an implementation of hook_library has already been done in #1240642: Views slideshow js is added in hook_init, in particular it seems a patch was committed at #1240642-14: Views slideshow js is added in hook_init (e4e377b), but it appears it would still need some testing and work (overhead impact #1066096: Support for multiple slideshows on one page reported at #15).

Could you perhaps try updating to the latest views_slideshow-7.x-3.x-dev and see if the latest committed changes work better?

Additionally, we would greatly appreciate to have your feedback, comments, issues, questions, suggestions or further recommendations on this issue, the latest commits or more particularly on the updated patch at #1240642-15: Views slideshow js is added in hook_init.
Any feedback, testing, reviews of the code and patches would be highly appreciated.
Thanks in advance!

gagarine’s picture

#1240642: Views slideshow js is added in hook_init use the core hook_library to load views_slideshow.js. I was speaking about the https://drupal.org/project/libraries module (already used for cycle).

I can certainly take the time to provide a patch.

DYdave’s picture

Hi gagarine,

Thanks again very much for your prompt and kind reply.

I'm not particularly convinced a dependency on the Libraries API would be absolutely necessary.
If the hooks and functions from Libraries API 2.x were to be implemented as part of the module, shouldn't a module dependency also be added to views_slideshow?
(If not, then I wouldn't see any problems with that)

Could you please explain a little bit more what it would bring to the module if Libraries API module was used, and that we couldn't already be able to do with the Core hook_library module?
Would that be absolutely necessary? Would the gain and improvement be very substantial?

Sorry, I'm not that familiar with the Libraries API module and would appreciate your feedback and guidance on that.

Any further questions, comments, issues, feedbacks, testing, reviews of the code and patches would be highly appreciated.
Thanks very much in advance!

gagarine’s picture

Component: Code » Cycle

Sorry rereading my bug report I notice it was absolutely not clear I was speaking about the "sub module" views_slideshow_cycle. I updated the summary.

As you can see in the info file we already use libraries to load jquery_cycle. This make sens because libraries module let you put a lib outside of the module folder (in the /library folder) so more than one module can use it.

My proposition only consist to update the actual code to the latest libraries API.

name = Views Slideshow: Cycle
description = Adds a Rotating slideshow mode to Views Slideshow.
dependencies[] = views_slideshow
dependencies[] = libraries
package = Views
core = 7.x

files[] = views_slideshow_cycle.module
files[] = views_slideshow_cycle.views_slideshow.inc
files[] = theme/views_slideshow_cycle.theme.inc

; Information added by drupal.org packaging script on 2011-10-26
version = "7.x-3.0"
core = "7.x"
project = "views_slideshow"
datestamp = "1319589616"
DYdave’s picture

Hi gagarine,

Thanks again very much for your prompt and very clear reply.

Indeed, I can understand much better now :-) .

Well, putting it this way..... then, sure!
Everything is very clear.

Personally, I'd love to help working on this, plus it seems very straight forward, but realistically speaking, I've already got quite a lot of stuffs going on, in terms of patches and modules maintenance (a lot more support requests to answer).
So I'm afraid I wouldn't necessarily be able to get back to you very quickly with a patch on this.

Therefore, patches would be more than welcome!
You could surely count on me as a reviewer/tester if any patches are uploaded in this ticket.

Otherwise, I assume @wangqizhong should be able to work on this and get back to you within a shorter amount of time (than me). Feel free to let us know what would work best for you.

Please also let us know at anytime if you would have any further questions, comments, concerns, issues, suggestions or ideas, we would surely be glad to provide more information or explain in further details.

Thank you very much for your great feedback, comments, help improving this module and keeping it up to the date.
Looking forward to seeing this issue's future updates.
Cheers!

gagarine’s picture

Assigned: Unassigned » gagarine

Cool, I can certainly work on that Monday.

gagarine’s picture

Issue summary: View changes

views_slideshow_cycle

NickDickinsonWilde’s picture

Assigned: gagarine » Unassigned
Issue summary: View changes
Status: Active » Closed (won't fix)

I'm looking at major changes happening only in 8.x now and given the current status of testing for 7.x (none) I'm leery of doing *potentially* breaking changes. So I'm not going to do this change; that said if someone has time for a patch I'll review it and test it manually.