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

feat: use message's original channel name when usercard popout is opened #3623

Merged

Conversation

jupjohn
Copy link
Contributor

@jupjohn jupjohn commented Mar 22, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR aims to fix an issue where opening a user's popout from the usercard in a non-user channel (/mentions) doesnt open use the channel's name. This caused the popout to have an invalid path and 404.

I'm not saying my fix is 'correct', but it's the best I can do without either passing too much info down or reworking it for the sake of a popout.

Fixes #2248

@jupjohn
Copy link
Contributor Author

jupjohn commented Mar 22, 2022

oh yeah, fixes #2248

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

Functionality works as intended, fixing the original issue. 👍

@Mm2PL Mm2PL self-requested a review March 22, 2022 09:05
@kornes
Copy link
Contributor

kornes commented Mar 24, 2022

requests to ivr api are also currently broken on /mentions tab due to same underlying issue. Can be fixed in scope of this PR here:

this->userName_, this->channel_->getName(),

@jupjohn
Copy link
Contributor Author

jupjohn commented Mar 24, 2022

requests to ivr api are also currently broken on /mentions tab due to same underlying issue. Can be fixed in scope of this PR here:

Good catch, thanks! Will fix it later today.

@kornes
Copy link
Contributor

kornes commented Mar 25, 2022

and one more with same problem - middle mouse button on username

openTwitchUsercard(this->channel_->getName(),

@jupjohn
Copy link
Contributor Author

jupjohn commented Mar 25, 2022

I'm wondering if it's worth rejigging this to pass through the original ChannelPtr instead of just a name. That way the snapshot of a user's messages can also be resolved for the original channel, mod actions work etc.

@Mm2PL thoughts?

@jupjohn jupjohn marked this pull request as draft March 26, 2022 21:57
@jupjohn jupjohn marked this pull request as ready for review March 26, 2022 22:23
@jupjohn jupjohn requested a review from Felanbird April 5, 2022 07:14
@Felanbird
Copy link
Collaborator

While in debug mode the application crashes when the target channel is closed and you attempt to open the usercard in /mentions.

Normal mode does not crash you just receive a nice chatterino.ivr: Failed IVR API Call! 404 "" 🙂

@Felanbird
Copy link
Collaborator

Felanbird commented Apr 5, 2022

I was holding-off on reviewing this PR as I wasn't sure if I wanted the /mentions tab context to be replaced (for searching through multiple days of mentions from the same user), but I think it's fine for the active channel context to be used instead.

@jupjohn
Copy link
Contributor Author

jupjohn commented Apr 5, 2022

There should've been a nullptr check when trying to get the channel from a weak pointer - I wouldn't expect it to crash. Will look into it later today

@jupjohn
Copy link
Contributor Author

jupjohn commented Apr 6, 2022

@Felanbird should be resolved. If the channel wasn't found I was passing an empty channel but not checking that on the other side. Have tested and works.

Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

Crash is fixed - and the variation I was secretly hoping for exists.
When the tab is closed it seems to fallback to the pre-PR behavior which is great.

image
image

@pajlada pajlada enabled auto-merge (squash) April 9, 2022 11:17
@pajlada pajlada merged commit 04c355f into Chatterino:master Apr 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't access usercard from /mentions tab
4 participants