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.

Sign up for GitHub

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

Jump to bottom

Fix RSS URL rule error #2760

Merged
merged 4 commits into from
Oct 26, 2024
Merged

Fix RSS URL rule error #2760

merged 4 commits into from
Oct 26, 2024

Conversation

jevenski
Copy link
Contributor

  • Fix error when trying to add new RSS rules using the add button. This error probably was probably introduced when the dialog was rewritten with jQuery.
  • The scope of functions defined in this plugin (rssurlrewrite) seemed a little messy. Some functions need to go into the plugin object, some need to go into the theWebUI object, and others need to stay local. I moved some of these functions a bit, but that's still not perfect yet. Some further fine tunings are still required.

Related: #2753

@jevenski
Fix RSS URL rule error
4e3b613 
- Fix error when trying to add new RSS rules using the add button.
  This error probably was probably introduced when the dialog
  was rewritten with jQuery.
- The scope of functions defined in this plugin (rssurlrewrite)
  seemed a little messy. Some functions need to go into the `plugin`
  object, some need to go into the `theWebUI` object, and others
  need to stay local. I moved some of these functions a bit, but
  that's still not perfect yet. Some further fine tunings are still
  required.
jevenski
jevenski commented Oct 24, 2024
View reviewed changes
plugins/rssurlrewrite/init.js
Comment on lines -6 to +8
theWebUI.curRule = null;
theWebUI.maxRuleNo = 0;
plugin.curRule = null;
plugin.maxRuleNo = 0;
plugin.rules = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These attributes should be local to the plugin itself, instead of the global theWebUI object.

jevenski
jevenski commented Oct 24, 2024
View reviewed changes
plugins/rssurlrewrite/init.js
theWebUI.showRules = function()
{
theWebUI.request("?action=getrules",[theWebUI.loadRules, this]);
function makeRuleListItem(rule, index) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extract this action for it to be reused in addNewRule() and loadRules().

jevenski
jevenski commented Oct 24, 2024
View reviewed changes
plugins/rssurlrewrite/init.js
* on rule's textbox focus events, so the `this` keyword
* refers to the event target, i.e. the focused rule item.
*/
function selectRule() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is only going to be used as an event handler for focusing on a rule list item, so a local function will work fine.

@stickz
Copy link
Collaborator

stickz commented Oct 24, 2024

@koblack Could you test this before I merge? I'm not an expert on the RSS elements of ruTorrent.

@koblack
Copy link
Contributor

koblack commented Oct 25, 2024 •
edited
Loading

Tested on 279f45b
Test button "?" returns error

Uncaught TypeError: this.request is not a function from TypeError: this.request is not a function
    at theWebUI.checkCurrentRule (<anonymous>:1151:87)
    at HTMLButtonElement.dispatch (http://xxx/ruTorrent-jevenski/js/jquery.js?v=51B2:2:40035)
    at v.handle (http://xxx/ruTorrent-jevenski/js/jquery.js?v=51B2:2:38006)

image

@jevenski
Copy link
Contributor Author

jevenski commented Oct 25, 2024 •
edited
Loading

I did some further adjustments towards the scope of plugin functions. The culprit was still the this keyword in JavsScript.

As a sidenote, it seems to me that the checkCurrentRule() function sends the Regexp tests to the server and retrieves the test result from it. The PHP code block does nothing server-specific but running the Regexp tests and spitting the results. It looks like a waste of resources because this can be done perfectly on the client side. Correct me if I missed anything.

@stickz
Copy link
Collaborator

stickz commented Oct 25, 2024

@koblack Could you confirm this PR is ready to merge?

@jevenski We can adjust checkCurrentRule() in v5.2. I would also like to drop support for Internet Explorer in v5.2 as well. Everything is perfectly functional right now. Let's keep it "as is" so we can release this version in the near future!

@stickz stickz mentioned this pull request Oct 25, 2024
Closed
@stickz stickz linked an issue Oct 25, 2024 that may be closed by this pull request
JS error: [ : 1153] Uncaught TypeError: Cannot read properties of undefined (reading 'length') #2753
Closed
2 tasks
@koblack
Copy link
Contributor

koblack commented Oct 26, 2024

Everything seems to be working fine now! Thanks a lot!

@stickz stickz merged commit e21d069 into Novik:master Oct 26, 2024
@jevenski jevenski deleted the add-rule branch October 26, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug v5.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JS error: [ : 1153] Uncaught TypeError: Cannot read properties of undefined (reading 'length')
3 participants
@jevenski @stickz @koblack