I reworked the options form, sorted the different options into categories and added a whole bunch of new options - nearly every option listed in the official Galleria documentation is supported.

Would be great if you could integrate this into the official branch!

CommentFileSizeAuthor
#1 more_options2.patch27.53 KBkroimon
more_options.patch27.53 KBkroimon

Comments

kroimon’s picture

kroimon commented
StatusFileSize
new more_options2.patch27.53 KB

Found a small error in the previous patch, sorry for that.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Cool suggestion. What's with the commented lines at the beginning?

In addition we should remember that extended / custom themes might suppotr more options or possibly a different option set...
I'm still unsure in how to handle those cases. We either could merge with a custo textfield (json settings notation) or even need submodules for each extended custom gallery and have a ajax/js controlled options page..

Finally this all should fit per-galleria settings case and not global only.

kroimon’s picture

kroimon commented

The commented lines are all those settings I thought were unimportant, but are there for future reference ;-)

Yeah, the possibly most flexible way would be to store all settings as a JSON array and merge them with a per-galleria settings array.
But this way it's harder to provide a form to the user (with help texts etc) and it's impossible to cover the settings of each possible custom theme (not even with submodules).
Maybe use a form like mine for normal users, store the values as a JSON array and provide an "Extended configuration" tab with a textfield where advanced users can edit the JSON directly?

The only question remaining is how to select overriden settings in a per-galleria settings form.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Actually yes.
I mean still having the basic form and providing that json textfield directly, no additional tab needed, possibly as collabsed 'advanced' fieldset.

The per-galleria override would provide a checkbox then with default unchecked to use global settings, showing the same form as soon as a user chooses to 'override global settings'.
Global settings might be overridden partially (per fieldset?) while we even could put the presets into those settings.
Will you find time to hel us implement this feature?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Referring to
#1173594: settings per image formatter, nodereference formatter and views display

kroimon’s picture

kroimon commented
Title: Better options page and more options » Better options page and per-page options
Assigned: Unassigned » kroimon
Status: Needs review » Needs work

Ok, I thought about all that and will begin implementing it.

I will replace the current global options with a variable list of option sets. There will be an undeleteable default set and the user can add as many other sets as he likes. As discussed in #1173594: settings per image formatter, nodereference formatter and views display, the user can select one of these option sets per page/formatter/view.

Each set is initially empty and options may be added through the user interface.
This way, only explicitly specified options are passed to the Galleria instance while the rest of them keep the default or theme specific default values.
It should also be possible to add custom options to allow configuration of plugins and theme-specific options.

Once this is done and there is need for it, I might also look at some sort of inheritance.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Haha ... +1 for the set implementation ;-)

Outside the set settings, we might still need global options for galleria, such as the library detection / selection...

Don't put too much work into inheritance - sure i'd like that idea, but it adds much complexity to the UI and the concept.

I think we'll need human and machine name - the set should then be loaded through the machine name. We'll need to switch the storage: No more n variables for each style...

Thank you so much for pushing this!

kroimon’s picture

kroimon commented

Yeah sure, the option sets only cover the Javascript options. Additional settings such as themes, library selection or anything we might think of will get it's own page.

Inheritance was just an idea and will stay on a "do later... maybe" list ;-)

I'm currently only working with one single "machine name" (a 255 character string) like the image styles are using, but may add another "label" later... Sure I dropped the variable storage and implemented a proper database schema.

kroimon’s picture

kroimon commented

Just a short update on this:
The backend (very comfortable options page, database storage etc.) is complete and looks pretty nice.
I'm currently working on the use of these option sets (the Galleria loading itself, JS/CSS inclusion etc.).
This isn't too complicated, but I don't think I'll find the time to complete it this weekend.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Can't wait for it! :-) (When will it be ready? :-D )

Don't hesitate to submit something early, if feedback might help somehow.
How about a screenshot if it's already there?

kroimon’s picture

kroimon commented
Status: Needs work » Needs review

