Support reversing tags #120

leo60228 · 2019-06-11T13:08:26Z

You can now use /tag:test/rev instead of /tag:test/ to reverse the order from the default for that collection.

I have signed the CLA
I didn't get a confirmation email

mrvdb · 2019-06-11T13:15:31Z

Nice idea!
Could this also be implemented similar to something like this:

/tag:test or /tag:+test : show collection having tag test
/tag:-test : show collection not having tag test

leo60228 · 2019-06-11T13:18:04Z

@mrvdb This is the first time I've ever touched Go; I'm not really sure how to implement something like that. What's your usecase for this?

mrvdb · 2019-06-11T13:32:12Z

My apologies, I see I've mis-assumed what this was about. I thought it was about filtering instead of ordering.

For the filtering I had something like this in mind for a while but I have no specific usecase other than the need to filter a collection. Using the + and - has a number of advantages in my view:

it's easy to extend (like +this+that-somethingelse)
not english language dependent

Having the sort order reversed is something else indeed. :-)

thebaer · 2019-07-03T15:40:36Z

Thanks for working on this, @leo60228! We have some other things on our plate at the moment, but I do think this would be a great addition to the platform.

My only feedback right now is that I'd prefer to use /chrono instead of /rev, since:

It more clearly explains which order posts will be in (chronological)
It's what Tumblr uses -- so if someone comes from that platform and tries it with WF, it'll just work

Thoughts?

(Also, I like the filtering idea, @mrvdb -- it's been discussed a bit on the forum. Would love to get your input over there.)

thebaer

Just need to address some of the naming and logic choices before merging.

thebaer · 2019-09-13T17:35:56Z

collections.go

@@ -810,9 +814,10 @@ func handleViewCollectionTag(app *app, w http.ResponseWriter, r *http.Request) e
 		return err
 	}

-	coll := newDisplayCollection(c, cr, page)
+	rev := strings.HasSuffix(r.URL.Path, "/rev") || strings.HasSuffix(r.URL.Path, "/rev")


We can remove the duplicate condition here. Also check for /chrono instead.

thebaer · 2019-09-13T17:36:27Z

collections.go

@@ -712,7 +716,7 @@ func handleViewCollection(app *app, w http.ResponseWriter, r *http.Request) erro

 	// Fetch extra data about the Collection
 	// TODO: refactor out this logic, shared in collection.go:fetchCollection()
-	coll := newDisplayCollection(c, cr, page)
+	coll := newDisplayCollection(c, cr, page, strings.HasSuffix(r.URL.Path, "/rev") || strings.HasSuffix(r.URL.Path, "/rev"))


We can remove the duplicate condition here. Also should check for /chrono instead.

thebaer · 2019-09-13T17:37:20Z

database.go

@@ -1127,10 +1127,10 @@ func (db *datastore) GetPosts(c *Collection, page int, includeFuture, forceRecen
 // given tag.
 // It will return future posts if `includeFuture` is true.
 // TODO: change includeFuture to isOwner, since that's how it's used
-func (db *datastore) GetPostsTagged(c *Collection, tag string, page int, includeFuture bool) (*[]PublicPost, error) {
+func (db *datastore) GetPostsTagged(c *Collection, tag string, page int, includeFuture bool, reversed bool) (*[]PublicPost, error) {


Let's use chrono or chronological as the parameter name, to avoid confusion.

thebaer · 2019-09-13T17:37:27Z

database.go

@@ -105,7 +105,7 @@ type writestore interface {

 	GetPostsCount(c *CollectionObj, includeFuture bool)
 	GetPosts(c *Collection, page int, includeFuture, forceRecentFirst, includePinned bool) (*[]PublicPost, error)
-	GetPostsTagged(c *Collection, tag string, page int, includeFuture bool) (*[]PublicPost, error)
+	GetPostsTagged(c *Collection, tag string, page int, includeFuture bool, reversed bool) (*[]PublicPost, error)


Let's use chrono or chronological as the parameter name, to avoid confusion.

thebaer · 2019-09-13T17:37:55Z

routes.go

@@ -170,6 +170,7 @@ func initRoutes(handler *Handler, r *mux.Router, cfg *config.Config, db *datasto
 func RouteCollections(handler *Handler, r *mux.Router) {
 	r.HandleFunc("/page/{page:[0-9]+}", handler.Web(handleViewCollection, UserLevelOptional))
 	r.HandleFunc("/tag:{tag}", handler.Web(handleViewCollectionTag, UserLevelOptional))
+	r.HandleFunc("/tag:{tag}/rev", handler.Web(handleViewCollectionTag, UserLevelOptional))


Let's go with /tag:{tag}/chrono for the route, instead of /tag:{tag}/rev.

thebaer · 2019-09-13T17:38:42Z

collections.go

@@ -93,6 +93,7 @@ type (
 	}
 	CollectionFormat struct {
 		Format string
+		Reversed bool


Let's name this Chronological, instead, to make this less confusing.

thebaer · 2019-09-13T17:39:36Z

collections.go

@@ -146,8 +150,8 @@ func (cf *CollectionFormat) Valid() bool {
 }

 // NewFormat creates a new CollectionFormat object from the Collection.
-func (c *Collection) NewFormat() *CollectionFormat {
-	cf := &CollectionFormat{Format: c.Format}
+func (c *Collection) NewFormat(reversed bool) *CollectionFormat {


Let's use chrono or maybe forceChrono as the parameter name, instead, to make this less confusing.

thebaer · 2019-09-13T17:40:33Z

collections.go

@@ -126,6 +127,9 @@ const (
 )

 func (cf *CollectionFormat) Ascending() bool {
+	if cf.Reversed {
+		return cf.Format != "novel"


It would be helpful to have some comments here explaining the logic a bit (or some explanation here on github). I'm not sure why we're checking for the novel format here instead of always returning true.

thebaer · 2019-09-13T17:43:15Z

collections.go

@@ -659,13 +663,13 @@ func checkUserForCollection(app *app, cr *collectionReq, r *http.Request, isPost
 	return u, nil
 }

-func newDisplayCollection(c *Collection, cr *collectionReq, page int) *DisplayCollection {
+func newDisplayCollection(c *Collection, cr *collectionReq, page int, reversed bool) *DisplayCollection {


Again let's use chrono or maybe forceChrono as the parameter name, instead, to make this less confusing.

leo60228 · 2019-09-13T18:19:37Z

The reason it's rev instead of chrono is because it uses the opposite of the blog's default ordering, not forcing chronological. For novel, it's in most recent order instead of the default of chronological.

Support reversing tags

60db211

thebaer requested changes Sep 13, 2019

View reviewed changes

thebaer added the help wanted label Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reversing tags #120

Support reversing tags #120

leo60228 commented Jun 11, 2019

mrvdb commented Jun 11, 2019 •

edited

leo60228 commented Jun 11, 2019

mrvdb commented Jun 11, 2019

thebaer commented Jul 3, 2019

thebaer left a comment

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

thebaer Sep 13, 2019

leo60228 commented Sep 13, 2019

Support reversing tags #120

Are you sure you want to change the base?

Support reversing tags #120

Conversation

leo60228 commented Jun 11, 2019

mrvdb commented Jun 11, 2019 • edited

leo60228 commented Jun 11, 2019

mrvdb commented Jun 11, 2019

thebaer commented Jul 3, 2019

thebaer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo60228 commented Sep 13, 2019

mrvdb commented Jun 11, 2019 •

edited