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

Upgrade IRC to v0.4 (6d90a43) #7242

Closed
wants to merge 1 commit into from
Closed

Upgrade IRC to v0.4 (6d90a43) #7242

wants to merge 1 commit into from
+1 −1

Conversation

ryanwohara
Copy link
Contributor

  • Improved side panel
  • Join/part/quit/kick messages
  • ;;ms for MemoServ
  • View the topic
  • Previous startup messages are hidden and replaced with the MOTD
  • Rejoin channel on kick
  • Renamed delimiter to prefix

@runelite-github-app
Copy link

runelite-github-app bot commented Jan 16, 2025 •
edited
Loading

irc: 636f6b940d73176974497b1fb961dad4b55b1e26...6d90a4386ab0ec8bd14681a7f87981c8ab065b3c

@ryanwohara ryanwohara force-pushed the irc branch 2 times, most recently from cdd1e46 to 3e7a98d Compare January 16, 2025 21:11
@ryanwohara ryanwohara changed the title Upgrade IRC to 0.4 (4caa297) Upgrade IRC to 0.4 (1c16455) Jan 16, 2025
@ryanwohara ryanwohara changed the title Upgrade IRC to 0.4 (1c16455) Upgrade IRC to 0.4 (5b8fc54) Jan 16, 2025
@ryanwohara
Copy link
Contributor Author

Sorry for the big diffs on two files - had to change the line endings from CRLF to LF

@ryanwohara ryanwohara changed the title Upgrade IRC to 0.4 (5b8fc54) Upgrade IRC to v0.4 (5b8fc54) Jan 18, 2025
@LlemonDuck
Copy link
Contributor

Can you please escape the incoming messages with StringEscapeUtils.escapeHtml4()?
https://github.com/ryanwohara/irc-plugin/compare/636f6b940d73176974497b1fb961dad4b55b1e26...ryanwohara:5b8fc54c039ff4d581c352f61c1024e867afe113?w=1#diff-b9c77d977dbe541b12889ed0b50b9406cb2597a878881c8970015319bf21c796R82

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 25, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 25, 2025
@ryanwohara
Copy link
Contributor Author

@LlemonDuck Thank you. Just added it here: ryanwohara/irc-plugin@636f6b9...ryanwohara:8a1c1820dd7cacefd1faf815ae84fed5e42f555b#diff-b9c77d977dbe541b12889ed0b50b9406cb2597a878881c8970015319bf21c796R83

@ryanwohara ryanwohara changed the title Upgrade IRC to v0.4 (5b8fc54) Upgrade IRC to v0.4 (8a1c182) Jan 25, 2025
@ryanwohara
Copy link
Contributor Author

Actually that seems to break my app
image

Without that tweak it renders as expected. I'm not familiar enough with JLabel to understand why 😬

Can I revert this change?

@ryanwohara ryanwohara marked this pull request as draft January 25, 2025 05:31
@ryanwohara
Copy link
Contributor Author

Converting this to a draft until I can revert this change. I don't think it's usable as it is now.

ryanwohara added a commit to ryanwohara/irc-plugin that referenced this pull request Jan 25, 2025
@ryanwohara
fix: Revert "fix: HTML escape input"
6d90a43 
This reverts commit 8a1c182.

JLabel seems to already safely display the characters. Adding
the StringEscapeUtils.escapeHtml4() function displays things
in a way that is impossible to read as a human.

runelite/plugin-hub#7242 (comment)
@ryanwohara ryanwohara changed the title Upgrade IRC to v0.4 (8a1c182) Upgrade IRC to v0.4 (6d90a43) Jan 25, 2025
@ryanwohara ryanwohara marked this pull request as ready for review January 25, 2025 18:06
@ryanwohara
Copy link
Contributor Author

ryanwohara commented Jan 25, 2025 •
edited
Loading

Moved back out of draft status.

Just to reiterate: I cannot use that StringEscapeUtils.escapeHtml4() method as < will display as &lt; in a literal sense.

@LlemonDuck
Copy link
Contributor

That is the goal, yes. Unsanitized IRC messages shouldn't be placed into html tags. If you want to do any other formatting first, you should do it after escaping.

@ryanwohara
Copy link
Contributor Author

@LlemonDuck No, this is not the goal. I want things to be readable.

This is what happens without your suggestion:
image

This is what happens with your suggestion:
image

It's not evaluating the HTML entities.
Also, this was fine as per the previous reviewers on this plugin.

@LlemonDuck
Copy link
Contributor

That behaviour is surprising to me, can you point to where it's being escaped then? This would even conflict with your desired URL replacing since that relies on the JLabel interpreting that anchor tag correctly.

This snippet does perform rich formatting based on message's contents, which is what I would expect to happen based on your code:

String message = "Dragon: <div>foo</foo>";

JLabel label = new JLabel("<html>" + message + "</html>");
add(label);

Additionally, using message = "Dragon: &lt;div&gt;foo&lt;/foo&gt;"; renders correctly with the characters <div>foo</foo> shown on the label.

I didn't see any preprocessing of control characters in Message.java either.

In terms of other notes:

  • Rich-replacing links with anchor elements (if I'm interpreting your intent correctly) is a bad idea as it hides the destination URL. In a browser one can hover the link to see a preview or copy the link without loading it to view it in full. Neither are possible by default in Swing and you provide no alternative means either.
  • Since you're doing I/O, you need to add a warning to the plugin hub manifest (example describing the data you're egressing. Something like This plugin submits your IP address and applicable IRC messages to a third party server not controlled by the RuneLite developers.
  • Use RuneLite's LinkBrowser utility class instead of Desktop to open links.

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 26, 2025
@ryanwohara ryanwohara closed this Jan 26, 2025
@ryanwohara
Copy link
Contributor Author

Making me jump through hoops not previously required is rude.

@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 26, 2025
@ryanwohara ryanwohara deleted the irc branch January 26, 2025 23:06
@LlemonDuck
Copy link
Contributor

Some of these changes are still going to be required even if you don't want to go forward with the substantive update or else it puts your plugin at risk of being disabled. Of particular importance would be the hub manifest warning.

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
plugin change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanwohara @LlemonDuck