Sooo... I have a first working version @ https://github.com/kroimon/Drupal-Galleria
It started small and became something like a complete rewrite ;-)
I'll try to split the changes into several commits later to increase readability.

The galleria module has to be reinstalled for the database schema to be created.

I haven't looked into using the option sets for Views yet as I haven't ever used Views for Galleria, but will try to do so this week or next weekend depending on my spare time.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Let us test this, i'm curious about my first impression.

You can imagine, the first thing i would have checked was the views settings form... ;-)

kroimon’s picture

kroimon commented

Oh yeah and I just saw that I committed a dirty version yesterday, so you should pull the newest commit :-D

And now I'll try to understand the Views part and look what I can do there ;-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Assigned: kroimon » s_leu

Any status update?

We'll review tomorrow your code. So please push if there's something more up-to-date to see. :-)

Hope a clean option solution for all cases will hit us very soon. Thank you for pushing this.

kroimon’s picture

kroimon commented

I didn't have the time to check out the Views feature yet, but feel free to test the rest.
The github repository is up-to-date.

s_leu’s picture

s_leu commented

Just tested your version kroimon. The configuration UI and concept itself is really great, nice job! Unfortunately it doesn't work so far.

I am able to build a set of options within the new configuration page. But when I try to access a page with a galleria (on node base) nothing seems to happen. I get a javascript error "Fatal error: No theme found" and only the node title gets displayed. Any ideas what went wrong? We need to fix that issue before we can commit your changes into the drupal repository so please review.

damien_vancouver’s picture

damien_vancouver commented

subscribe!

kroimon’s picture

kroimon commented

For the record: s_leu's problem has been solved by a clean reinstall.

News: I just pushed Views support to my GitHub repository. It should work as before, just with the ability to use the new option sets. The commit is a bit longer than one would expect, but i refactored the code a lot, removing some code duplications and hopefully making things a bit faster and easier to maintain.

Ah and I successfully tested node_reference support (via references module)!

That means we should be feature complete.
Please test my changes and provide feedback!

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

kroimon:
I'm happy to see you proceeding. however the problem by s_leu is not solved if a reinstall is clean.
We absolutely must consider upgrade pathes to make the new features publicly ready.

Typically this incorporates not only a hook_install / hook_enable but also a hook_update_xxxx to allow the schema updates from a previous version apply cleanly.

You would be the most efficient one to write the hook_update_xx since you introduced the changes. Tell me if you don't find the time and we need to take that part.

Sounds like a great step forward... so testing needed...

Thinking about a feature complete gallery solution i still miss media support...
#1171444: Add media module / field support
Still motivated to push the galleria module? ;-)

Since Media module is a very popular inititive and something like a new paradigm for media management in D7, i consider this critical for a feature complete galleria module... I'm eager to push 7.x-1.x to stable and make a public release!! :-)

kroimon’s picture

kroimon commented

There you go.
Commit d6a7ac6bfd062c48b7a0 adds an update hook which creates the first (and current) version of the database schema and also migrates the old variable-based settings into the 'default' option set.

My 'feature complete' was related to this issue, not the complete module. I think you should release it as 1.0 as it is now (after testing this one thoroughly) and add media support later, maybe in 1.1.
I might look into that later if I can find time for it.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Coool.

Also agree that will be enough for a 1.0.
Important however is testing...

As soon as i see it is ready for public (reviewing,...) i'll publish an alpha release. So pls help.

kroimon’s picture

kroimon commented

I pushed some other commits to fix some small bugs I found today.
Please be sure to always test the latest version before reporting bugs :-)

Edit: I will merge the bugfix commits into fewer main commits/patches once all bugs have been found and fixed.

kroimon’s picture

kroimon commented

@miro_dietiker:
Because it was easy as hell, I just added support for the media module: commit #71e0aeb

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Hahaa gotcha. You finally did it :)
I'll review things ASAP.

Thanks a lot!

kroimon’s picture

kroimon commented

There are two things that work as before, but I'm still unhappy with:

1. The node_reference support always uses all images of the referenced field, not only the images of the field instance on the referenced node. I could change that, but it would be a backwards-incompatible change.

