Similar to what Peter did for the global values in facetapi_facet_settings_form_submit(), the per realm settings should also make use of the #tree property to clean up the code that saves the values as settings.

CommentFileSizeAuthor
#2 facetapi-change-settings-form-structure-to-isolate-widget-settings.patch3.85 KBdas-peter

Comments

cpliakas’s picture

cpliakas commented
Issue tags: +code cleanup

Added code cleanup tag.

das-peter’s picture

das-peter commented
StatusFileSize
new facetapi-change-settings-form-structure-to-isolate-widget-settings.patch3.85 KB

I just had the problem that different widgets interfered each other by sharing the same setting names. Thus I've created attached patch.
It uses #tree to isolate the settings of each widget in an own container.
This makes it also easier to create the settings form in the widget class. No more special knowledge about the basic form structure or the display state stuff is needed.

Currently there's one disadvantage I know of: The setting values in the form need to be merged in two steps into the $facet_settings->settings.
But I think this can be changed once the whole form structure is refactored.

cpliakas’s picture

cpliakas commented
Status: Active » Needs review

Marking as needs review.

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

I definitely understand the desire for simplicity. The problem that I have with this patch is that settings are designed to be shared, i.e. the soft limit. For example, the way it works currently is that the facets share the soft limit settings, so it you change from the "links" to the "links with checkboxes" widget you don't have to change your soft limit as well. I think this will be really important for chart facets, where you will have a slew of settings that should be shared so you can change between pie graphs, bar charts, etc without having to change a bunch of additional settings as well.

das-peter’s picture

das-peter commented

Thanks for enlightening me - I thought about what the intentions for sharing the settings was but I couldn't figure it out.
However, currently the settings should be shared, at least the ones which have the same name. This happens since the settings with the same name are set for each widget container. The difference is that there's no collision on save.
This means if I've used the "links" widget and switch to the "links with checkboxes" widget the "softlimit" is shared initially . But before the patch it was possible to have two "softlimit" inputs which interfered with each other due the fact how these duplicates are handled in forms.

I try to illustrate that with some pseudo code - I don't feel like I've the right words to explain it :)
Before:

<input name="widget" value="links">
<input name="softlimit" value="$settings->softlimit"> Links
<input name="softlimit" value="$settings->softlimit"> Links with checkboxes

On save store "softlimit" (but which one?)

After:

<input name="widget" value="links">
<input name="links[softlimit]" value="$settings->softlimit"> Links
<input name="links_with_checkbox[softlimit]" value="$settings->softlimit"> Links with checkboxes

On save store "$widget[softlimit]"

A possible downside of this is of course that the settings of inactive widgets are lost if they aren't shared with the active widget.

cpliakas’s picture

cpliakas commented

I see what you are getting at. Well done for discovering this! So there is actually a reason why there are two form elements with the same name, and it is a core issue with the form states not allowing dependencies on multiple values.

Originally I wanted to have the "soft" limit show up when either the "links" or "links with checkboxes" option was selected, however the form states system only allows you to depend on a single value for some reason. Therefore I couldn't say "show this form element when one of these values are selected". See the issue posted at #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions for more details. Ultimately the Facet API stuff is a big hack to get around this until the fix is backported to Drupal 7.

The reason for most of the complexity is so that the base class (links) can define settings and be extended by a sub-widget (links with checkboxes) that pulls all of the same settings and stays in sync when one value changes. This will be the case in chart facets where a few widgets will have the same settings as the base widget. Form the end user's perspective, it appears as if the widgets are sharing various settings. The hack is that there are actually multiple form elements sharing the name as you mentioned above.

I definitely see where you are trying to go, and I think I am on board as long as the UX from the end user's perspective is maintained. Take a look at the facetapi.admin.inc file, as that is where the JS magic happens to keep all of the form elements in sync.

Thanks for the contribution and bearing with me while I wrap my head around this,
Chris

cpliakas’s picture

cpliakas commented
Status: Needs work » Closed (won't fix)

Closing this task, as it is much too difficult for a code cleanup. Using #tree makes it much more difficult to format the settings the way we want them, so closing as this is not a very important task.