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

[Feature Request]: Add sort by date to the history page #5595

Open
3 tasks done
jlvivero opened this issue Aug 21, 2024 · 11 comments
Open
3 tasks done

[Feature Request]: Add sort by date to the history page #5595

jlvivero opened this issue Aug 21, 2024 · 11 comments

Comments

@jlvivero
Copy link

Guidelines

  • I have searched the issue tracker for open and closed issues that are similar to the feature request I want to file, without success.
  • I have searched the documentation for information that matches the description of the feature request I want to file, without success.
  • This issue contains only one feature request.

Problem Description

I'm unsure if I should make a new feature request for this, or not, but the history has no way of switching between old and new videos (I have to scroll all the way down to see older videos)

(there's somewhat similar request but none are quite exactly like this ask)

Proposed Solution

a sort by date/channel button in the history?

similar to the general search at top, the search in history back should have a button for filtering, for this specific request, just a sort by feature is more than enough

Alternatives Considered

pagination of the history page would also work to get to the bottom, but sorting probably feels a bit better to me

Issue Labels

ease of use improvement, new feature, new optional setting

Additional Information

I would imagine a filter button like the one that already exists in the top platform would be more than enough, for now sort by would be the only option and I would imagine the options would be:

viewed date
upload date

just those two options are probably more than enough for this feature

@jlvivero jlvivero added the enhancement New feature or request label Aug 21, 2024
@kommunarr kommunarr added the good first issue Good for newcomers label Oct 15, 2024
@kommunarr kommunarr changed the title [Feature Request]: Add sort by date/channel button/filter to the history page [Feature Request]: Add sort by date to the history page Oct 15, 2024
@kommunarr
Copy link
Collaborator

Limiting this issue to only be pertaining to a History Sort By for managed scope. Please create a separate issue with more refinement on the idea for the filtering if you still would like one.

The sort by should ideally have viewed date asc/desc and upload date asc/desc. There may be API-specific data complications with the latter if someone else has more context on that.

@jlvivero
Copy link
Author

jlvivero commented Oct 17, 2024

that's fine by me, that's imo the important aspect, the rest was mostly just as a reference, oh, and quick question that might be a bit more into the implementation detail

I see in src/datastore/handlers/base.js that we have the following lines

class History {
  static find() {
    return db.history.findAsync({}).sort({ timeWatched: -1 })
  }

would adding a parameters that changes the sort by be good enough? (so when you press a button the parameter becomes 1, then you press it again it becomes -1.

something like

class History {
  static find(sortby, direction) {
    return db.history.findAsync({}).sort({ sortby: direction })
  }

not sure what the frontend part would look like yet, since I haven't fully explored the code yet, but wanted to see if that approach made sense.

I also found in

src/renderer/store/modules/history.js

  async grabHistory({ commit }) {
    try {
      const results = await DBHistoryHandlers.find()

      const resultsById = {}
      results.forEach(video => {
        resultsById[video.videoId] = video
      })

      commit('setHistoryCacheSorted', results)
      commit('setHistoryCacheById', resultsById)
    } catch (errMessage) {
      console.error(errMessage)
    }
  },

which is where I would assume we would add the parameters, but it might also be where we do the sorting if we feel like we should sort here? (though I'm leaning more towards the first spot, and you would just pass the parameter?)

haven't used vue in a while, so I'm unfamiliar as to where the frontend part would go exactly and how you could pass around the settings, If I have time to take a stab at it I'll do a bit more research, otherwise I'll leave this just for whoever can take this issue has a reference on where things might be.

@kommunarr
Copy link
Collaborator

CC: @absidue and @PikachuEXE on your recommendations for handling history sorting. My less-informed assumption would be to add a mutation in src/renderer/store/modules/history.js that performs sorting on state.historyCacheSorted, and that way you avoid having to unnecessarily re-assign resultsById.

@PikachuEXE
Copy link
Collaborator

I don't think I know anything in this area
The only history handling performance PR I know is #4017

@m3thm4th

This comment was marked as spam.

@kommunarr
Copy link
Collaborator

@m3thm4th I think that's a neat idea. I'd recommend creating a separate feature request for it.

@m3thm4th

This comment was marked as spam.

@jlvivero
Copy link
Author

jlvivero commented Nov 1, 2024

I'm going out of town and will probably be back in a week or two, if no one has taken this by then, I'll take a stab at it

@jlvivero
Copy link
Author

so I did a bit of research for this

base.js is probably the best place to add a parameter, because it uses the db query and just gives it a direction, not interfering with any of the history limits that exists. (it basically has no performance impact that I could notice)

taht does mean that you have to send the direction from somewhere

src/datastores/handlers/electron.js

static find(direction) {
    return ipcRenderer.invoke(IpcChannels.DB_HISTORY, {
      action: DBActions.GENERAL.FIND,
      data: direction,
    });
  }

a data variable needs to be added to be properly handled

and that needs to also be called from
src/renderer/store/modules/history.js

const actions = {
  async grabHistory({ commit }) {
    try {
      let direction = 1;
      const results = await DBHistoryHandlers.find(direction);

      const resultsById = {};
      results.forEach((video) => {
        resultsById[video.videoId] = video;
      });

      commit("setHistoryCacheSorted", results);
      commit("setHistoryCacheById", resultsById);
    } catch (errMessage) {
      console.error(errMessage);
    }
  },

so both the locations I saw originally and the one in the middle need it, but the part where I'm not 100% sure is on the UI side of things

for testing purposes I added a simple switch on src/renderer/views/History/History.vue

and added the variable to History.js in the same path, but I don't think taht's where the search is called (only the limit part seems to be done there)

I'll do a bit more digging, but If there's anyone that has a decent idea of where the trigger for the history originates from, it would speed things up for sure.

(I'll take a stab at it next weekend most likely, and see what I can find)

@absidue
Copy link
Member

absidue commented Nov 17, 2024

@jlvivero grabHistory is called once at app startup, so that is not where you want to be looking. As the sort by drop down would be on the history page, it would probably be best to sort it there.

@jlvivero jlvivero mentioned this issue Nov 22, 2024
4 tasks
@jlvivero
Copy link
Author

so I made a PR and noticed I was never assigned to the issue itself, please assign it to me and feel free to review the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To assign
Development

No branches or pull requests

5 participants