The use_pager setting is getting lost.

When looking into it, see that panopoly is moving a bunch of settings under the #tree fieldset 'exposed' and then trying to look for that value in various places. Since exposed is a #tree, values under it are are given the name exposed[value]. However, using the #parents property, can override that and set it back so name will appear as specified, e.g. #parents = array('use_pager') will use the name 'use_pager' and #parents = array('exposed', 'sort_by') use the name exposed[sort_by].

So patch does that, cleanup the submission function and anywhere looking for [exposed] unnecessarily.

An alternate solution would be to make a new fieldset and put all items under it, but this presently looked like less a change/easier to test.

CommentFileSizeAuthor
#18 panopoly_magic-parents-2016527-18.patch11.25 KBdsnopek
#5 2016643_panopoly_magic_screw_pre_render_with_2016527-5.patch21.05 KBmpotter
#3 2016643_panopoly_magic_screw_pre_render-3.patch5.5 KBhefox
#3 2016643_panopoly_magic_screw_pre_render_with_2016527-3-do-not-test.patch5.13 KBhefox
#2 2016527-panopoly_magic_parents_exposed-2.patch12.19 KBhefox
#1 2016527-panopoly_magic_parents_exposed-1.patch12.18 KBhefox
panpopoly_magic_exposed_parents_eww.patch12.18 KBhefox
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hefox’s picture

hefox CreditAttribution: hefox commented
FileSize
2016527-panopoly_magic_parents_exposed-1.patch12.18 KB

Had a #parent instead of #parents for one

hefox’s picture

hefox CreditAttribution: hefox commented
FileSize
2016527-panopoly_magic_parents_exposed-2.patch12.19 KB

one more bug meaning widget_title not saving

hefox’s picture

hefox CreditAttribution: hefox commented
FileSize
2016643_panopoly_magic_screw_pre_render_with_2016527-3-do-not-test.patch5.13 KB
2016643_panopoly_magic_screw_pre_render-3.patch5.5 KB

header type wasn't working

mpotter’s picture

mpotter CreditAttribution: mpotter commented
Status: Needs review » Reviewed & tested by the community

Using this in Open Atrium 2.

mpotter’s picture

mpotter CreditAttribution: mpotter commented
FileSize
2016643_panopoly_magic_screw_pre_render_with_2016527-5.patch21.05 KB

Needed to reroll this for Panopoly rc5. Here is the combined 2016643 and 2016527 patch again.

mpotter’s picture

mpotter CreditAttribution: mpotter commented
Status: Reviewed & tested by the community » Needs review

Since this is a complex patch, moving it back to Needs Review for RC5 testing.

mpotter’s picture

mpotter CreditAttribution: mpotter commented

Hrm, messy. Now this conflicts with #2017159: Panopoly Magic: render preview outside of pane configuration form.

So we've basically got 3 patches that conflict. Need somebody to sort this out and either get this back into 3 different patches, or post a rollup patch of all 3 issues.

hefox’s picture

hefox CreditAttribution: hefox commented 
patch < 2016643_panopoly_magic_screw_pre_render_with_2016527-5.patch 
patching file panopoly_magic.module
patch < 2017159_panopoly_magic_preview_post_render-16.patch 
patching file panopoly-magic.js
patching file panopoly_magic.module
Hunk #4 succeeded at 368 with fuzz 2.

panopoly_magic patched with 2016643_panopoly_magic_screw_pre_render_with_2016527-5.patch.                                                             panopoly_magic patched with 2017159_panopoly_magic_preview_post_render-16.patch.

I'm not able to reproduce the issue with applying patches, at least in this order (switching order does get a conflict I think)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Version: 7.x-1.x-dev » 7.x-1.0-rc5
Issue summary: View changes
Issue tags: +sprint

Adding the "sprint" tag - we're going to focus on this for the next release!

Jim Kirkpatrick’s picture

Jim Kirkpatrick CreditAttribution: Jim Kirkpatrick commented

