pager.inc - Convert theme_ functions to Twig

Related,

#1778990: Merge theme_pager_link*() theme functions into theme_link()
#1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically

Where do the pager theme files go if there's no folder in d8/core/modules/* ?

Do i just create one?

Thanks c4rl. Will do.

I'll post an initial patch for this issue tonight, just need to convert a handful of theme() calls to render arrays.

Markup matches up (other than whitespace and attribute order), let's see if any tests break. The pager tests passed locally.

I’ve removed the following declarations from drupal_common_theme() in theme.inc. These theme functions were removed in #1598886: Clean up pager theme functions and replaced with theme suggestions (i.e. pager_link__first), but the drupal_common_theme() deletions were lost in a reroll on that issue.

    'pager_first' => array(
      'variables' => array('text' => NULL, 'element' => 0, 'parameters' => array()),
    ),
    'pager_previous' => array(
      'variables' => array('text' => NULL, 'element' => 0, 'interval' => 1, 'parameters' => array()),
    ),
    'pager_next' => array(
      'variables' => array('text' => NULL, 'element' => 0, 'interval' => 1, 'parameters' => array()),
    ),
    'pager_last' => array(
      'variables' => array('text' => NULL, 'element' => 0, 'parameters' => array()),
    ),

This also updates documentation referring to theme_pager(), leaving one reference in aggregator-wrapper.tpl.php to avoid conflict with #1896060: aggregator.module - Convert PHPTemplate templates to Twig.

Hi Cottser,

I was hoping #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically could be fixed with this conversion. It would be a change on how the query parameters are resolved within theme_pager_link(). It was already raised within #1778990: Merge theme_pager_link*() theme functions into theme_link(). Could you please consider?

Hi @mondrake, thanks for the feedback (but not sure why you changed the issue status). During the Twig conversion phase we are focusing only on converting theme functions and templates to Twig. Cleanup, consolidating and refactoring will come later, and we could certainly use your help throughout the whole process!

I know your patch is small but I don't think it should be added to this issue. I'll leave a comment on #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically as well.

Thank you @Cottser for feedback, and sorry for breaking the workflow.

Add conversion summary table

Tagging.

I don't see anything else to change here, assigning to @steveoliver for review.

-  return l($text, current_path(), array('query' => $query, 'attributes' => $attributes));
+  // @todo l() cannot be used here, since it adds an 'active' class based on the
+  //   path only (which is always the current path for pager links). Apparently,
+  //   none of the pager links are active at any time - but it should still be
+  //   possible to use l() here.
+  // @see http://drupal.org/node/1410574
+  $attributes['href'] = url(current_path(), array('query' => $query));

We're just replacing one bug #1410574: Make l() respect the URL query string before adding the 'active' CSS class for another here aren't we? going from all pager links always having the active class to no pager links ever having the active class. Isn't the active class in pagers pretty much the only time the active class is actually useful? I'm possibly missing something about the way pagers work here...

+ *   - attributes.href: The internal path or external URL being linked to, such
+ *     as "node/34" or "http://example.com/foo".

Not quite, that's what href was before it was passed to url(). The return of url() is a relative or absolute URL but not an internal path.

@thedavidmeister - so it sounds like we should stick with l() for your first point, correct?

#18 - yeah. I was thinking keep it simple and don't change that line here at all as there's already a separate issue open for that bug.

Implementing #17 - back to l(). theme_pager_link is up for consolidation anyway.

Update remaining

Not to derail this further, but a question:

Can this use #theme => link once gut l() is in or #type => link, so that output is changeable via other preprocess coming later?

Hm, thinking about this again, pager_links should just use #theme or #type link directly, therefore usage of l() here is fine.

Thanks!

HTML output for both pager and mini pager is good. rtbc.

Profiling.

Scenario: One page with two views, each with pagers. One full, one mini.

=== 8.x..8.x compared (519d3e4861676..519d3efa03160):

ct  : 144,138|144,138|0|0.0%
wt  : 1,001,360|1,000,271|-1,089|-0.1%
cpu : 928,933|926,736|-2,197|-0.2%
mu  : 9,813,376|9,813,376|0|0.0%
pmu : 10,096,484|10,096,484|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d3e4861676&...

=== 8.x..1898474 compared (519d3e4861676..519d3fdb36ac9):

ct  : 144,138|144,786|648|0.4%
wt  : 1,001,360|1,002,693|1,333|0.1%
cpu : 928,933|930,480|1,547|0.2%
mu  : 9,813,376|9,863,896|50,520|0.5%
pmu : 10,096,484|10,144,888|48,404|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d3e4861676&...

+++ b/core/includes/pager.incundefined
@@ -200,51 +200,55 @@ function theme_pager($variables) {
+      '#text' => isset($tags[1]) ? $tags[1] : t('‹ previous'),
...
+      '#text' => isset($tags[3]) ? $tags[3] : t('next ›'),

We change it from ">>" to ">"

+++ b/core/includes/pager.incundefined
@@ -300,26 +306,30 @@ function theme_pager($variables) {
-    return '<h2 class="element-invisible">' . t('Pages') . '</h2>' . theme('item_list', array(
-      'items' => $items,
-      'attributes' => array('class' => array('pager')),
-    ));
+
+    $variables['items'] = array(
+      '#theme' => 'item_list',
+      '#items' => $items,
+      '#attributes' => array('class' => array('pager')),

We seem to have lost the title of the item_list ('Pages')

+++ b/core/includes/pager.incundefined
@@ -200,51 +200,55 @@ function theme_pager($variables) {
+      '#text' => isset($tags[1]) ? $tags[1] : t('‹ previous'),
...
+      '#text' => isset($tags[3]) ? $tags[3] : t('next ›'),

We change it from ">>" to ">"

#28, should have been marked CNW, not CNR, right?

#20: 1898474-20.patch queued for re-testing.

@dawehner #28 -

+++ b/core/includes/pager.incundefined
@@ -200,51 +200,55 @@ function theme_pager($variables) {
+      '#text' => isset($tags[1]) ? $tags[1] : t('‹ previous'),
...
+      '#text' => isset($tags[3]) ? $tags[3] : t('next ›'),

We change it from ">>" to ">"

previous and next were both a single < or > already. First and Last are the links which contain << or >>.

+++ b/core/includes/pager.incundefined
@@ -300,26 +306,30 @@ function theme_pager($variables) {
-    return '<h2 class="element-invisible">' . t('Pages') . '</h2>' . theme('item_list', array(
-      'items' => $items,
-      'attributes' => array('class' => array('pager')),
-    ));
+
+    $variables['items'] = array(
+      '#theme' => 'item_list',
+      '#items' => $items,
+      '#attributes' => array('class' => array('pager')),

We seem to have lost the title of the item_list ('Pages')

The "Pages" title is present in core/modules/system/templates/pager.html.twig:

{% if items %}
  <h2 class="element-invisible">{{ 'Pages'|t }}</h2>
  {{ items }}
{% endif %}

Review of the patch looks good. I'm moving on to profiling.

Profiling results

Scenario

* Set front page to show 1 item with full pager
* Create block showing 1 item with mini pager, place in sidebar
* Create 3 nodes

=== 8.x..8.x compared (519fd31a307c4..519fd34c48dd6):

ct  : 68,855|68,855|0|0.0%
wt  : 292,003|292,102|99|0.0%
cpu : 278,409|278,943|534|0.2%
mu  : 15,896,128|15,896,128|0|0.0%
pmu : 16,068,720|16,068,720|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd31a307c4&...

=== 8.x..1898474-20-pager compared (519fd31a307c4..519fd39121ab9):

ct  : 68,855|69,227|372|0.5%
wt  : 292,003|293,548|1,545|0.5%
cpu : 278,409|280,512|2,103|0.8%
mu  : 15,896,128|15,972,456|76,328|0.5%
pmu : 16,068,720|16,144,704|75,984|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd31a307c4&...

This looks good, feedback has been addressed since last RTBC and acceptable performance tradeoff to me!

#20: 1898474-20.patch queued for re-testing.

Simple re-roll of patch in #20, system.theme.css moved location.

+{#
+/**
+ * @file
+ * Default theme implementation to display a pager link.
+ *
+ * Available variables:
+ * - link: The pager link.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_pager_link()
+ *
+ * @ingroup themeable
+ */
+ @todo Revisit this template after http://drupal.org/node/1595614 is resolved.
+#}
+{{ link }}

Is there a reason we can't make this theme('link') with suggestions now? I'm not sure it needs to wait until #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays is resolved for all other link functions.

I suppose that's what #1778990: Merge theme_pager_link*() theme functions into theme_link() is meant for

@catch re comment in #40, I prepared a patch that would combine #39 with #1778990: Merge theme_pager_link*() theme functions into theme_link() and get rid of theme_pager_link altogether. However I am uncertain to post here since #20 was RTBC already. Please advise.

@mondrake could you post the patch as do-not-test with an interdiff against #39? If it's a small change relative to this one I think we could go ahead combining the issues.

Here we go.

Sorry, disregard #44.

I wouldn't have a problem combining #1778990: Merge theme_pager_link*() theme functions into theme_link() into this issue, particularly since they both touch much of the same code, and if left separate, will ultimately need more work to get a re-roll after one or the other gets in (which-ever goes first). On the other hand, I'm not really sure that the interdiff in #45 could be considered a "small change", but hey, the work to integrate them is already done. Thanks mondrake!.

Maybe we could get both catch's and perhaps even sun's feedback on this (sun originally worked on the other issue).

needs re-roll because of #1363112: Simplify names of "element-x" helper classes

re-roll of #45

this changes the pager twig template

-  <h2 class="element-invisible">{{ 'Pages'|t }}</h2>
+  <h2 class="visually-hidden">{{ 'Pages'|t }}</h2>

also added a theme suggestion to the final render array of template_preprocess_pager()

-      '#theme' => 'item_list',
+      '#theme' => 'item_list__pager',

to make it consistent with the output in theme_views_mini_pager()

related #517242: theme_pager() could be more efficient when there is nothing to page

Lets give this one more round of manual testing.

Besides this, this looks very very RTBC to me.

Sooo nice cleanup.

Thanks all!

all++

+++ b/core/includes/pager.incundefined
@@ -156,16 +158,29 @@ function pager_get_query_parameters() {
+  // Fill in default link labels.
+  $tags = &$variables['tags'];
+  $tags += array(
+    t('« first'),
+    t('‹ previous'),
+    '',
+    t('next ›'),
+    t('last »'),
+  );
+

@@ -197,54 +212,79 @@ function theme_pager($variables) {
-      'text' => (isset($tags[0]) ? $tags[0] : t('« first')),
...
+      '#text' => $tags[0],

+++ b/core/modules/views/views.theme.incundefined
@@ -1156,31 +1156,37 @@ function theme_views_mini_pager($vars) {
-      '#theme' => 'pager_link__previous',
+      '#theme' => 'link__pager__previous',
       '#text' => (isset($tags[1]) ? $tags[1] : t('‹‹')),
...
     $li_next = array(
-      '#theme' => 'pager_link__next',
+      '#theme' => 'link__pager__next',
       '#text' => (isset($tags[3]) ? $tags[3] : t('››'))

Would it make sense to change these lines that touch the $tags in views mini pager to match the logic in the standard page?

patch needed a reroll.

The user.admin.inc piece has been fixed in issue #2009690: Replace theme() with drupal_render() in user module, so no longer relevant here.

@azinoman your patch was including some leftover + removed the Twig template.

Re-roll patch soon.

Re-roll, taken away the user.admin.inc piece.

Attempt to fit in input in #51. Let's see if bot is happy.

#57: 1898474-57.patch queued for re-testing.

Re-testing after #1985470: Remove theme_link() went in, is that impacting this issue?

#57: 1898474-57.patch queued for re-testing.

+++ b/core/includes/pager.incundefined
@@ -197,54 +212,79 @@ function theme_pager($variables) {
+    $li_next = array(
+      '#theme' => 'link__pager__next',
+      '#text' => $tags[3],
+      '#path' => $current_path,
+      '#element' => $element,
+      '#options' => array(
+        'query' => pager_query_add_page($parameters, $element, $pager_page_array[$element] + 1),
+        'attributes' => array(
+          'title' => t('Go to next page'),
+          'rel' => 'next',
+        ),
+      ),

Yes, all these render arrays will no longer be rendered as theme('link') is no longer there...

So this would have to be changed to sth like

    $li_next = array(
      '#type' => 'link',
      '#title' => $tags[3],
      '#href' => $current_path,
      ....
      ),

But:

• we are going to miss the theme suggestions - anyone any idea how to deal with that?
• we would have been passing '#element' and '#interval' for alternative theme implamentation - is that applicable with #type?

we are going to miss the theme suggestions - anyone any idea how to deal with that?

The patch at #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched would help with this if it got reviewed ;) it allows you to safely build a composite #type -> #pre_render -> #markup and #theme => array('suggestion', 'another_suggestion') style arrays, with the #markup from the #pre_render being used as a fallback for when the theme suggestions are not *implemented* (rather than the current situation where #markup only renders if #theme is not *set*).

OK, another reroll to cover part of #62 i.e. switching from #theme to #type.

Put in @todos for the rest points in #62.

#65 is OK, but setting back to needs work because of the @todos. Also, doesn't look as a Novice task any more :(, so removing tag.

Looking for comments/proposals, I'm stuck now.

yeah, I had a look at this. The suggestions issue is deeper than I first thought. There are ways to make this work eventually, but I don't think there's any immediate "quick fix" :/

I have to admit that I don't know as much as I'd like about the way suggestions work in detail, I'm going to keep looking into this and see what I can come up with - even if it has to just be a workaround for D8 and leave the improvements to the API in D9, i dunno yet..

this is what we want:

$li_next = array(
  '#theme' => 'link__pager__next',
  '#text' => $tags[3],
  '#path' => $current_path,
  ...
);

have we tried any of these?
1) the same way we'd call theme:

$li_next = array(
  '#type' => 'link__pager__next',
  '#text' => $tags[3],
  '#path' => $current_path,
  ...
);

2) or maybe

$li_next = array(
  '#type' => array('link__pager__next', 'link__pager', 'link'),
  '#text' => $tags[3],
  '#path' => $current_path,
  ...
);

I'd like to see us find a way to get #1 working. I think its supposed to work now... perhaps a bug?

@jenlampton - I tried '#type' => 'link__pager__next' before wrapping up patch in #65 and can confirm it produces no output (with #title and #href though, not with #text and #path).

#2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works related maybe?

#69 - yeah, that issue is related. It relies on cleaning up a lot of #type/#theme namespace conflicts in core though. It could also do with a re-roll to handle '__' syntax suggestions.

We also need to make sure that the #pre_render for #type 'link' (or equivalent processing function) handles all the relevant sanitisation and other processing of $variables before the theme suggestion starts doing things assuming they've been processed as a "link". #1985974: Make l() optionally return structured output is related to that bit.

maybe related #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()?

Just putting it out there, one of the reasons that theme_link() was removed was that in like one or two years of people asking for a use-case for needing a template override across various issues, no good examples came up.

Do we actually need theme suggestions for pager links, or would adding the classes "next" and "pager" for theming and contextual information be enough (keeping in mind that the structure of links can be altered with hook_link_alter)?

I've never done this before with pager links so I don't really know what the goal of the pager suggestions is.

#72 - neither am I, was just following the patch upstream. IMHO, 3-nested theme suggestion for pager links seems overkill, and core will not implement any of these, falling back on the link type.

+1 for the proposal in #72. I do not think we need to pass scope in the link classes though, as that's already in the wrapper attributes. We may want to keep the $element, $interval context to be passed to hook_link_alter though.

I was just wondering if anything like below may make sense??

+++ b/core/includes/pager.incundefined
@@ -197,59 +212,93 @@ function theme_pager($variables) {
+    $li_first = array(
+      '#type' => 'link',
+      '#title' => $tags[0],
+      '#href' => $current_path,
+      '#options' => array(
+        'query' => pager_query_add_page($parameters, $element, 0),
+        'attributes' => array(
+          'title' => t('Go to first page'),
+          'rel' => 'first',
+        ),
+        'pager_context' => array(
+          'link_type' => 'first',    // or 'previous', 'next', 'item', etc.
+          'element' => $element,
+          'interval' => ....,
+        ),
+      ),

@73:

Let's say As a themer, I want to replace the "next" pager link with a big fat ugly IMG tag of an arrow (for argument sake).

to do that I have to check *every link in the site* inside my theme_link_alter() function, to test to see if $options['pager_context'] exists, and if it does, if $options['pager_context']['link_type'] is "next". Thats two additional conditionals for every link in the site.

I'm no perf whiz, but that sounds sub-optimal, compared to providing a theme suggestion once, having drupal check once to see if that theme suggestion was implemented somewhere, and calling that theme suggestion implemented in my theme once.

i've marked the sister issue #1778990: Merge theme_pager_link*() theme functions into theme_link() as closed wont fix for now, since we're discussing what's the best way forward on this issue.

I'm really sad to see theme link go... I guess I dont understand the underlying reason why its going away.

Its obvious that pagers are a subset of links, but without having theme_link to depend on for suggestions, I think this means that we almost *have* to retain something like theme_pager_item().

@74:

True. But in this specific case, as a themer, you will most probably use an alternative CSS weapon like e.g.

ul.pager li.pager-first a {
  visibility: hidden;
}

ul.pager li.pager-first {
  background: transparent url('my-big-fat-ugly-arrow.png') no-repeat center center;
}

Hrm. Touché.

=)

Another thing to keep in mind, is a progression towards dream markup.

Take for example comment #3 from mortendk: #1912608-3: Update pagination markup for new CSS standards and improved accessibility

I suppose, in an ideal world a Twig themer might just implement the pager.html.twig file in his theme, and do any changes there.

@jwilson3 -

to do that I have to check *every link in the site*

"have to" is too strong - I don't think your use-case warrants a theme override OR an alter. A CSS based image replacement is faster than both an alter and invoking the theme system and is a much better way to separate content and presentation, it's also closer to what is being pushed in the "dream markup" discussions for D8. Using an alter is measurably faster than a theme override, see #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig for some profiling.

Another option would be to use hook_element_info_alter() to set an extra #pre_render for #type 'link' which would allow you to avoid running your checks against every link running through l() and limit it to render arrays that are expecting to undergo extra processing, by design.

I'm pretty sure you still can implement theme suggestions for pagers if you want using the array syntax rather than 'foo__bar', but the issue is that the preprocessing of variables done for #type link will have to be done in the override, they won't be provided automatically, which leads to...

I'm really sad to see theme link go...

It was never really there, it had security holes in it in D7 #1187032: theme_link needs to be clearer about saying not to call it directly and didn't do other processing that l() did for "active" class and such. This was rectified only very recently in D8, by making it call l() directly but the performance hit of using theme() over l() or #type 'link' was considered excessive. Using #type 'link' and implementing your own sanitisation in a theme suggestion overriding it is still easier in D8 (just call l()) than it was in D7 using #theme 'link' (can't call l() because l() called theme('link') sometimes so you had to copy and paste the relevant logic from l() manually into the preprocess for your theme suggestion).

So, to be clear, the existing suggestions based on theme 'link' have always been a bit broken in that the base hook wasn't correctly preparing the variables it was passing to the templates and I doubt many people even realised this.

It also wan't compatible with #type 'link' at all, you couldn't do array('#type' => 'link', '#theme' => 'link') without PHP throwing errors, for example, so it wasn't compatible with element_info() implementations. While this problem is solvable, there's more to consider.

I'm no perf whiz, but that sounds sub-optimal, compared to providing a theme suggestion once, having drupal check once to see if that theme suggestion was implemented somewhere, and calling that theme suggestion implemented in my theme once.

Well... in D7, doing just about anything related to theme('link') overrides or preprocessing (which is always required if you want to correctly prepare your link variables as per l()) led to l() calling theme('link') for every single link on the site whether it was relevant to the current link or not, which led to a performance overhead quite significantly larger than implementing a single alter for every single link on the site. What you're describing is not at all how the theme system worked for links in D7 and is only how it worked for about a month or so in D8, and the D8 implementation was not "drillable" or Twig based any way and nobody seemed to be actively working on fixing that in a performant way AFIACS (there were two issues open that I know of, but they both seem stalled on API architectural issues or performance).

I guess I dont understand the underlying reason why its going away.

There's a general movement at the moment (which probably won't be complete before D8 launches, but is well on its way) to get all the theme functions into Twig templates. When you think about #theme 'link', imagine it not as a relatively "light" function that is invoked (albeit with the overhead of the theme system doing it's "magic" and preprocess phases) but as a full-blown Twig template containing a single tag - which in many people's opinion is total overkill.

For larger templates this Twig conversion is great and usually represents no more than 0.5% performance overhead per template, for small theme functions that generate a single tag, the Twig template represents a much larger % performance overhead (like 10-20%) for no extra flexibility in what can be achieved over an alter on the simple renderable element used to generate the single tag. The nature of the smaller templates is that they are also called many more times in a page than larger templates (like, hundreds of times for links) so that larger % hit is also a larger % of the total page render time. For example, converting #theme 'html_tag' to #type 'html_tag' shaved nearly 4% of the *total* page load time for the front page on a fresh Drupal 8 install #1825090: Remove theme_html_tag() from the theme system.

Using #type -> #pre_render/drupal_alter() -> #markup is essentially guaranteed to be faster than anything run through theme() and for small (single tag), very commonly rendered HTML elements we have to seriously consider whether the theme system is worth what we pay for it.

There's also an argument that the theme system is very "cluttered" with many unnecessary hooks that all do the same or very similar things #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays and so if not for the sake of performance, for the sake of simplicity and learnability we should be constantly re-assesing what we really want to build templates for and what could be consolidated/removed/done differently - #1833920: [META] Markup Utility Functions.

I suppose, in an ideal world a Twig themer might just implement the pager.html.twig file in his theme, and do any changes there.

Absolutely. I don't see why we need suggestions for pager links on the level of individual links if we have a perfectly good pager template wrapping them all (with associated preprocess function that could modify links in a targeted fashion).

Not sure discussion landed yet, nevertheless a re-roll attached with #73 implemented and keeping up with #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file

I reviewed this patch, and it seems just fine. After I downloaded the patch, I created four articles and changed the page limit to three. I think this patch works!

Remove sandbox link

The discussion about removing __ style theme suggestions with a base hook of 'link' should be a separate issue as it will effect more than just pagers.

Re-roll needed because of #2027031: [Change notice] Move views_theme_functions() to ViewExecutable method.

was RTBC before

related #2030293: View preview is broken in UI if a pager has to be displayed

The discussion about removing __ style theme suggestions with a base hook of 'link' should be a separate issue as it will effect more than just pagers.

I take this back, it doesn't require a new issue. There are no 'link__' suggestions currently in core, only pager_link__ and menu_link__

Theme suggestions for 'link' would be as useless/broken as theme_link() itself was in D7 (if not more), I don't see any reason/use-case to try and support them.

I believe that trying to introduce a link__ that didn't previously exist would be a mistake, and #type 'link' is the correct implementation here as per #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays.

The patches from @mondrake are good in this respect :)

Yes I think this is fine. There's no flexibility lost that can't be had via other mechanisms, and it's not needed in the majority of cases here anyway.

Committed/pushed to 8.x, thanks!

Will need a change notice.

There are two notices already

Removed theme functions

and

theme_pager functions now use theme suggestions (theme_pager_link__*) (this one is now totally obsoleted).

Shall we edit the existing ones or create new ones?

untagging manual test

Changing the existing ones sounds great.

See

https://drupal.org/node/1795832/revisions/view/2628244/2745697

and

https://drupal.org/node/1819788/revisions/view/2409756/2745691

Just noticed a small error in views.theme.inc

Follow-up issue #2030817: Wrong link_type passed in link's pager_context in views.theme.inc

cross post added tag back, removing.

Revised change notices look good to me, thanks very much @mondrake!