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

Update IRC to v1.0.2 (9ef5d8d) #7474

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ryanwohara
Copy link
Contributor

Complete refactor of the architecture.
More configuration options.
Multiple channel support.
Server selection.
Links are clickable.
Images can be previewed without your browser.
Font selector.
Best effort emoji support.
Ctrl+k, Ctrl+i, Ctrl+b, Ctrl+u support in the side panel.

Includes authors and new warning in the plugin metadata.

@runelite-github-app
Copy link

runelite-github-app bot commented Feb 19, 2025

@ryanwohara ryanwohara marked this pull request as draft February 19, 2025 06:47
@ryanwohara ryanwohara force-pushed the irc branch 6 times, most recently from 6c0b835 to d2adf04 Compare February 19, 2025 08:23
@ryanwohara ryanwohara changed the title IRC v1.0 Update IRC to v1.0 Feb 19, 2025
@ryanwohara ryanwohara changed the title Update IRC to v1.0 Update IRC to v1.0 (e12516c) Feb 19, 2025
@ryanwohara ryanwohara changed the title Update IRC to v1.0 (e12516c) Update IRC to v1.0 (da80a97) Feb 19, 2025
@ryanwohara ryanwohara changed the title Update IRC to v1.0 (da80a97) Update IRC to v1.0 (3a96374) Feb 19, 2025
@ryanwohara ryanwohara marked this pull request as ready for review February 19, 2025 17:37
@ryanwohara ryanwohara requested a review from a team as a code owner February 19, 2025 17:37
@ryanwohara
Copy link
Contributor Author

(This was discussed in Discord but I'm noting here for the sake of everyone)

I cannot find any instance of System.out calls in my codebase 🤔 this might be a false positive in the automated checks

@ryanwohara ryanwohara changed the title Update IRC to v1.0 (3a96374) Update IRC to v1.0 (e542b8d) Feb 19, 2025
@ryanwohara
Copy link
Contributor Author

Trying to remove the json dependency as it was duplicated and unnecessary for this work

@ryanwohara ryanwohara changed the title Update IRC to v1.0 (e542b8d) Update IRC to v1.0 (008ef4d) Feb 19, 2025
@ryanwohara
Copy link
Contributor Author

Downgrading kitteh irc seemed to have solved my issues - about to push up my changes.

@ryanwohara ryanwohara force-pushed the irc branch 2 times, most recently from 23d88ee to 3f6c213 Compare February 19, 2025 23:16
@ryanwohara ryanwohara requested a review from a team as a code owner February 19, 2025 23:16
@ryanwohara ryanwohara force-pushed the irc branch 2 times, most recently from 349f8e1 to 6c194c9 Compare February 19, 2025 23:27
@ryanwohara ryanwohara marked this pull request as draft March 5, 2025 16:34
@ryanwohara ryanwohara changed the title Update IRC to v1.0 (008ef4d) Update IRC to v1.0.0 (7be0991) Mar 8, 2025
@ryanwohara ryanwohara marked this pull request as ready for review March 8, 2025 00:15
@ryanwohara
Copy link
Contributor Author

image

This PR isn't as bad as it looks on the surface, I promise

@ryanwohara ryanwohara changed the title Update IRC to v1.0.0 (7be0991) Update IRC to v1.0.1 (0543db5) Mar 14, 2025
@runelite-github-app
Copy link

runelite-github-app bot commented Mar 14, 2025

This plugin requires a review from a Plugin Hub maintainer. The reviewer will request any additional changes if needed.


Internal use only: Reviewer details Maintainer details

@ryanwohara
Copy link
Contributor Author

Showing  with 3,003 additions and 997 deletions.

Should be less daunting to review now.

Thank you in advance.

Copy link
Contributor

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

You've got a lot of regexes that are used a lot of times without pre-compiling (e.g. stripStyles & the part that prevents sending ;P as an IRC message, message boldness/underline/italic/color matching, link matching). It might be an idea too compile them once & re-use them

an NPE is fired if a user tries to send an IRC message by typing ;hi with no username set (i.e. before ircAdapter is set). Mark ircAdapter as nullable to get hints for other places that might be without checks.

The link parsing makes it possible to do some funky stuff with the <a href element that's created. That regex should probably be locked down a bit more. (example message: https://f'>a</a><b>hi)

I think it'd be a good idea to leave link previews for a separate PR - they're pretty janky right now:

  • multiple link previews can get stuck on screen
  • link preview says it accepts avif/webp but no animation works
  • link preview user agent feels weird

non-blocking:

  • You can parse emojis straight into the final map or list instead of parsing them into a temporary map/list then copying them into the final map
  • DMs can have dupe entries in the panel
  • closePane function looks a bit rough, around ~366 you'd probably want to null-check to avoid NPE

The PR was very reviewable now, thanks for pulling out the extra dependencies!

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Mar 23, 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 Mar 24, 2025
@ryanwohara ryanwohara changed the title Update IRC to v1.0.1 (0543db5) Update IRC to v1.0.2 (9ef5d8d) Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants