Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pschuelke’s picture

I get the same error/issue. The mini pager tags->text for links do not change the display. Also if you use a '-' it gives a 'Warning: Invalid argument supplied for foreach() in views_object->unpack_translatable() (line 305...' You can get around it here.

walker2238’s picture

Title: Mini Pager ("tags") » Mini Pager ("tags") aren't being applied.
Jorrit’s picture

Status: Active » Needs review
FileSize
2.75 KB

Please see the attached patch for a fix.

dawehner’s picture

The only problem i have with this patch at the moment is that it will probably change the current output
of mini pagers.

The defaults defined on the full pager are "« first" etc. so they will be also rendered like that.
You probably have to change the default values in options_definition as well.

Jorrit’s picture

Thanks for your remarks, you're right. I have found another small issue: the views_theme() definition and theme_views_form_views_form() expect a quantity variable, but that variable isn't actually used nor supplied. So my patch also removes that. I am interested to hear your comments on my new patch.

dawehner’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_pager_mini.incundefined
@@ -18,9 +18,41 @@ class views_plugin_pager_mini extends views_plugin_pager_full {
+    // The 1, 3 index are correct. See theme_pager documentation.

We could add a // @see reference for that.

+++ b/theme/theme.incundefined
@@ -1040,19 +1040,13 @@ function theme_views_mini_pager($vars) {
-  $quantity = $vars['quantity'];
...
-  // Calculate various markers within this pager piece:
-  // Middle is used to "center" pages around the current page.
-  $pager_middle = ceil($quantity / 2);

Oh is this variable not used at all?

Jorrit’s picture

Yes. theme_views_pager_mini is a copy of cores theme_pager, but not everything that should have been removed, was removed.

dawehner’s picture

Thank you for this clean solution!

Oh i see, so i think this is RTBC after a small // @see

Jorrit’s picture

Status: Needs work » Needs review
FileSize
3.81 KB

Here you go. I guess versions 6.x-3.x and 8.x-3.x also needs an update, I'll look at that later.

dawehner’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_pager_mini.incundefined
@@ -18,9 +18,53 @@ class views_plugin_pager_mini extends views_plugin_pager_full {
+   * Provides the definition and default values of settings for this pager.
...
+   * Provides the form for setting options.
...
+   * Renders the mini pager.

What about using Overrides views_plugin_pager_full::$function instead, but keep the extra comments?

The rest of the patch is certainly RTBC. No worries about d8, we can port this after the patch got in, even the othe way round makes us a bit happier.

Jorrit’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

Is that a Drupal convention? I didn't know about that. Here is a new patch.

I can't get Drupal 8 to work at this moment, perhaps the core 8.x branch is broken temporarily. I'll check later.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs tests

Yeah, the handbook page http://drupal.org/node/1354 is pretty handy for that, i just use that one handbook page all the time :)

Tests maybe not be required for 7.x but at least for 8.x we really have to be sure that everything works (especially things which have been broken before), so adding tags. If you don't have the endurance to work on tests/d8 i will for sure do that.

Jorrit’s picture

I accidently saw your checkin at #1805674: Mini pagers should extend full pagers. I was already puzzled that the mini pager didn't extend the full pager in D8, thanks for fixing that. I still don't have Drupal 8 running, though.

Do you have any idea when the patch can be committed? Do other people need to take a look at it?

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.x-dev
Component: Code » views.module
Status: Reviewed & tested by the community » Needs work
Jorrit’s picture

Is there are reason why the patch can't be applied to 7.x before I or someone else ports it to 8.x?

xjm’s picture

Well, per the core backport policy, issues should be fixed first in D8 and then backported. Since Views in D7 is in contrib, though, I guess it's up to @dawehner to decide whether #11 can go into 7.x-3.x as-is. However, this issue should stay in the core queue for a complete D8 fix since it's apparently a blocker for an accessibility bug.

dawehner’s picture

I think it's okay to first get the patch in 7.x in.

dawehner’s picture

Committed that patch to 7.x-3.x and working on a 8.x patch.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests +VDC
FileSize
9.99 KB

Sadly the mini pager is horrible broken at the moment (it doesn't work for multiple reasons).

The menu_rebuild is something which needs to be done, for whatever reason at the moment.

aspilicious’s picture

FileSize
8.52 KB

I can't see a pager? Why is this thing green? What am I doing wrong?

dawehner’s picture

I can't reproduce that, how does this happen?

aspilicious’s picture

I create a new view, set the number of items to 2 on the mini pager. Thats it.

dawehner’s picture

I have no idea how/why this happens, i can't reproduce that at all.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I failed to apply the patch correctly. After a 2nd try I managed to get it working. Party!
All looking good now.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/views.moduleundefined
@@ -1569,8 +1569,7 @@ function views_disable_view(ViewStorage $view) {
   $view->disable();
 }
 
-/**
- * Loads a view from configuration.
+/*** Loads a view from configuration.

This hunk is wrong :(

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.98 KB

oh, corrected that.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -31,10 +65,23 @@ public function summaryTitle() {
-      'parameters' => $input, 'element' => $this->options['id']));
+    // The 1, 3 index are correct.
+    // @see theme_pager().

Can't use @see like this should be "See"

+++ b/core/modules/views/views.moduleundefined
@@ -1542,8 +1542,7 @@ function views_disable_view(View $view) {
-/**
- * Loads a view from configuration.
+/** Loads a view from configuration.
  *
  * @param string $name

Still incorrect :p

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.57 KB

He, now it looks less ugly :)

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -31,10 +65,22 @@ public function summaryTitle() {
+      'element' => $this->options['id'],
+      'tags' => $tags,
+  ));
+    return $output;

Ow I hate to do this, the brackets needs spaces in front of them

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.57 KB

Yeah they are looking awkward.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Unless I missed something this is rdy to go!

catch’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/Mini.phpundefined
@@ -24,6 +24,40 @@
 class Mini extends Full {
 
+  /**
+   * Overrides \Drupal\views\Plugin\views\pager\Full::defineOptions().
+   *
+   * Overrides the full pager options form by deleting unused settings.
+   */
+  public function defineOptions() {
+    $options = parent::defineOptions();
+
+    unset($options['quantity']);
+    unset($options['tags']['first']);
+    unset($options['tags']['last']);
+    $options['tags']['contains']['previous']['default'] = '‹‹';
+    $options['tags']['contains']['next']['default'] = '››';
+
+    return $options;
+  }

It's a little bit odd that Mini extends Full only to delete stuff. How hard would it be to make Full extend Mini and add stuff?

dawehner’s picture

Assigned: Unassigned » dawehner

What about making a common SqlPagerBase base class, from which both implementations extend? Assign the issue to myself.

dawehner’s picture

FileSize
31.73 KB

So Full and Mini extends SqlBase now, an interdiff sadly doesn't make sense here,.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Update status.

dawehner’s picture

#34: drupal-1625248-34.patch queued for re-testing.

dawehner’s picture

FileSize
32.23 KB

Funny the changes for theme('item_list') didn't got applied to the views code.

Also the reroll was needed for things like fieldset => details.

Status: Needs review » Needs work

The last submitted patch, drupal-1625248-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
41.49 KB

No idea, but for some odd reason this patch doesn't work with patch -p1 but fine with git apply,
so here a new patch which can be applied as there is no rename in there.

Status: Needs review » Needs work

The last submitted patch, drupal-1625248-39.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.23 KB

Forgot to pull recently, sorry.

dawehner’s picture

Issue tags: -VDC

#41: drupal-1625248-41.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1625248-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#41: drupal-1625248-41.patch queued for re-testing.

dawehner’s picture

#41: drupal-1625248-41.patch queued for re-testing.

dawehner’s picture

#41: drupal-1625248-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1625248-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.75 KB

Okay rerolled against some issues like the one which provided better naming for all this different fields.

dawehner’s picture

Title: Mini Pager ("tags") aren't being applied. » Mini Pager ("tags") are broken
damiankloip’s picture

Status: Needs review » Needs work

Nice!!! This is some of the best clean up and refactoring I've seen in VDC! :)

SOrry, just the one pick. Aside from that, this looks great.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/SqlBase.phpundefined
@@ -247,7 +205,7 @@ public function validateOptionsForm(&$form, &$form_state) {
+            array('@items_per_page' => $items_per_page))

The indentation got messed up a bit here?

dawehner’s picture

Status: Needs work » Needs review

Here is the context of the code:

        form_set_error('pager_options][expose][items_per_page_options', t('Please insert the items per page (@items_per_page) from above.',
            array('@items_per_page' => $items_per_page))

I think this looks fine :)

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Ahh, yeah, sorry :(

tim.plunkett’s picture

Issue tags: -VDC

#48: drupal-1625248-48.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1625248-48.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.75 KB

Hopefully we will not need that long to fix bugs like that once core is stable.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc.

catch’s picture

Status: Reviewed & tested by the community » Fixed
 6 files changed, 328 insertions(+), 553 deletions(-)

:)

Committed/pushed to 8.x.

damiankloip’s picture

Yeah! awesome patch Daniel.

dgatom’s picture

Thanks a lot =]

dawehner’s picture

Wow thanks, this is really really good that we got that in!

Status: Fixed » Closed (fixed)

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

j0rd’s picture

So just upgraded to views 3.6 on D7 and I believe the patch which was applied to D7 has a regression.

I get these errors now.

Notice: Undefined index: quantity in CUSTOMTHEME_views_mini_pager() (line 469 of themes/custom/yt/yt.theme.inc).

It appears this code removed quantity from theme_views_mini_pager().

There's a couple places in the code I can see this getting done

+  /**
+   * Provides the definition and default values of settings for this pager.
+   *
+   * Overrides the full pager options form by deleting unused settings.
+   */
+  function option_definition() {
+    $options = parent::option_definition();
+
+    unset($options['quantity']);
+    unset($options['tags']['first']);
+    unset($options['tags']['last']);
+    $options['tags']['previous']['default'] = '‹‹';
+    $options['tags']['next']['default'] = '››';
+
+    return $options;
+  }
+
+  /**
+   * Provides the form for setting options.
+   *
+   * Overrides the full pager options form by deleting unused settings.
+   */
+  function options_form(&$form, &$form_state) {
+    parent::options_form($form, $form_state);
+    unset($form['quantity']);
+    unset($form['tags']['first']);
+    unset($form['tags']['last']);
+  }

Is there any reason to not pass quantity to the theme function? I understand you don't need it for the theme implementation you're using....but someone else might decide (like me) their mini pager implementation is not simply Forward and Backwards.