2. I'm not sure if Views support works as someone would expect. It works as before, but it doesn't display every image, just the first ones of every row:

  foreach ($variables['rows'] as $row) {
    $lang = $row->_field_data['nid']['entity']->language;
    $item = $row->_field_data['nid']['entity']->{$img_field_name}[$lang][0];
    $items[] = $item;
  }

Shouldn't that be a loop over every image in the row?

  foreach ($variables['rows'] as $row) {
    $lang = $row->_field_data['nid']['entity']->language;
    foreach ($row->_field_data['nid']['entity']->{$img_field_name}[$lang] as $item) {
      $items[] = $item;
    }
  }
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

kroimon
node reference support has been added recently. So it's not that popular in usage.
The intention was to support nodereference style galleries that refer to an image node. This is always limited to ONE field with ONE file in it.
Alternatively we might have a reference to a node containing ONE field with possibly multiple images. This is something like a super-gallery of a gallery node.
Finally a referred node might have multiple fields with images AND possibly multiple images in it. Note that typical gallery nodes have a separate title image that is possibly identical to a content image.

So while for the simple image-as-a-node approach one might be able to choose on nodetype level - which is the master imagefield...
We might even allow on a gallery nodetype to define if an image is of type gallery, or a master imagefield...
Thus all integration code might be correct in any case (considering those settings).
Alternatively one could place the configuration in the formatter settings. But again to choose a field in the formatter would be strange and this even needs to correspond with the node settings... While when referring to two image nodes of different type - with possibly different imagefields - it's even wrong.
So - The needed options should be in the node type settings - if structural options needed.

For super-gallery approach, one might define some settings in the formatter like:
- Display all available image fields
- Deep recurse gallery references (well - avoid recursion!)

Well there might be the clear case where a gallery node reference refers to further galleries - no matter how the gallery in it works. That might even incorporate deep recursion.. But hey that's a little crazy, no?

THEN the next thing..
Views integration is a bit messy here and even your suggestion i not correct...
A view might even list users and output an image field. There won't be anything like a 'nid' around then. That's why we need to refer to the primary id alias with a lookup in the view base definition. (There is a report that this is broken!)

Here also we would need an option - if we want to output the first image only per node - or all in a field (if multiple images allowed).
In general i would expect an image per line

However: note that it might need a field handler for image fields to allow in views to specify e.g. a DELTA - to output image N only (e.g. the first) and not always all images...

Having fun with image/gallery handling? ;-)

kroimon’s picture

kroimon commented

Well what I understood is that both node reference and views support can be used in many many different ways, and I have no idea what the best way is ;-)
So I think this is out of scope of this issue and should be addressed later :-D And I'm not yet sure I really want to touch that, because I don't think I will ever use it myself :-P

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Hahaa... so it's our part again ;-)
(I'll put it in separate issues then... after merging)

Thank you for the provided things .. sure.

More review inputs coming soon!

kroimon’s picture

kroimon commented

To make pulling and reviewing easier: I merged my initial bugfix commits from my 7.x-1.x-newoptions branch into fewer main patches and pushed them as branch 7.x-1.x. Contents are one and the same, just easier to read.

tommychris’s picture

tommychris
Primary language Hungarian
Location Budapest
commented

subscripe

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Assigned: s_leu » Unassigned
Status: Needs review » Fixed

OK, reviewed and merged cleanly. Cool stuff! Also upgrade path works.
Most output seems much more reliable now.

Regarding settings i see:
- If width / height could be part of the option set... Why are the image styles not part of it? Possibly one should define 3 default image presets that can be overridden in a set.

Feature request:
- I was a little confused, deleting an option was not handled by js (without reload) ;-)

Finally i've fixed a few additional things:
- Added function documentation @todo
- Changed some README description
- Removed node/nid limitation: Now it even works with users and other fieldable entities (like profile2)
- Removed views rows that doesn't have image field data. Removing notices..

kroimon’s picture

kroimon commented

Nice to see the merge :-)

I found another small copy&paste / code moving error (thanks to your last commit). You can safely pull my commit ce351ad056 as this code is never called.

The image styles are not part of the option sets, because they are not part of the options array that get passed to the Galleria JavaScript. I thought about adding them anyway, but then somehow decided to keep that separate.
If there's a good reason, we can change it, but should do so soon to reduce the users' confusion ;-)

I din't use AJAX for the form mainly because I never worked with Drupal's AJAX/AHAH API before. (OK, I never really worked with any Drupal APIs before that, but it was a nice experience and worth learning it).

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Fixed » Needs work

Ah yes i thought those line where crap. =)
But not 100% sure.

merged & pushed!

As long as the galleria size is related to the image presets, i feel they really should be chosen in the same UI. No matter what settings array they get used...
kroimon - still finding time to fix this?

kroimon’s picture

kroimon commented

kroimon - still finding time to fix this?

Yepp ;-)

kroimon’s picture

kroimon commented

In most cases the thumbnail size depends on the theme whereas the image size depends on the Galleria instance's overall size as defined in the option set.
So, when including the image styles and therefore the image sizes into the option sets, shall I also include the theme selection?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Yeeeees :-)
it seems i omitted that ;-)

If you allow a specific setting per instance, i would prefer to allow ANY value ...
(think of a checkbox "add custom settings" after setting the default option set. then you'll be able to set any option you like to override the option set for this specific item only)

That's why all settings should use the same mechanism.

So you possibly see the next step(s)? ;-)

kroimon’s picture

kroimon commented

So I'll move the image style and theme selection into the option sets.

I don't think I really get what you meant with the "add custom settings" thingy... Do you mean some kind of inheritance between option sets?
Or the possibility to override settings per page/instance? I definitely don't like the latter as it clearly is a step backwards. A centralized settings page is way cooler ;-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

i really mean overrise per page/instance (as you name it).

Centralized is great for defaults that hit many cases. As soon as you have specific settings this is possibly too centralized..
But hey, no decision and no request to implement it. It's just a feeling...

Looking forward to the integrated image style and theme selection!

kroimon’s picture

kroimon commented

Pull and test commit aefb8824e1.
Ah and don't forget to run update.php as the database schema changed again!

@miro_dietiker:
One reason why I don't like the idea is that the space for the field formatter settings form is very limited (only inside this table cell) and there is not much room to show full option tables etc.

damien_vancouver’s picture

damien_vancouver commented

I'm still getting the "Fatal error: No theme found." after updating to latest 7.x-3.x-dev and running the DB update to create the new schema.

I don't know why, since my new option set looks like my old option set. I also tried the following procedure to totally uninstall and reinstall the module (clean install is not an option for me at this point). I thought that would fix it, but it's still giving me that error.

1. disabled galleria module
2. removed the galleria_optionset table from my database
3. Deleted the galleria row from the "system" table
(the module is now uninstalled completely - I guess it should have an uninstall hook to do this for you).
4. Enabled the module again
5. Visited a new, empty default configuration
6. re-added my old options from my old galleria production site one by one on the config page
(at this point it was still giving me Fatal Error: Theme not found.)
7. Edited the display settings for my content type's galleria fields and just hit Update again

.. still getting the error though. I was following this issue and it came up right away in the issue search for the module to here, so I suspect this is the right place to comment.

I'm using the miniml theme I purchased and downloaded from the Galleria site. I also tried changing the theme display to classic theme, and that did not help. My advanced settings detect the galleria library and both themes.

Any suggestions anyone, or suggestions on where to look next for the problem?

kroimon’s picture

kroimon commented

@damien_vancouver: This might be the same issue as #1237454: Anonymous JS Aggregation fail / views formatter: "Fatal error: No theme found.". Please continue discussion there.

@miro_dietiker: Have you tried my last patches on integrating the theme and image styles into the option sets?

kroimon’s picture

kroimon commented
Status: Needs work » Fixed

Pushed. Marking as fixed for now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.