New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.Next vs .NextInSection goes in opposite directions #1061

Closed
bep opened this Issue Apr 15, 2015 · 38 comments

Comments

Projects
None yet
9 participants
@bep
Member

bep commented Apr 15, 2015

Which is confusing.

I guess NextInSection does it right.

@bep bep added the Bug label Apr 15, 2015

@billychappell

This comment has been minimized.

billychappell commented Apr 19, 2015

The reason they're different is due to .Next evaluating the same condition as .PrevInSection, while .NextInSection's matches .Prev in hugolib/site.go; it looks like they were just mixed up.

Take a look:
(relevant functions can be found starting at line 382 and 839 in hugolib/site.go)

if i > 0 {
    wp.Page.NextInSection = s.Sections[k][i-1].Page
}
if i > 0 {
    page.Prev = s.Pages[i-1]
}

and

if i < len(s.Sections[k])-1 {
    wp.Page.PrevInSection = s.Sections[k][i+1].Page
}
if i < len(s.Pages)-1 {
    page.Next = s.Pages[i+1]
}

I've created a fork and changed them to be consistent, then manually tested that they go in the same direction; you can compare it with the current release here.

That's assuming .NextInSection/.PrevInSection were "correct," as bep stated. If you want, I can write some tests to ensure that we don't go through this again and that .Next/.NextInSection are linking to the last-written articles while .Prev/.PrevInSection go backwards in time before sending a pull request.

PS. As a relatively inexperienced dev, this is my first time posting on a public Github repo; hopefully this helps! Let me know if I've done anything wrong or can do anything else to contribute.

@bep

This comment has been minimized.

Member

bep commented Apr 19, 2015

Good analytics, @billychappell

We should wait for @spf13 to get his saying before we do anything about this, but when the pages are sorted by date, it makes most sense that next is newer.

  • But this is a breaking change in a very used feature
  • But I guess we should do all the breaking changes when we can. If it's confusing for the current 2000 users it will be so for the future 20000.
@billychappell

This comment has been minimized.

billychappell commented Apr 19, 2015

@bep Thanks for the reply! I hadn't considered the level of impact it could have on users; I have one question while we wait for spf13:

With this type of breaking change, wouldn't it be better to add a new type, something like LegacyMode map[string]interface{}, to the Site struct so that we can allow users to enable or disable deprecated features in versions immediately before and after such changes?

I'm imagining something like this:

func (s *Site) setupPrevNext() {
    // New Site variables helps users transition
    // to breaking changes over multiple updates.
    if s.LegacyMode["PrevNext"] == true {
        // Legacy Mode for PrevNext automatically set
        // to true in v0.14, false in v0.15, deprecated in v0.16.
        for i, page := range s.Pages {
            if i < len(s.Pages)-1 {
                page.Next = s.Pages[i+1]
            }

            if i > 0 {
                page.Prev = s.Pages[i-1]
            }
        }
    } else {
        // When not in legacy mode, .Next and .Prev matches
        // .NextInSection and .PrevInSection.
        for i, page := range s.Pages {
            if i > 0 {
                page.Next = s.Pages[i-1]
            }

            if i < len(s.Pages) {
                page.Prev = s.Pages[i+1]
            }
        }
    }
}

It's something to consider, but to keep things moving along, I'll make this my last suggestion for this issue and yield to whichever fix @spf13 thinks is best.

@bep

This comment has been minimized.

Member

bep commented Apr 19, 2015

@billychappell i thinks that gets too complicated. If we had some way of detecting "wrong usage", we could log it (see helpers.Deprecated).

In this case I think the best is just to DO IT, but make it very clear in the release notes. It isn't like this software is used to drive nuclear power plants (I hope) ...

And we are still at pre 1.0, so some breaking changes will happen.

@bep

This comment has been minimized.

Member

bep commented Apr 29, 2015

@spf13 bump

1 similar comment
@bep

This comment has been minimized.

Member

bep commented May 6, 2015

@spf13 bump

@bep

This comment has been minimized.

Member

bep commented Jul 25, 2015

@spf13 I thinks we're just going bold and fix this issue by reverting the direction of Prev and Next and mark it with big, bold letters in the release notes?

@billychappell

This comment has been minimized.

billychappell commented Jul 26, 2015

@bep Almost forgot about this! One last thought: If you wanted to avoid breaking changes, you could do a quick check with os or even p.PublishDate:

if f.FileInfo().ModTime() < releaseDate {
    // ...current code
} 
// ...new code

But that would clutter up the codebase. Alternatively, it wouldn't take long to write a cli tool that automatically reversed for all their pages -- reversing the order of all a blog's post could have a big impact on traffic and confuse a lot of end-users if the update made it to production without appropriate changes, even though it never should.

Probably still overshooting here, but the potential impact is worth a second thought.

@spf13

This comment has been minimized.

Contributor

spf13 commented Jul 27, 2015

I'm fine with making a breaking change. Let's just make 100% sure we make the right change. Are we sure that NextInSection is the right order and not Next?

@bep

This comment has been minimized.

Member

bep commented Jul 27, 2015

  • I'm using Next / Prev on my blog
  • Default sorting
  • Next navigates to Older content, or backwards in the sorted list
  • Prev navigates to Newer content, or forward in the sorted list

PrevInSection works opposite of this. What is right? In my head the above is so counter-intuitive that I must concentrate hard to get it right in the template.

a class="btn btn-lg bs-btn-default{{if not .Next}} disabled{{end}}" href={{if .Next}}{{.Next.Permalink}}{{end}} Eldre
      a class="btn btn-lg bs-btn-default{{if not .Prev}} disabled{{end}}" href={{if .Prev}}{{.Prev.Permalink}}{{end}} Nyare
@billychappell

This comment has been minimized.

billychappell commented Jul 28, 2015

I think older/newer is a better dichotomy than prev/next for users navigating a blog in general; the direction of prev/next is inconsistent across web properties and greatly increases the chances of the UI doing something other than what the user expects, negatively impacting the overall UX.

However, I agree with bep that, in the context of prev/next, my natural assumption would be that "previous" would take me to previous(ly written), or older, content, while next would head toward the latest.

Obviously it's hard to say for sure, but I'd be willing to bet that bep's proposed direction is encountered more often in the wild and is the default assumption of more average blog readers than any other way.

@spf13

This comment has been minimized.

Contributor

spf13 commented Jul 28, 2015

So the initial behavior wasn't accidental and as @billychappell says, it's horribly inconsistent. I actually spent time investigating what others did before this initial naming and my research found that the way I did it was used a bit more, but regardless, it's awful and we should do better.

How about we do this:

  1. Rename Prev/Next to Newer/Older.
  2. Add a new Prev/Next which matches the order that prevInSection and nextInSection use.

Does that sound right? Then people have a really obvious label for which way it's going and we can adjust the existing labels to be consistent.

@bep

This comment has been minimized.

Member

bep commented Jul 28, 2015

How about we do this:
1. Rename Prev/Next to Newer/Older.
1. Add a new Prev/Next which matches the order that prevInSection and nextInSection use.

Does that sound right?

Not really.

The main topic of this issue is that the to navigators go in different directions. It's not very important "who's right".

I suggest we fix the one who have lived shortest: Prev/NextInSection (to create the least amount of havoc in user templates).

@bep bep added this to the v0.15 milestone Oct 9, 2015

@bep

This comment has been minimized.

Member

bep commented Oct 9, 2015

Bump.

@spf13

This comment has been minimized.

Contributor

spf13 commented Oct 10, 2015

Sorry. I thought we did this. I agree with you. "Fix" the more recent one.
On Fri, Oct 9, 2015 at 5:10 PM Bjørn Erik Pedersen notifications@github.com
wrote:

Bump.


Reply to this email directly or view it on GitHub
#1061 (comment).

@billychappell

This comment has been minimized.

billychappell commented Oct 17, 2015

Hey, is this what you guys were looking for? If it's on the right track, I can finish up tests for weighted pages with sections and any additional changes, then make a pull request.

@bep

This comment has been minimized.

Member

bep commented Oct 17, 2015

@billychappell yea, that looks good.

billychappell added a commit to billychappell/hugo that referenced this issue Oct 20, 2015

@billychappell

This comment has been minimized.

billychappell commented Oct 20, 2015

@bep Actually, after writing some tests and looking at it looks like @spf13 was right -- using the same test cases, the current .PrevInSection//NextInSection are already going in the same direction.

@rickb777

This comment has been minimized.

rickb777 commented Sep 9, 2016

I would favour a clean-sweep approach to fixing this problem. Taking the view that Hugo's primary mode of grouping content is the section means that automatic navigation within sections should be made easy. My proposal is thus:

.Section is one of the page variables - but it changes becomes a structure instead of a string