This appears to be covered by the patch pending over on #2097199: Views pager settings do not stick. - but on that patch I just fixed the broken form array locations so it's very simple compared to the approach here.

Not sure why the fix/cause is as complex as this issue implies... Can someone explain? And ideally mark the linked issue as a dupe if we think it is (and the patch there has no merit).

hefox’s picture

hefox CreditAttribution: hefox commented

This patch is longer, but makes the code more readable/manageable

Jim Kirkpatrick’s picture

Jim Kirkpatrick CreditAttribution: Jim Kirkpatrick commented

Then why not commit the quick fix patch now, then continue working on this clean-up separately? It's broken now, after all...

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

@Jim Kirkpatrick: That's a possibility!

The advantage of this patch is that it's been used in Open Atrium for months so it's gotten a lot of testing. That said, no one outside of the Open Atrium team has gotten a chance to give it a real detailed review yet. However, I intend to try and fit it in before the 1.0 release!

In any case, I'll look at the simpler one too when I get around to it. :-)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Something wonky is going on with the patches on this issue, which I'm attempting to unravel.

First of all, only @hefox's patch in #1 and #2 actually solve the issue described here using the approach mentioned in the issue description.

@hefox's two patches on #3 actually look like it's some version of the patch from #2016643: Panopoly magic: Undefined index row_classes, group: use hook_view_pre_view instead of hook_views_pre_render and doesn't solve the pager issue or even resemble approach described.

Then @mpotter's patch on #5, looks like the patch from #3 plus a whole bunch of unrelated code style fixes (removing whitespace at the end of line). This one also doesn't fix the pager issue.

And it's actually the patch from #5 that's included in OpenAtrium!

So, as an attempt at review: I really like the patch at #2 and the approach that @hefox chose - it's way simpler than trying to move all those form elements around and then pull the values from the multiple places they could be at. I'm going to try and give this some more review and see if I can pull all the Panopoly Magic patches into a coherent whole. :-)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

@hefox: Do you have a newer version of the patch from #2 laying around somewhere?

Since that's the newest one that actually fixes the issue, that's what I'm going to work off of. However, it does have some problems! The following fields will get saved (ie. you can edit the setting again and see the right value), but they won't actually get reflected in the view when it's rendered:

  • widget_title
  • view_settings (ie. fields, content, table)
  • header_type

If you can fix those issues, I'll definitely commit this because your approach is way cleaner than what's currently in Panopoly Magic. However, in the meantime, I'm going to commit the patch at #2097199: Views pager settings do not stick..

Thanks!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Title: Panopoly Magic: Sometimes use_pager is lost; use #parents instead of checking under exposed » Panopoly Magic: use #parents instead of checking under exposed

Updated title now that the pager issue has been fixed in another way.

hefox’s picture

hefox CreditAttribution: hefox commented

Not ATM and not currently working on a project that uses panopoly, but might as well leave this open if me/someone else gets a chance

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Component: Code » Magic
Category: Bug report » Task
Status: Needs work » Needs review
FileSize
panopoly_magic-parents-2016527-18.patch11.25 KB

Ok, thanks! Here is a re-roll of @hefox's patch from #2, after the other patches committed to Panopoly Magic. Strangely, in testing it now after committing #2016643: Panopoly magic: Undefined index row_classes, group: use hook_view_pre_view instead of hook_views_pre_render, it appears to not have the problems I noted in #15. :-) So, either I did a bad job of testing last night or this patch was dependent on #2016643: Panopoly magic: Undefined index row_classes, group: use hook_view_pre_view instead of hook_views_pre_render.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Fixed

Gave the re-roll of this patch another review! It caused some the visual improvements in panopoly-magic.css to stop working, so I got them working again. Otherwise, everything works and this actually fixes: #2154103: Live preview won't show the overridden title for a view

So, committed in 682cc47.

Status: Fixed » Closed (fixed)

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