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.

Sign up for GitHub

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

Jump to bottom

Remember last opened chat tab #3205

Merged
merged 6 commits into from
Jun 20, 2024
Merged

Remember last opened chat tab #3205

merged 6 commits into from
Jun 20, 2024

Conversation

Marc-Spector
Copy link
Collaborator

Since the main tabs are now recreated with each click, the last opened chat tab of the channel or player is no longer remembered.
This pull request will help bring this feature back.

Sheikah45
Sheikah45 reviewed Jun 17, 2024
View reviewed changes
Copy link
Member

@Sheikah45 Sheikah45 left a comment

Choose a reason for hiding this comment

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

At a high level this makes sense.

I just wonder if we could just use a map or something in Chat navigation instead of a list of a new class.

src/main/java/com/faforever/client/chat/ChatNavigation.java Outdated Show resolved Hide resolved
@Marc-Spector
Copy link
Collaborator Author

At a high level this makes sense.

I just wonder if we could just use a map or something in Chat navigation instead of a list of a new class.

When creating the ChatTab class I assumed that in the future there would be other variables that could be added. For example, lastVisibleMessageIndex variable if the user scrolls up and goes to another main tab and then returns to the chat in the same place. This is just a rough example.

@Sheikah45
Copy link
Member

Fair I suppose. Although the last visible message is stored in the Chat channel object now just as FYI

@Marc-Spector Marc-Spector marked this pull request as ready for review June 18, 2024 09:08
@Marc-Spector Marc-Spector marked this pull request as draft June 18, 2024 09:11
@Marc-Spector Marc-Spector marked this pull request as ready for review June 18, 2024 09:51
Sheikah45
Sheikah45 requested changes Jun 18, 2024
View reviewed changes
src/main/resources/theme/chat/chat.fxml Outdated
@@ -14,7 +14,7 @@
<children>
<TabPane fx:id="tabPane" tabClosingPolicy="ALL_TABS" AnchorPane.bottomAnchor="0.0" AnchorPane.leftAnchor="0.0"
AnchorPane.rightAnchor="0.0" AnchorPane.topAnchor="0.0" visible="false">
<Tab text="+" closable="false">
<Tab fx:id="addChannelTab" text="+" closable="false">
Copy link
Member

Choose a reason for hiding this comment

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

The fx:id value actually gets set as the id for the node. So you shouldn't need to set the id in the controller itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh! I'll know, thanks. But we don't need the id anymore. Found a more elegant solution using isClosable condition

@Sheikah45 Sheikah45 enabled auto-merge (squash) June 20, 2024 22:06
@Sheikah45 Sheikah45 merged commit 7c903c8 into develop Jun 20, 2024
3 checks passed
@Sheikah45 Sheikah45 deleted the feature/rememberChatTab branch June 20, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sheikah45 Sheikah45 Sheikah45 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Marc-Spector @Sheikah45