The fields within .Section would be

  • .Name -- the section name (string)
  • .Older -- previous in terms of pub date; present only when there is one
  • .Younger -- next in terms of pub date; present only when there is one
  • .HasOlder -- flag indicating whether there is an older page than the current one
  • .HasYounger -- flag indicating whether there is a younger page than the current one
  • .Oldest -- first in terms of pub date; present always
  • .Youngest -- last in terms of pub date; present always
  • .Lighter -- previous in terms of weight; present only when there is one
  • .Heavier -- next in terms of weight; present only when there is one
  • .Lightest -- first in terms of weight; present always
  • .Heaviest -- last in terms of weight; present always
  • .Size -- number of pages in the section (integer)

Apart from the flags and .Name/.Size, all the others are node structures (http://gohugo.io/templates/variables/#node-variables)

The benefits of this approach are that it is comprehensively able to provide intra-section navigation for both date- and weight-based orderings; and that it would be very simple to use.

As things stand, I see no equivalent case for inter-section navigation provided by Hugo. Every website will have its own distinct requirements on that aspect.

@bep

This comment has been minimized.

Member

bep commented Feb 28, 2017

This issue has been automatically marked as stale because it has not been commented on for at least four months.

The resources of the Hugo team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, and you feel that it is still valuable, please open a proposal at https://discuss.gohugo.io/.

This issue will automatically be closed in four months if no further activity occurs. Thank you for all your contributions.

@bep bep added the Stale label Feb 28, 2017

@bep bep modified the milestones: v0.45, v0.46 Jul 17, 2018

@bep bep modified the milestones: v0.46, v0.47, v0.48 Aug 3, 2018

@bep bep modified the milestones: v0.48, v0.49 Aug 22, 2018

@bep bep modified the milestones: v0.49, v0.50 Sep 13, 2018

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Sep 21, 2018

I'd like to tackle this issue. Before I do anything, are we in agreement for implementation?

I like @rdwatters idea of using new names to help with the transition however to remove the ambiguity of which goes forward in time and which goes backwards, what about using these names?

.NewerPage .OlderPage
.NewerPageInSection .OlderPageInSection

I figure this way no one should be confused as to which direction in time the link would go.

I'd also then update Docs to say that the old way is deprecated, to use the new ones. Maybe the old way is removed in X amount of Hugo releases? Maybe 10 releases?

@moorereason

This comment has been minimized.

Contributor

moorereason commented Sep 22, 2018

I don't think .NewerPage makes sense because the sort order takes more than date into consideration. Page weight takes precedence over the date. See the default sort logic in hugolib/pageSort.go:

// defaultPageSort is the default sort for pages in Hugo:
// Order by Weight, Date, LinkTitle and then full file path.
var defaultPageSort = func(p1, p2 *Page) bool {

My recommendations:

  1. Make a breaking change, 3 years in the making. We've waiting long enough to make this change that either change will likely effect the same number of people. Anyone who has used both will appreciate us fixing this obviously inconsistent behavior.

  2. Make the change to .Next .Prev.

  3. Prior to the deprecation release, add a site configuration option to enable the future, default behavior. Example: UseNewNextPrevOrder = true. This will allow everyone to prepare ahead of the breaking release.

  4. For the deprecation release, add a site configuration option to enable the original, deprecated behavior. Example: UseDeprecatedNextPrevOrder = true. We would simultaneously deprecate the UseNewNextPrevOrder option.

  5. Make a sticky forum post detailing the plan.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Sep 22, 2018

@moorereason Ah good point. I agree newer/older won't make as much sense as I thought then.

In that case, I still prefer rdwaters approach of using NextPage and PreviousPage over adding a site config variable.

Make a breaking change, 3 years in the making.

Yes but it's not like Hugo has put out there that this is going to change. As far as some people, specifically theme devs are considered, this would be a sudden issue.

Hugo's own contributing guide points out to "strive not to break existing sites.".

Since .New and .Previous are already ambiguous by themselves, .NextPage and .PreviousPage can both fix the issue, offer a timeframe for people to move from the deprecated variables, as well as add clarity.


Regardless of the decision, I hope we can come to one in the next week or so in order for this extremely old Issue to finally be closed. The prospect of closing an old issue like this makes me happy for the project. 😄

@rdwatters

This comment has been minimized.

Contributor

rdwatters commented Sep 23, 2018

In the event you go with my recommendation (admittedly, my recommendations are cheap since I don't do all the heavy lifting like @felicianotech, @moorereason, Bjørn Erik, etc), this could be a way to go about it:

  1. Create new .NextPage and .NextPageInSection
  2. Document them as new/preferred in the list with current Page Variables, and simply update .Next or .NextInSection as legacy/deprecated/etc. (We already use "See also" references in this <dl>, btw).
  3. Terminal message would help, but even with .time (if my memory serves..can't remember which variable/function was deprecated last year), the syntax technically changed.

A breaking change with.Next and .NextInSection could be a royal pain in the neck for themes, and I can see it coming up in the forums all over place since it's hard for people who have been using some form of CD for a long time to check the console or assume that a page variable's meaning has inverted itself.

@felicianotech, can I be greedy and ask whether you're considering the sorting options we currently use on where clauses with the default still being "Weight > Date > LinkTitle > FilePath"? Think this would be super wicked awesome 😄 E.g.—

  • .NextPageInSection.ByPublishDate
  • .NextPageInSection.ByWeight
  • .NextPage.ByDate
  • Etc.....
@moorereason

This comment has been minimized.

Contributor

moorereason commented Sep 23, 2018

For the record, I agree with @rdwatters: @felicianotech's approach is better.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Sep 24, 2018

A PR for this should be fairly straight forward so I'll get started on it.

@rdwatters I'm not a strong programmer so adding in the sorting stuff will be more difficult for me and will slightly bloat this feature. Have you seen many people wanting to be able to do that kind of sorting for Next and Prev?

@kaushalmodi

This comment has been minimized.

Member

kaushalmodi commented Sep 24, 2018

(wondering why @bep is silent on this)

I don't really have an opinion on how this gets fixed. But I will adopt whatever solution is chosen as long as the next/prev start working consistently and correctly. This fix is long overdue.

@rdwatters

This comment has been minimized.

Contributor

rdwatters commented Sep 24, 2018

@felicianotech I was definitely being greedy with the ask 😄

I think my thought process was similar to why old/new wouldn’t work. Don’t get me wrong: ordering by publish date is great, but it limits the functionality to blogs...

For example, if you’re writing a series, you might want to put the next or previous in that series according to weight or another front matter property.

If you’re putting together a tutorial in multiple parts, you might also want to have the previous and next defined by a specific property in front matter (or in conjunction with taxonomy, etc).

@bep

This comment has been minimized.

Member

bep commented Sep 24, 2018

(wondering why @bep is silent on this)

I created the issue, not sure how silent that is. I, for some reason I don't remember, pushed the "mute button" on this issue some time ago.

@rdwatters this issue is very specific. Not about what we should order by.

The problem with any fix to this is that it is a breaking change, and I'm not sure how to handle that, but

  • the semantics of Next/Prev in section is to navigate to the default sort, which is weight/date.
  • If I'm reading the latest post, it does not make sense to me that Next takes me ... back.
@bep

This comment has been minimized.

Member

bep commented Sep 24, 2018

(wondering why @bep is silent on this)

Now I remember. Fixing this is lot of work (the code fix is easy, I talk about the breakage), so a part of me wants to fix the problems with Pages.Prev and Pages.Next (the problems being a somewhat clumsy API and the quadratic complexity of the implementation). With that fixed, we could eventually remove the "old next/prevs".

@rdwatters

This comment has been minimized.

Contributor

rdwatters commented Sep 24, 2018

@rdwatters this issue is very specific. Not about what we should order by.

I should have been clearer 😄 I was talking about providing options so that the order would be up to the Hugo end user (similar to the way pages are ordered now) and not whether it should be done in a certain way auto-magically.

Besides, it was more of a nice-to-have. This does sound very complex. Maybe once the fundamental change is made (with the same weight => date => etc), I'll put in a feature request for ordering I mention above...

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Sep 24, 2018

I'm going to keep this as simple as possible and just attempt to fix the issue without adding anything right now.

I'm not sure about the complexity that bep mentions but I'm sure he'll chime in on my PR. 👍

felicianotech added a commit to felicianotech/hugo that referenced this issue Sep 25, 2018

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes gohugoio#1061

kaushalmodi added a commit to kaushalmodi/hugo that referenced this issue Sep 25, 2018

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes gohugoio#1061

kaushalmodi added a commit to kaushalmodi/hugo that referenced this issue Sep 25, 2018

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes gohugoio#1061

felicianotech added a commit to felicianotech/hugo that referenced this issue Sep 25, 2018

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes gohugoio#1061

@bep bep closed this in #5252 Sep 26, 2018

bep added a commit that referenced this issue Sep 26, 2018

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes #1061
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment