Skip to content
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

Sort by date on the history page #6214

Open
wants to merge 5 commits into
base: development
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/renderer/helpers/history.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const SORT_BY_VALUES = {
DateAddedNewest: 'newestFirst',
DateAddedOldest: 'oldestFirst',
}
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

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

Please move this constant into the src/constants.js file and add HISTORY to its name (not worth creating a new file for a single constant, especially when we already have a file for constants that are used in multiple places).

20 changes: 20 additions & 0 deletions src/renderer/store/modules/history.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { set as vueSet, del as vueDel } from 'vue'
import { DBHistoryHandlers } from '../../../datastores/handlers/index'
import { SORT_BY_VALUES } from '../../helpers/history'

const state = {
historyCacheSorted: [],
useAscendingOrder: SORT_BY_VALUES['DateAddedNewest'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
useAscendingOrder: SORT_BY_VALUES['DateAddedNewest'],
useAscendingOrder: SORT_BY_VALUES.DateAddedNewest,


// Vuex doesn't support Maps, so we have to use an object here instead
// TODO: switch to a Map during the Pinia migration
Expand All @@ -16,6 +18,10 @@ const getters = {

getHistoryCacheById(state) {
return state.historyCacheById
},

getSortOrder(state) {
Copy link
Member

Choose a reason for hiding this comment

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

Getters, mutations and actions are global in the store, please use namespaced names whenever possible (tells other developers where it comes from and avoids name conflicts).

Suggested change
getSortOrder(state) {
getHistorySortOrder(state) {

return state.useAscendingOrder
}
}

Expand All @@ -36,6 +42,15 @@ const actions = {
}
},

async selectSort({ commit }, sort) {
try {
const order = sort
commit('setSortOrder', order)
} catch (errMessage) {
console.error(errMessage)
}
},

Comment on lines +45 to +53
Copy link
Member

Choose a reason for hiding this comment

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

As this action is just a plain wrapper, there isn't any benefit to using it compared to just calling the mutation directly, so it's better to just call the mutation directly in this case.

Suggested change
async selectSort({ commit }, sort) {
try {
const order = sort
commit('setSortOrder', order)
} catch (errMessage) {
console.error(errMessage)
}
},

async updateHistory({ commit }, record) {
try {
await DBHistoryHandlers.upsert(record)
Expand Down Expand Up @@ -109,6 +124,11 @@ const mutations = {
state.historyCacheSorted = historyCacheSorted
},

setSortOrder(state, order) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setSortOrder(state, order) {
setHistorySortOrder(state, order) {

state.useAscendingOrder = order
return state.useAscendingOrder
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary return.

Suggested change
return state.useAscendingOrder

},

setHistoryCacheById(state, historyCacheById) {
state.historyCacheById = historyCacheById
},
Expand Down
27 changes: 22 additions & 5 deletions src/renderer/views/History/History.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { defineComponent } from 'vue'
import { isNavigationFailure, NavigationFailureType } from 'vue-router'
import debounce from 'lodash.debounce'
import FtLoader from '../../components/ft-loader/ft-loader.vue'
import FtSelect from '../../components/ft-select/ft-select.vue'
import FtCard from '../../components/ft-card/ft-card.vue'
import FtFlexBox from '../../components/ft-flex-box/ft-flex-box.vue'
import FtElementList from '../../components/FtElementList/FtElementList.vue'
Expand All @@ -10,6 +11,9 @@ import FtInput from '../../components/ft-input/ft-input.vue'
import FtAutoLoadNextPageWrapper from '../../components/ft-auto-load-next-page-wrapper/ft-auto-load-next-page-wrapper.vue'
import FtToggleSwitch from '../../components/ft-toggle-switch/ft-toggle-switch.vue'
import { ctrlFHandler } from '../../helpers/utils'
import { mapActions } from 'vuex'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { mapActions } from 'vuex'
import { mapMutations } from 'vuex'

import { getIconForSortPreference } from '../../helpers/utils'
import { SORT_BY_VALUES } from '../../helpers/history'

const identity = (v) => v

Expand All @@ -20,10 +24,7 @@ function filterVideosWithQuery(videos, query, attrProcessor = identity) {
} else if (typeof (video.author) === 'string' && attrProcessor(video.author).includes(query)) {
return true
}

return false
}).sort((a, b) => {
absidue marked this conversation as resolved.
Show resolved Hide resolved
return b.timeWatched - a.timeWatched
})
}

Expand All @@ -38,6 +39,7 @@ export default defineComponent({
'ft-input': FtInput,
'ft-auto-load-next-page-wrapper': FtAutoLoadNextPageWrapper,
'ft-toggle-switch': FtToggleSwitch,
'ft-select': FtSelect,
},
data: function () {
return {
Expand All @@ -51,17 +53,28 @@ export default defineComponent({
}
},
computed: {
sortByNames: function () {
return [
this.$t('History.Sort By.DateWatchedNewest'),
this.$t('History.Sort By.DateWatchedOldest'),
]
},
sortOrder: function() {
return this.$store.getters.getSortOrder
},
historyCacheSorted: function () {
return this.$store.getters.getHistoryCacheSorted
return this.sortOrder === SORT_BY_VALUES['DateAddedNewest'] ? this.$store.getters.getHistoryCacheSorted : this.$store.getters.getHistoryCacheSorted.toReversed()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return this.sortOrder === SORT_BY_VALUES['DateAddedNewest'] ? this.$store.getters.getHistoryCacheSorted : this.$store.getters.getHistoryCacheSorted.toReversed()
return this.sortOrder === SORT_BY_VALUES.DateAddedNewest ? this.$store.getters.getHistoryCacheSorted : this.$store.getters.getHistoryCacheSorted.toReversed()

},

fullData: function () {
if (this.historyCacheSorted.length < this.dataLimit) {
return this.historyCacheSorted
} else {
return this.historyCacheSorted.slice(0, this.dataLimit)
}
},
sortByValues() {
return Object.values(SORT_BY_VALUES)
}
},
watch: {
fullData() {
Expand Down Expand Up @@ -186,5 +199,9 @@ export default defineComponent({
keyboardShortcutHandler: function (event) {
ctrlFHandler(event, this.$refs.searchBar)
},
getIconForSortPreference: (s) => getIconForSortPreference(s),
...mapActions([
'selectSort'
])
Comment on lines +202 to +205
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getIconForSortPreference: (s) => getIconForSortPreference(s),
...mapActions([
'selectSort'
])
getIconForSortPreference,
...mapMutations([
'setHistorySortOrder'
])

}
})
11 changes: 11 additions & 0 deletions src/renderer/views/History/History.vue
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@
<div
class="optionsRow"
>
<ft-select
v-if="fullData.length > 1"
class="sortSelect"
:placeholder="$t('History.Sort By.Sort By')"
:value="sortOrder"
:select-names="sortByNames"
:select-values="sortByValues"
:icon="getIconForSortPreference(sortOrder)"
@change="selectSort"
/>

<ft-toggle-switch
v-if="fullData.length > 1"
:label="$t('History.Case Sensitive Search')"
Expand Down
4 changes: 4 additions & 0 deletions static/locales/en-US.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,10 @@ History:
Empty Search Message: There are no videos in your history that match your search
Search bar placeholder: "Search in History"
Case Sensitive Search: Case Sensitive Search
Sort By:
Sort By: Sort By
Copy link
Member

Choose a reason for hiding this comment

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

We really need to deduplicate this string, as this pull request will add the 6th Sort By string/text, I'll do a separate pull request for that though.

Copy link
Author

@jlvivero jlvivero Dec 27, 2024

Choose a reason for hiding this comment

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

do I

  • leave it like this for now then?
  • hard code the string for now (since this string doesn't have translation yet for this specific instance anyways?)
  • maybe reference another string for now?

Copy link
Member

Choose a reason for hiding this comment

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

I've opened #6495 to deduplicate the strings, if you are willing to wait until that pull request is merged and rebase, you'll be able to use the Global.Sort By string that that pull request adds.

Copy link
Author

Choose a reason for hiding this comment

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

I don't mind, since it's the holidays haven't had time to finish my changes, so I can wait and rebase

DateWatchedOldest: Earliest Watched First
DateWatchedNewest: Latest Watched First
Settings:
# On Settings Page
Settings: Settings
Expand Down
Loading