-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Implemented RSS reader and auto downloader in reference WebUI #12557
Conversation
Well, I'll look into it ASAP. |
Sorry, what do you mean? |
Please remind/ask him to review all web ui/api related PRs. |
@Piccirello, please review this PR ASAP, since it's most wanted feature for many years. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks pretty good, from the perspective of how it fits into the current web app's architecture. I haven't evaluated the correctness of the implementation in any way, as I've never used qBittorrent's RSS feature. Someone else will need to rigorously test the implementation.
new Event(e).stop(); | ||
let completionCount = 0; | ||
rules.forEach(rule => { | ||
window.parent.qBittorrent.RssDownloader.modifyRuleState(rule, "previouslyMatchedEpisodes", [], () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, we can't yet use arrow functions while we support IE11. I'd love to drop IE11 support (as Bootstrap 5 has now done), but we've collectively decided to support it for a bit longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could tell IE11 support had already (unintentionally) been dropped. Since the refactor from 4.2.0 IE doesn't load the page correctly.
If you still want I can replace all arrow functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for officially dropping support for IE 11. I think it has been long enough anyway.
$('showRssReaderLink').addEvent('click', function(e) { | ||
showRssReader = !showRssReader; | ||
LocalPreferences.set('show_rss_reader', showRssReader.toString()); | ||
initializeTabs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, will this work? initializeTabs
is defined later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works because initializeTabs
isn't actually called there. It's only called on click so after initialization.
Though since there was quite a bit of duplicate code I moved things around and renamed the function to updateTabDisplay
to better reflect its purpose.
I also removed the adding and removing of the tabs event listeners , I think it's fine to have those three event listeners even when the buttons are not displayed. Please take a look.
Any news on this PR? Is there any showstopper? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I barely scratch the surface, it would take ages for only one man to review it all...
Works! |
Implemented RSS Reader and AutoDownloader in reference WebUI.
Rebased to resolve merge conflicts. |
Any update? |
Nothing new on this? |
@Chocobo1, if you don't have any serious comments in general, and you can't check it thoroughly, I would merge it as is and wait for user feedback. Judging by the related Issue, there is a fairly large user base. |
I'm ok if Piccirello is fine with it too. @Piccirello What do you think? |
I tested very quickly with a public RSS feed I found online and everything seemed to work. I'm ok with merging as long as we have users thoroughly test it before it makes it into a release. |
@seprode |
It would be useful if someone provides test builds with this feature merged. |
A quick test of a new master build worked perfectly (adding new subscription, updating it, selecting entry, getting details, double click to download). Couldn't test the rules system via the webUI, but will report later if any errors appear. Thanks a lot @seprode !!! |
@glassez 1 or 2 hours after this comment is posted, Windows builds with this feature will be available here: https://github.com/FranciscoPombal/qBittorrent/actions/runs/129842756 |
I found a bug in the webui. It truncates an rss url string. Max characters is set too low. Tried a jacket rss url (which are somewhat long) and it cut the string at a certain point. Try enter this and you will see. You wont get any results, but you can check in the feeds.json it has been cut. Also not sure where to post feedback on this new feature so i posted it here. Should i have opened an issue? UPDATE: edited manually with nano feeds.json with and its updating the feed. Now I have another issue. The RSS downloader popup containing all the forms to create new rules, has all the forms disabled. No error shows on log or browser console. Could it be the linuxserv docker image that is not good? |
I'm happy to help test and give feedback after I get my new server set up. I appreciate your patience with a new Linux user and right brainer. :) |
setup docker on your environment then use this image: Update: for more info on the variables go to https://hub.docker.com/r/linuxserver/qbittorrent |
Yes, that's definitely an issue. I think it should also be removed for newfolder and rename_feed.
The forms stay disabled when you select a rule? |
Disabled as soon as i hit the autodownloader button and the popup loads. I cant create any rules. Unraid This docker: Brave, tried edge aswel. |
So the context menu doesn't load? The Web UI is missing a dedicated add rule button and instead only has the context menu. Maybe that should be added. |
The rss popup for creating rules pops and i can see the forms, but the forms are disabled, so i cannot interact with them to create a new rule. |
@jobrien2001 |
I cannot test atm but i will report back and see if I can get past it. I would agree the add rule button should be there. Maybe the forms hidden when they are in a disabled state. |
I have started work on fixes in this branch. Should I create an issue for each fix/commit or squash all changes? |
Fixes look good. I would say one PR for all these fixes, but with the 3 separate commits. Then others can decide whether or not they should be squashed (in which case the purpose of each change should be mentioned in the commit message body). |
Everything working properly from the UI, but there is still a problem with handling completed rss torrents. A lot of us use jackett as a proxy to get rss magnets. Example rss item from jackett, note the id and torrenturl attributes (the id is a link to the page and the torrenturl a link that will redirect to a magnet).
Jackett's torrentURL and link attributes change every single time an rss is requested. If i set an autodownloader rule and it downloads an item it will not be marked I have 2 suspicions:
I did some digging and these 2 blocks of code look where this is happening in rss_autodownloader.cpp I do not know the language so please forgive any errors in my assessment.
"if (Feed *feed = Session::instance()->feedByURL(job->feedURL))"
"if (BitTorrent::MagnetUri(torrentURL).isValid()) {" |
Update: If I download manually the rss article in the webui by using the RSS tab and right clicking on a giving article and selecting download.... it will correctly mark the article with Something must be different in the rss auto download code. Here is confirmation that its working properly marking
|
@jobrien2001, your last comments are unrelated to both current topic and WebUI. If you want to report some problem in RSS core functionality you should create separate Issue (one Issue per problem). |
Add rule button and feed max length is working now. Manual download button is working and so is Open news. BTW the buttons looks great and the user interface looks more straight forward because of them. |
Not sure if this is WebUI or another RSS core problem. -add a feed Rule still exists assigned to that feed (which doesnt exist anymore). This might be harmless or intended to preserve the rule settings. The reference to the feed can be removed by simply opening the rule and hitting save again. |
3 Days running without a problem. 1 thought about the icons: I found myself clicking on the big trashcan icon on the main toolbar instead of the small trashcan when trying to delete a rule. Maybe a plus and minus would be better choice for icons to distinguish from the main toolbar. @glassez Created a separate issue. Thank you. |
As long as this doesn't break anything I would keep it as intended behavior. Even if this were considered a bug I don't think the WebUI should deal with it. Instead that should be changed at API or Core level.
Currently all shown icon are identical to the desktop UI. If we were to change them here they should also be changed in the desktop UI. I thought about adding icons to the "New subscription", "Mark items read", "Update all" and "RSS Downloader..." buttons. The problem is that it the regular buttons are styled by the browser and thus the custom styling for the image buttons would only really be fitting for one browser. |
It's a disadvantage of currently used approach, IMO. I have repeatedly suggested that we separate Web UI from Desktop UI completely. Really they are separate applications. |
I've yet to see this suggested by anyone that actually uses the Web UI. If this split were to happen, I predict that the Web UI and APIs would quickly fall behind in functionality. |
May I suggest an enhancement? A size column in the RSS list, if thats its available in the API. Some feeds do provide a size attribute. |
@jobrien2001 please open new threads to discuss each new bug or feature. |
Implemented RSS reader and auto downloader in reference WebUI
I have implemented the RSS reader and auto downloader in the reference WebUI. Requires API defined in #12549 and closes #12355.
Screenshots:
Regarding localization: all used strings are in the same context as their desktop ui version. Is there an easy way to port the translations over?
Also the
renameRule
API seems to be broken. Since I couldn't figure out where the mistake lies I created an issue.