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 date column to the built-in search engine #20703

Merged
merged 2 commits into from Apr 29, 2024

Conversation

ducalex
Copy link
Contributor

@ducalex ducalex commented Apr 14, 2024

This PR adds a date column to the built-in search engine to show when a torrent was published/uploaded on the torrent site.

When a plugin wants to show a date, it can now add a date entry to its result dict. The value format is a unix timestamp (an integer representing seconds since epoch).

This commit adds the date column to both the Qt UI and the WebUI and updates the nova3 prettyPrinter.

Plugins with no date support will keep working.

If this PR is accepted, I will send a PR to the plugins repo to update as many plugins as I can! Here's the updated thepiratebay for testing the PR: https://github.com/ducalex/qbittorrent-search-plugins/blob/ducalex/add-date-field-tpb/nova3/engines/piratebay.py

Related open issues: #1463

Still to do are translations (a single additional string "Date"), am I expected to update all translations?

The guidelines suggest I include a screenshot (desktop top, webui bottom):
Untitled

@luzpaz luzpaz added Search engine Issues related to the search engine/search plugins functionality PR waiting review labels Apr 14, 2024
src/base/search/searchhandler.cpp Outdated Show resolved Hide resolved
src/base/search/searchhandler.cpp Outdated Show resolved Hide resolved
@glassez glassez requested a review from a team April 18, 2024 06:08
@Piccirello
Copy link
Member

This is a nice addition! I think a more descriptive name could be useful, like "Uploaded On" or "Created On".

I'll note that I personally prefer "At" to "On" (i.e. "Uploaded At") since this field includes the time, however the Transfers table already uses On for this use case (e.g. "Added On', "Completed On").

@ducalex
Copy link
Contributor Author

ducalex commented Apr 18, 2024

This is a nice addition! I think a more descriptive name could be useful, like "Uploaded On" or "Created On".

I'll note that I personally prefer "At" to "On" (i.e. "Uploaded At") since this field includes the time, however the Transfers table already uses On for this use case (e.g. "Added On', "Completed On").

I struggled to choose a name! I've considered Added, Uploaded, Created, and Published. I quickly dismissed Created because the torrent itself also has a creation date that is unrelated to when i twas sent to the website so it might be misleading (Though if you think Created is best, I'll still go with it of course). I've looked at what the torrent websites usually name that column and "Added" or "Added On" seemed the most common, followed by simply "Date" or "Time".

There's also the question of all the other symbols added by this PR:

  • SearchResultColumn enum name (currently PL_DATE)
  • SearchResult member variable name (currently date)
  • SearchSortModel SearchColumn enum name (currently DATE)
  • The key used in json serialization for the WebUI (currently date)
  • The key used in the search plugins (currently date)

Do you foresee a future where more than one "date" might be returned by a search result? If so maybe I should rename all of them to something better, but what?

@glassez
Copy link
Member

glassez commented Apr 19, 2024

I struggled to choose a name! I've considered Added, Uploaded, Created, and Published

I would vote for Published.

@glassez
Copy link
Member

glassez commented Apr 19, 2024

maybe I should rename all of them to something better, but what?

Accordingly to chosen name, e.g. PL_PUBLICATION_DATE.

@Piccirello
Copy link
Member

My personal choice is for Uploaded On. If we do go with Published instead, I'd strongly push for appending "On" to make clear that it's a date.

Do you foresee a future where more than one "date" might be returned by a search result? If so maybe I should rename all of them to something better, but what?

It's unlikely that there will be, but it's also notoriously difficult to predict future improvements. I never would have predicted this date field! The safest option would be to rename the relevant symbols once you decide on a new column name (e.g. uploaded_on).

@glassez
Copy link
Member

glassez commented Apr 19, 2024

If we do go with Published instead, I'd strongly push for appending "On" to make clear that it's a date.

It was implied that any of the suggested options would have been used with the addition of "On", wasn't it? At least that's how I thought of it. So yes, I vote for "Published On".

@ducalex
Copy link
Contributor Author

ducalex commented Apr 19, 2024

"Published On" and "Uploaded On" are both good options. Neither seem to have an edge when it comes to translations.

So what do we do? Wait for others to chime in? Am I the tie breaker? Flip a coin?

@@ -533,6 +534,7 @@ void SearchJobWidget::appendSearchResults(const QVector<SearchResult> &results)
setModelData(SearchSortModel::SIZE, Utils::Misc::friendlyUnit(result.fileSize), result.fileSize, (Qt::AlignRight | Qt::AlignVCenter));
setModelData(SearchSortModel::SEEDS, QString::number(result.nbSeeders), result.nbSeeders, (Qt::AlignRight | Qt::AlignVCenter));
setModelData(SearchSortModel::LEECHES, QString::number(result.nbLeechers), result.nbLeechers, (Qt::AlignRight | Qt::AlignVCenter));
setModelData(SearchSortModel::DATE, QLocale().toString(result.date.toLocalTime(), QLocale::ShortFormat), result.date);
Copy link
Member

Choose a reason for hiding this comment

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

How does it look like if the plugin does not provide a date?

Copy link
Contributor Author

@ducalex ducalex Apr 19, 2024

Choose a reason for hiding this comment

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

It's blank when not set. I think it's also blank on any invalid QDateTime (which shouldn't happen, but who knows).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer it shows Unknown? Perhaps we should add a new helper Utils::Misc::friendlyDate to handle it?

Copy link
Member

Choose a reason for hiding this comment

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

Would you prefer it shows Unknown?

I would keep it blank for now.

@xavier2k6
Copy link
Member

I struggled to choose a name! I've considered Added, Uploaded, Created, and Published

I would vote for Published.

Another vote in favor of Published On.

@Pentaphon
Copy link

Another vote in favor of Published On.

This is the only right solution for the new column.

@ducalex
Copy link
Contributor Author

ducalex commented Apr 21, 2024

I've made the changes for "Published On" and renamed struct/enum members to pubDate/PUB_DATE (I went with abbreviation because other sibling members also used abbreviation, like DESC. but let me know if you want a full publicationDate/PUBLICATION_DATE).

src/base/search/searchhandler.cpp Outdated Show resolved Hide resolved
src/webui/api/searchcontroller.cpp Outdated Show resolved Hide resolved
@glassez glassez requested a review from a team April 23, 2024 04:45
@glassez glassez added this to the 5.0 milestone Apr 23, 2024
@glassez glassez self-requested a review April 23, 2024 07:20
glassez
glassez previously approved these changes Apr 23, 2024
xavier2k6
xavier2k6 previously approved these changes Apr 23, 2024
@ducalex ducalex force-pushed the ducalex/add-date-to-search branch 2 times, most recently from 3fc91c9 to 4a24097 Compare April 23, 2024 13:59
This adds a date column to the built-in search engine to show when a torrent was published/uploaded on the engine site.

When a plugin wants to show a date, it can now add a `pub_date` entry to its result dict. The value format is a unix timestamp (an integer representing seconds since epoch).

This commit adds the date column to both the Qt UI and the WebUI.

Plugins with no date support will keep working.

Still to do are translation (a single additional string "Published On")
@ducalex

This comment was marked as resolved.

@glassez

This comment was marked as resolved.

@stalkerok
Copy link
Contributor

Published On

Very strange decision to choose this particular name, it is not used anywhere on trackers, usually it is “Added” “Uploaded”

@Pentaphon
Copy link

Pentaphon commented Apr 24, 2024

Very strange decision to choose this particular name, it is not used anywhere on trackers, usually it is “Added” “Uploaded”

The search engine is not a tracker so "published on" works for all sources regardless of what they use and best of all, makes the most sense.

@stalkerok
Copy link
Contributor

The search engine is not a tracker so "published on" works for all sources regardless of what they use and best of all, makes the most sense.

Nevertheless, you search by trackers and the displayed date is also taken from the tracker. IMO, I would not deviate from the variant used by trackers.

@Pentaphon
Copy link

Nevertheless, you search by trackers and the displayed date is also taken from the tracker. IMO, I would not deviate from the variant used by trackers.

What they use varies. It's best to pick one thing for our application that makes sense. A torrent is published on a site so "published on" makes sense. If you can come up with a column name that people think is better than "published on", I'd like to see it.

@stalkerok
Copy link
Contributor

stalkerok commented Apr 25, 2024

Don't have to make anything up, most often it's “Added” with or without “On”.
(I don't insist, we can leave it as it is.)

@stalkerok

This comment was marked as outdated.

@glassez

This comment was marked as duplicate.

@stalkerok

This comment was marked as outdated.

@glassez

This comment was marked as resolved.

@stalkerok

This comment was marked as outdated.

@ducalex
Copy link
Contributor Author

ducalex commented Apr 25, 2024

He just didn't change it accordingly. It works if you change 'date' to 'pub_date'.

That's my bad, I've updated the link in the description to point to the branch head.

There's a few more plugins available (you'll have to pick the appropriate file from each branch): https://github.com/ducalex/qbittorrent-search-plugins/branches though some are still WIP like EZTV.

As for the name of the column I don't have much of an opinion, I changed it to Published On (perhaps prematurely) because it seemed to go in that direction in the comments.

@ducalex ducalex dismissed stale reviews from xavier2k6 and glassez via fb7015d April 25, 2024 17:17
@glassez glassez merged commit 42b8796 into qbittorrent:master Apr 29, 2024
9 of 14 checks passed
@glassez
Copy link
Member

glassez commented Apr 29, 2024

@ducalex
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Search engine Issues related to the search engine/search plugins functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants