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

Add URL handling to chat #1667

Merged
merged 122 commits into from Feb 1, 2018

Conversation

3 participants
@FreezyLemon
Copy link
Member

commented Dec 7, 2017

Closes #806.

The old LinkFormatter class was mostly copied over and renamed to MessageFormatter.
It's implemented to format messages on ChatLine.Message's setter.

For the actual link sprite, I took inspiration in the OsuLinkSpriteText class that was in ProfileHeader. I extended it and let ChatLink inherit it, which does all the stuff regarding click events and tooltips in chat.
The click events are not complete since not everything is implemented yet. So at the moment, some links will give a notification saying exactly that (see OsuGame).
I added a new GetBeatmapRequest which gets a map by map ID (not set ID), so that I could get the set ID off of that.

I also added a new test case and fixed the old ChatDisplay test case since it would break without the necessary dependencies.
The new interface ICanDisableHoverSounds adds a bool which lets a Parent element decide on whether to play the hover sound or not. This is needed so that the hover sound is not played too often (every word is a sprite, so if a link is multiple words long it would play the hover sound every time you move between words). If the interface is not implemented however, it is just ignored and the sound is played regardless (this is done for ease of use and not to break existing implementations).

The change in PreviewButton is something I found in testing: Sometimes preview.Length can be 0 (i don't quite remember why this happens), and when it is, it breaks its own Width (NaN) and crashes.

FreezyLemon added some commits Dec 1, 2017

Moved LinkFlowContainer out of ProfileHeader to make it available for…
… other uses too (e.g. chat) and renamed it to LinkTextFlowContainer bc it can contain both links and text, not only one
Also moved LinkText to its own file so the chat could reuse it (Profi…
…leHeader's private class ProfileLink also still inherits from this, though)
Added new class for chat lines, that colour the messages after format…
…ting. URLs will become blue, and on hover (also defined here) be turned yellow-ish
Updated implementation to be based around a "LinkId" (atm the positio…
…n of the link, anything unique to a link inside its message will be fine), which does not allow matching (OnHover related) between different links
Added all files to the .csproj and also introduced basic action filte…
…ring when you set the URL on an OsuLinkSpriteText object
Made the Chat testcase include a beatmapsetoverlay so links can be cl…
…icked from in there. Also had to implement private DI to make it work
Added "getIdFromUrl" call back to content.action because performance …
…impact is small and no unnecessary id calculations are done
Added new OsuLinkSpriteText.TextColour property that sets the interna…
…l content (OsuHoverContainer)'s colour instead of the whole container, so that text colour is always changed through that (e.g. link colouring, link hover fade).

Implemented it to be used when adding text to an OsuLinkTextFlowContainer.
Made default link ID -1 (if no link is present) because linkId is cur…
…rently being set to link.index which can be 0.
Made it so the link ID is always added before loading the SpriteTexts…
… (fixed weird bug where some sprites would be white instead of blue). Also improved XML doc on TextColour

@peppy peppy force-pushed the FreezyLemon:url-parsing-support branch from a0c790d to b27577e Jan 17, 2018

@peppy

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

i still need to do a pass on code quality, but also found:

  • osump:// links don't seem to work (and aren't tested).
  • osu:// links don't seem to work (and aren't tested).
  • typing an editor link gobbles the portion after the dash

kapture 2018-01-17 at 20 35 18

@peppy

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

All aforementioned issues are now fixed. I'm going to do a second pass over the code as I'm still not confident about some of what is going on here.

Safe to say it's in a working and tested state now, though.

peppy added some commits Jan 30, 2018

@peppy

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

I think I'm happy with this one. @smoogipoo would you like the opportunity to review this before merging? I rewrote a good portion so if you feel the need to review then let me know and I'll wait for that to happen.

@smoogipoo

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

Yes, I would like to review this.

@peppy peppy requested a review from smoogipoo Jan 30, 2018

@peppy

peppy approved these changes Jan 30, 2018

@smoogipoo smoogipoo merged commit ad51095 into ppy:master Feb 1, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.