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

Integrate new chat into game #18377

Merged
merged 15 commits into from
May 26, 2022
Merged

Integrate new chat into game #18377

merged 15 commits into from
May 26, 2022

Conversation

jai-x
Copy link
Member

@jai-x jai-x commented May 23, 2022

Reference design: https://www.figma.com/file/f8b2dHp9LJCMOqYP4mdrPZ/

2022-05-23_22-31-09.mp4

Integrates ChatOverlayV2 into game, matching existing functionality and passing existing tests

Manual flows tested:

  • Joining channels
  • Leaving channels
  • Chatting in public channels
  • Chatting in DM channels
  • Opening DM channel from user overlay
  • Opening a channel from a mention notification
  • Opening DM channel from a DM notification

Features present in design but not in this PR:

  • Mention count appearing in the channel list
  • Unread state appearing in the channel list
  • Filtering channel list visibility by type
  • Font resizing
  • Search
  • Overlay minimised display

@Joehuu
Copy link
Member

Joehuu commented May 24, 2022

I added some issues that the redesign resolves, but I'm not sure about these behavioral ones:

I think they are fixed, as the channel selector is now in the same place as chat.


I haven't been following the implementations, but there are some parts that I think should be different:

  • the "Add more channels" should be a simple + because it blends in with other text
  • it defaults to the channel selector instead of the first tab after every relaunch
  • "osu!chat" should just be "chat" (to match toolbar title, notably)

@jai-x
Copy link
Member Author

jai-x commented May 24, 2022

  • the "Add more channels" should be a simple + because it blends in with other text

At the moment, I've just implemented as is specified in the design, which may change and evolve from usability feedback.
I do notice that the design has already been updated with new "+" icons but I'm unsure exactly how they are supposed to function.
I have asked about them in a Figma comment.

  • it defaults to the channel selector instead of the first tab after every relaunch

This is because the overlay binds and sets the current channel to a non-null value before the channel manager can poll and join the default channels. Oversight, will fix.

  • "osu!chat" should just be "chat" (to match toolbar title, notably)

This is just placeholder design, as the top bar will have channel filters added in subsequent PR as per design.

@pull-request-size pull-request-size bot added size/L and removed size/M labels May 24, 2022
@peppy
Copy link
Member

peppy commented May 25, 2022

I haven't investigated the cause in detail yet, but on a release build this doesn't load any channels for me:

osu! 2022-05-25 at 08 49 31

I guess the live release doesn't either...

@jai-x
Copy link
Member Author

jai-x commented May 25, 2022

I'm able to connect to chat fine on the latest release, master, and on this branch. Seems like very weird failure :/

@peppy
Copy link
Member

peppy commented May 25, 2022

It's a non-issue, I've found the cause and will fix separately.


private void cycleChannel()
{
List<Channel> overlayChannels = filterChannels(channelManager.JoinedChannels).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

This isn't working as expected. The order here is not the visual order and therefore results in weird traversal:

osu.2022-05-25.at.09.43.37.mp4

Also, the channel scroll view should be re-centering itself after selecting a new channel via keyboard. You can call ScrollIntoView on the drawable to make this happen.

currentChannel.Value = overlayChannels[(currentIndex + direction + overlayChannels.Count) % overlayChannels.Count];
}

private IEnumerable<Channel> filterToChatChannels(IEnumerable channels) => channels.Cast<Channel>().Where(c => c.Type != ChannelType.System);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this change correct? It implies that channels like multiplayer channels should show up in the overlay?

Copy link
Member

Choose a reason for hiding this comment

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

probably not, but should show that the function is not clear. i was changing it to fix traversal and things like announce channels not being considered.

it should be written to exclude rather than include, i think.

osu.Game/Overlays/ChatOverlayV2.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/Chat/ChannelList/ChannelList.cs Outdated Show resolved Hide resolved
@peppy
Copy link
Member

peppy commented May 26, 2022

For whatever it's worth, announcements now show up in this display:

osu! 2022-05-26 at 09 41 25

A further step (separate PR) would give them a heading, but also only show that section if any announcement channels are actually present.

peppy
peppy previously approved these changes May 26, 2022
@jai-x
Copy link
Member Author

jai-x commented May 26, 2022

Yeah this was intended as, since the channel filter method changed, I decided that the top "CHANNELS" section would be the catch all for all types of channel except for DMs. Will improve in a follow up PR 👍

@peppy peppy disabled auto-merge May 26, 2022 12:02
@peppy peppy merged commit f71f761 into ppy:master May 26, 2022
@jai-x jai-x deleted the new-chat-integrate branch May 26, 2022 14:25
This was referenced May 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants