ordering: implement in item list

[pbek]

• Resolves: Sorting by date does not work #2678

Summary

This pull requests now implements the ordering of feed item in the item list.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Changelog entry added for all important changes.

[SMillerDev]

I think this broke the frontend tests.

[pbek]

Hm, when I run npm run test in the master branch, I also get so many errors my terminal doesn't even keep them in the history. 😅 What test did fail?

[pbek]

I'm not able to find out what is even failing... ☹️

/home/runner/actions-runner/_work/news/server/apps/news/src/components/routes/Feed.vue:34
return this.$store.getters.feeds.find(feed => feed.id === this.id);
^

TypeError: Cannot read properties of undefined (reading 'find')

That line of code isn't even in Feed.vue:34 in the PR, it's in line 45 https://github.com/nextcloud/news/pull/2811/files#diff-84a57fb48913250199e88bd350bb75de52612350b7131c2d4833ffa3db89d6f2R45, and it's not part of the PR. 🤔

I can't make the tests show what actually failed. 😬

[pbek]

@powerpaul17, @devlinjunker, do you maybe have some insight what went wrong?

[pbek]

It seems to be an issue with the feed() function in
https://github.com/pbek/news/blob/28d2753b0056e50bc3849cce427b8d61794a2850/src/components/routes/Feed.vue#L45.
I wonder why that wasn't an issue before...

[pbek]

But meanwhile there are merge conflicts too. @wofferl, are you ok with my conflicting changes of those two files?

• src/components/feed-display/FeedItemDisplayList.vue
• src/dataservices/item.service.ts

[wofferl]

But meanwhile there are merge conflicts too. @wofferl, are you ok with my conflicting changes of those two files?

* src/components/feed-display/FeedItemDisplayList.vue

* src/dataservices/item.service.ts

You should modify these files and add a fallback to the global setting store.state.oldestFirst when feed.ordering === FEED_ORDER.DEFAULT

[pbek]

@wofferl, thank you for your answer. Could I please get a little more context?
Are we talking about this part in the store?

news/src/store/app.ts

Lines 36 to 38 in 22cdb3c

	oldestFirst() {
	return state.oldestFirst
	},

You should modify these files

You mean "take my version"?

and add a fallback to the global setting store.state.oldestFirst when feed.ordering === FEED_ORDER.DEFAULT

Could you please elaborate on what we want to achieve?

[wofferl]

@wofferl, thank you for your answer. Could I please get a little more context? Are we talking about this part in the store?

news/src/store/app.ts

Lines 36 to 38 in 22cdb3c

	oldestFirst() {
	return state.oldestFirst
	},

You should modify these files

You mean "take my version"?

and add a fallback to the global setting store.state.oldestFirst when feed.ordering === FEED_ORDER.DEFAULT

Could you please elaborate on what we want to achieve?

I mean this parts which have changed to this in master to use the global setting for ordering and which your commit changes to use the feed setting. The goal here is to first use the feed setting and fallback to the global oldestFirst when FEED_ORDER.DEFAULT is set for this feed.

news/src/components/feed-display/FeedItemDisplayList.vue

Lines 131 to 136 in 6603803

	// Always want to sort by id
	sort: (a: FeedItem, b: FeedItem) => {
	if (this.$store.getters.oldestFirst) {
	return a.id < b.id ? -1 : 1
	} else {
	return a.id > b.id ? -1 : 1

news/src/dataservices/item.service.ts

Lines 89 to 101 in 6603803

	static async fetchFeedItems(feedId: number, start?: number): Promise<AxiosResponse> {
	return await axios.get(API_ROUTES.ITEMS, {
	params: {
	limit: 40,
	oldestFirst: store.state.oldestFirst,
	search: '',
	showAll: store.state.showAll,
	type: ITEM_TYPES.FEED,
	offset: start,
	id: feedId,
	},
	})
	}

And I think, this commit leads to another problem, that needs to be solved. The way the items actual loaded it will not load all articles with mixed ordering views. Eventually we need different lastItemLoaded states for each ordering.

[pbek]

I mean this parts which have changed to this in master to use the global setting for ordering and which your commit changes to use the feed setting. The goal here is to first use the feed setting and fallback to the global oldestFirst when FEED_ORDER.DEFAULT is set for this feed.

@wofferl, thank you very much, should be implemented now.

And I think, this commit leads to another problem, that needs to be solved. The way the items actual loaded it will not load all articles with mixed ordering views. Eventually we need different lastItemLoaded states for each ordering.

I didn't even know we can keep different orderings in mind.
Hm, I guess we should create an issue for that?

[wofferl]

I mean this parts which have changed to this in master to use the global setting for ordering and which your commit changes to use the feed setting. The goal here is to first use the feed setting and fallback to the global oldestFirst when FEED_ORDER.DEFAULT is set for this feed.

@wofferl, thank you very much, should be implemented now.

And I think, this commit leads to another problem, that needs to be solved. The way the items actual loaded it will not load all articles with mixed ordering views. Eventually we need different lastItemLoaded states for each ordering.

I didn't even know we can keep different orderings in mind. Hm, I guess we should create an issue for that?

Yes, and I don't know if it is really a good idea to have such a feature, it don't make things easier.
In news 24 it doesn't seems to work really either, at least on my installation.
Is anyone really want different orderings for different feeds? @Grotax @SMillerDev what do you think?

[pbek]

Yes, and I don't know if it is really a good idea to have such a feature, it don't make things easier.

Even the thought of it scares me. 😁
I'm very content if I can only order individual feeds by oldest / newest.

[wofferl]

Yes, and I don't know if it is really a good idea to have such a feature, it don't make things easier.

Even the thought of it scares me. 😁 I'm very content if I can only order individual feeds by oldest / newest.

Have you tested what happens when you are viewing mixed ordering feeds in Folder view or in All Article/Unread view?
Did that work especially when you switch between the views?

[pbek]

Have you tested what happens when you are viewing mixed ordering feeds in Folder view or in All Article/Unread view?
Did that work especially when you switch between the views?

I currently don't even see the "All Articles" menu entry in the master branch. 🤔

The ordering while loading items in infinite scroll in "Unread" is a mess (older and newer items loaded by infinite scroll).

In the pull request branch, the ordering of items in "Unread" is consistent (newest first). So, I guess, that's a big improvement.

[wofferl]

Have you tested what happens when you are viewing mixed ordering feeds in Folder view or in All Article/Unread view?
Did that work especially when you switch between the views?

I currently don't even see the "All Articles" menu entry in the master branch. 🤔

It only shows if you activate "show All articles"

The ordering while loading items in infinite scroll in "Unread" is a mess (older and newer items loaded by infinite scroll).

In the pull request branch, the ordering of items in "Unread" is consistent (newest first). So, I guess, that's a big improvement.

You mean the date showing is a mess or in sense of the items where reordered during scroll.

You can search this repository for ordering/sorting and you will find multiple topics over the last ten years why "real" sorting by date doesn't work.

Here a simple example:

You have two feeds, the fetcher download new items grouped by feeds.

For example since you last have looked at your news, the fetcher has downloaded 40 items for each feed.
In the database the are grouped, feed 1 has now 40 items with id 1-40, feed 2 with id 41-80. If you refresh the frontend you will get 40 items from the database dependent on ordering.

When ordering oldest to newest you get 40 items ordered by item id from the backend, in this case item 1-40, you will only see the items from feed 1. At the end of this bunch the frontend requests another 40 (41-80), so you now get the items from feed2. Because the second bunch can have dates that overlap with the first bunch, sorting all 80 articles in the frontend, will now reorder the item array and you might need to scroll up to see unread articles of the second bunch. This also can confuse the logic to load new articles or which item is selected.

So having different sortings (item id in backend and date in frontend) can not work.

[wofferl]

Here an example what I mean. This is how my feed looks like after reading the first 40 articles, navigating with shortkey, with your actual version.

The new articles where inserted everywhere, which is chronologically correct, but disturbs the flow of reading. There are also now unread items where I need to scroll back and the selected item is no longer selected.

[pbek]

You mean the date showing is a mess or in sense of the items where reordered during scroll.

Yes, the feed wasn't ordered anymore after scrolling.

I don't know how to deal with the issue properly. 🤷🏻

[wofferl]

You mean the date showing is a mess or in sense of the items where reordered during scroll.

Yes, the feed wasn't ordered anymore after scrolling.

I don't know how to deal with the issue properly. 🤷🏻

It depends on what you want to achieve. A “real” sorting by publish date will not work without major changes in the API and the backend. It actual will only work by item id, which corresponds to the date when an article is added to the database.

So I would change a.pubDate back to a.id as it is currently in the master and then test how it will behave if you have several different sorted feeds with items > 100. Especially when switching between different views.

[pbek]

So I would change a.pubDate back to a.id as it is currently in the master and then test how it will behave if you have several different sorted feeds with items > 100. Especially when switching between different views.

Fine with me, done.

[wofferl]

Since this.config.ordering is only valid for feeds I would suggest something like this to prevent ordering switching in folder or unread view:

                               let oldestFirst
                               if (this.fetchKey.startsWith('feed-')) {
                                        switch (this.config.ordering) {
                                        case FEED_ORDER.OLDEST:
                                                oldestFirst = true
                                                break
                                        case FEED_ORDER.NEWEST:
                                                oldestFirst = false
                                                break
                                        case FEED_ORDER.DEFAULT:
                                        default:
                                                oldestFirst = this.$store.getters.oldestFirst
                                        }
                                } else {
                                        oldestFirst = this.$store.getters.oldestFirst
                                        
                                }

But there is still some confusion with the ordering in mixed views.

For example the default ordering is set to oldest->newest.
Now open initially a feed with ordering newest->oldest.
If you now switch to unread and scroll the elements you get the oldest elements, but after the first 40 items you will see the 40 items you have first seen in the feed.
If you scroll further it loads new items, which will be sorted above the first items and the view remains on the initial items.
and so on... this repeats endlessly

[wofferl]

But there is still some confusion with the ordering in mixed views.

For example the default ordering is set to oldest->newest. Now open initially a feed with ordering newest->oldest. If you now switch to unread and scroll the elements you get the oldest elements, but after the first 40 items you will see the 40 items you have first seen in the feed. If you scroll further it loads new items, which will be sorted above the first items and the view remains on the initial items. and so on... this repeats endlessly

Ok, this seems a general problem which exist in master too. I have created an issue for that.

[pbek]

Thank you for testing! So I think this PR is "ready for merging" then? 😊