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

add viewed date support #626

Merged
merged 4 commits into from
Jan 16, 2023
Merged

add viewed date support #626

merged 4 commits into from
Jan 16, 2023

Conversation

jpgichw
Copy link
Contributor

@jpgichw jpgichw commented Jan 14, 2023

  • add the viewed date (in the end of the array by reversing with tac) on executing add_to_hist, format is "12/10/22 12:48:06 +1100"
  • make it viewable in the interface (only while checking history)
  • alter interface width to add space for viewed date

adapted from a74ca6c

reminder that @pystardust might want to remove upload date altogether to make it less cluttered #171 (comment)

this is an old altered version I had lying around on my computer, seems like there is a lot that has changed since then. still working after a bit of modification. in the text interface the viewed date only shows the date and time, not the timezone but the timezone is shown in the thumbnail interface.

extra commands: date, tac. both are part of the GNU coreutils

- add the viewed date (in the end of the array by reversing with tac) on executing add_to_hist, format is "12/10/22 12:48:06 +1100"
- make it viewable in the interface (only while checking history)
- alter interface width to add space for viewed date

adapted from pystardust@a74ca6c
@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 14, 2023

I think, instead of adding a viewed attribute to the json in the add_to_hist function, we should just change the date attribute to be date +'%m\/%d\/%y %H:%M:%S %z'. That way no other modifications are necessary, ie the changes made in video_info_text, thumbnail_video_info_text, etc...

The reason I say this, is because the attributes that are present are not meant to necessarily represent one specific thing. For watch history we can just make date represent the watch date instead of upload date.

Edit:
lets also avoid tac because it's not posix compliant

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 14, 2023

I've experimented this before, if you add the date command in the attribute instead of the add_to_hist function, it will add the date at an inaccurate time (i.e. when you search vs when you open the video).

imo using the date attribute for viewed times as it is a bit misleading/unclear, will also break the interface as it'll show the supposed "viewed" dates when searching, unless you wanna also hide/remove the upload date altogether. I think it's an important feature to show how relevant a video is.

I used tac so I can add the attribute in the third line from the bottom. for an example, I can't use sed to add to the with bottom of the array with the same add_to_hist command for peertube history because it has a different number of attributes. however I can just add it to the top like this without tac:

[
  {
    "viewed": "01/14/23 12:23:00 +1100",
    "scraper": "invidious_search",
    "ID": "dQw4w9WgXcQ",
    "url": "https://youtube.com/watch?v=dQw4w9WgXcQ",
    "title": "Some title",
    "channel": "channel name",
    "thumbs": "https://invidious.lidarshield.cloud/vi/dQw4w9WgXcQ/hqdefault.jpg",
    "duration": "3:22",
    "views": "5114043",
    "date": "4 years ago",
    "description": ""
  }
]

should I change the function?

@Euro20179
Copy link
Collaborator

imo using the date attribute for viewed times as it is a bit misleading/unclear, will also break the interface as it'll show the supposed "viewed" dates when searching.

I think you misunderstood me. I am not saying we should redefine the date attribute to be when the video was viewed. I am saying that in the add_to_hist function we run a command to change what the date attribute equals. We would change it to what your proposed viewed attribute would be, so date +'%m\/%d\/%y %H:%M:%S %z'.

This way we don't have to deal with the viewed attribute.

@jpgichw jpgichw marked this pull request as draft January 14, 2023 07:52
@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 15, 2023

@Euro20179 ah I see. still, people still might want to use that metadata even if it's just for viewing history. I'm aware that youtube upload dates is relative but we should keep it in case we fix it to a normal date.

I personally browse my history like a diary sometimes, just looking at the dates when I watch it and when it's uploaded is good for context.

@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 15, 2023

but we should keep it in case we fix it to a normal date.

This is a good point.

What I dont like is that this adds a special property just for the history scraper. So I have an idea. Im going to allow scrapers to create their own version of video_info_text similar to thumbnail_video_info_text, then there is no need for this line: [ $scrape = "history" ] && printf "%-${viewed_len}.${viewed_len}s\t" "$viewed", In addition any more special properties added in the future can be handled by it's respective scraper better.

Please fix the use of tac, the viewed property can be anywhere it doesn't really matter, and I will merge this.

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 15, 2023

@Euro20179 alright. should I remove the special property now or we'll deal with it later? I'll add a todo comment just in case

@Euro20179
Copy link
Collaborator

Leave the special property, I'm gonna try my hand at integrating it in better. Just try to remove the use of tac.

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 15, 2023

ps: I think you have to request changes on your end for me to work on it

@Euro20179 Euro20179 marked this pull request as ready for review January 15, 2023 07:22
@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 15, 2023

using sed -z will have the same result as with tac. it'll add it in the end of the rows

@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 15, 2023

It doesnt need to be at the end, also I dont think the -z option is posix complient.

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 16, 2023

sorry, mild ocd

@Euro20179
Copy link
Collaborator

lol, if you want it to be at the end then that's fine, just make sure its posix complient.

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 16, 2023

I'll find a way later (if I do), this is alright.

@jpgichw
Copy link
Contributor Author

jpgichw commented Jan 16, 2023

there is a weird behavior if you combine the two sed commands, it inserts on the third line normally, but if you use the -c u flag it inserts on the fourth line.

it doesn't break anything though. but I'll undo this just to be sure.

@Euro20179
Copy link
Collaborator

by the way, feel free to write your own add_to_hist function in your ytfzf config, you can make it to whatever you want, so if you want to make it so that the viewed property is at the top, go for it.

@Euro20179 Euro20179 merged commit e5cad4a into pystardust:development Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants