Skip to content

Add sort by date in history page #7157

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

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

anurag2787
Copy link
Contributor

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #5595

Description

In this PR i have added sort button in the history page where allowing user can organize their watch history either by Latest First or Oldest First where it will shows the watch history in ascending or descending order

Sort By Oldest First

Screenshot 2025-04-06 233452

Sort By Latest First

Screenshot 2025-04-06 233431

Desktop

  • OS: Windows 11
  • OS Version: 24H2 (OS Build 26100.3476)
  • FreeTube version: 0.23.2

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 6, 2025
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 6, 2025 18:09
@efb4f5ff-1298-471a-8973-3d47447115dc

Hi @anurag2787 nice to see a pr from you again. Did you already take a look at #6214 and the feedback we left on that

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • src/renderer/views/History/History.css: Language not supported
Comments suppressed due to low confidence (1)

static/locales/en-US.yaml:53

  • There is a trailing whitespace at the end of the 'Latest First' text which should be removed for consistency.
DateNewestHistory: Latest First 

:value="sortOrder"
:select-names="sortByNames"
:select-values="sortByValues"
:icon="getIconForSortPreference(setHistorySortOrder)"
Copy link
Preview

Copilot AI Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getIconForSortPreference function is receiving setHistorySortOrder as its argument; it likely should receive the current sort order (e.g., the value from sortOrder) to determine the correct icon.

Suggested change
:icon="getIconForSortPreference(setHistorySortOrder)"
:icon="getIconForSortPreference(sortOrder)"

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested how copilot would work on this PR. Please disregard if this is a non issue.

auto-merge was automatically disabled April 7, 2025 09:24

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2025 09:24
@anurag2787
Copy link
Contributor Author

In getIconForSortPreference function instead of passing setHistorySortOrder i have passed the value of it sortOrder so that it can determine the correct icon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icon doesnt change like on other pages.

VirtualBoxVM_4KNwAlFBhV.mp4

@@ -49,6 +49,8 @@ Global:
Live: Live
Community: Community
Sort By: Sort By
DateOldestHistory: Oldest First

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DateOldestHistory: Oldest First
DateOldestHistory: Earliest Watched First

@@ -49,6 +49,8 @@ Global:
Live: Live
Community: Community
Sort By: Sort By
DateOldestHistory: Oldest First
DateNewestHistory: Latest First

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DateNewestHistory: Latest First
DateNewestHistory: Latest Watched First

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 8, 2025
auto-merge was automatically disabled April 12, 2025 07:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 12, 2025 07:28
@anurag2787
Copy link
Contributor Author

Fixed Icon change like on other pages

12.04.2025_12.46.03_REC.mp4

Also Updated history sort labels

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Apr 13, 2025
@PikachuEXE PikachuEXE changed the title Added sort by date Add sort by date in history page Apr 14, 2025
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works but I wonder why the new translation keys are added to Global not History
image

auto-merge was automatically disabled April 15, 2025 13:58

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 15, 2025 13:58
@anurag2787
Copy link
Contributor Author

I have fixed it, the new translation keys now been added to the History instead of Global

PikachuEXE
PikachuEXE previously approved these changes Apr 16, 2025
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (blocking): When setting order to Earliest Watched First and then new history entries are added, they're shown at the start instead of the end, despite them being the latest entries watched. I believe this is because upsertToHistoryCache is not updated to handle non-default sort methods.


const state = {
historyCacheSorted: [],
useAscendingOrder: HISTORY_SORT_BY_VALUES.DateAddedNewest,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
useAscendingOrder: HISTORY_SORT_BY_VALUES.DateAddedNewest,
sortOrder: HISTORY_SORT_BY_VALUES.DateAddedNewest,

suggestion: Update variable name and concomitant uses to better reflect its type (sort type, not boolean).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sort History by Earliest
  2. Open new Window
  3. Go to History page
  4. See that its sorted by Latest

It should be also sorted by Earliest

@anurag2787
Copy link
Contributor Author

Hy, since i have worked on similar feature #7139 where i migrated the playlist sort preference from sessionStorage to Vuex store to persist it across sessions so I'd like to confirm for the history sort preference should i follow same approach and store it in Vuex or prefer to store in sessionStorage instead?

@absidue
Copy link
Member

absidue commented May 5, 2025

In vuex settings store, that way it will also get remembered across app launches. As a general rule unless there is an extremely good reason, you should not be putting anything in session storage in FreeTube.

As for the sorting itself, I feel like this pull request is a bit over engineered. If we break it down the goal is to implement an option to keep the order the the history is already in (newest to oldest) or have it in the reverse order (oldest to newest). That means that the .sort() call is actually overkill, as you can either use the original array as is or use .toReversed() to get a copy in the reverse order.

The entire sorting could be handled by changing the existing historyCacheSorted computed on the history page, without needing to modify anything in the history store:

historyCacheSorted: function () {
  const historySorted = store.getter.getHistoryCacheSorted

  if (this.historySortOrder === HISTORY_SORT_BY_VALUES.DateAddedOldest) {
    return historySorted.toReversed()
  } else {
    return historySorted
  }
}

auto-merge was automatically disabled May 8, 2025 20:30

Head branch was pushed to by a user without write access

@anurag2787 anurag2787 force-pushed the added-sort-in-history branch from 6f10c83 to d25f09b Compare May 8, 2025 20:30
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 8, 2025 20:30
auto-merge was automatically disabled May 8, 2025 20:39

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 8, 2025 20:39
@anurag2787
Copy link
Contributor Author

anurag2787 commented May 8, 2025

I’ve simplified the implementation as per the feedback and removed the over engineered sorting logic. Additionally i have stored the user history sort preference in the Vuex settings store ensuring it persists across app launches

@efb4f5ff-1298-471a-8973-3d47447115dc

Sorting doesnt change when user uses keywords to filter

VirtualBoxVM_bS3GY5q9CD.mp4

auto-merge was automatically disabled May 9, 2025 05:53

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 9, 2025 05:53
@anurag2787
Copy link
Contributor Author

I have fixed the issue. This was happening because of filterVideosWithQuery has a hardcoded .sort((a, b) => b.timeWatched - a.timeWatched) which overrode the selected sort order.

@efb4f5ff-1298-471a-8973-3d47447115dc
  1. Sort History by Earliest

    1. Open new Window

    2. Go to History page

    3. See that its sorted by Latest

It should be also sorted by Earliest

Its better but not quite the same UX as how other dropdowns behave

VirtualBoxVM_VZJVmscjhk.mp4

@anurag2787
Copy link
Contributor Author

I have checked carefully but i am still having trouble in spotting exactly what behavior is different between my dropdown and the expected one could you please elaborate a bit more

@efb4f5ff-1298-471a-8973-3d47447115dc

@anurag2787 so in the first few seconds of the video the following happens. When sort order changes it automatically also changes in the other window.

When i navigate towards the History page this doesnt happen

auto-merge was automatically disabled May 9, 2025 19:25

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 9, 2025 19:25
@anurag2787
Copy link
Contributor Author

Thanks for clarifying I see the issue now and have fixed it so that the sort order syncs correctly

Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@efb4f5ff-1298-471a-8973-3d47447115dc

Apologies for the wait. We will get back to reviewing this as soon as possible

@PikachuEXE
Copy link
Collaborator

Need all the test cases in testing section...

  • Icon should change
  • Earliest first should not show latest when new entry added
  • Sorting doesn't change when user uses keywords to filter (I don't even know what this means
  • Sort order change affects all other windows and new windows and future app instances

I definitely miss something I dunno what...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add sort by date to the history page
5 participants