-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: master
Are you sure you want to change the base?
Conversation
6c0b835
to
d2adf04
Compare
(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 |
Trying to remove the json dependency as it was duplicated and unnecessary for this work |
Downgrading kitteh irc seemed to have solved my issues - about to push up my changes. |
23d88ee
to
3f6c213
Compare
349f8e1
to
6c194c9
Compare
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 |
Should be less daunting to review now. Thank you in advance. |
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.
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!
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.