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 Smart live replays #2529

Merged
merged 29 commits into from Feb 5, 2022
Merged

Add Smart live replays #2529

merged 29 commits into from Feb 5, 2022

Conversation

Marc-Spector
Copy link
Collaborator

@Marc-Spector Marc-Spector commented Jan 20, 2022

Closes #82
Closes #2525
Marks #1959 as outdated because the Live Replayes table interaction is disabled (no selection)

@Marc-Spector Marc-Spector marked this pull request as draft January 20, 2022 13:15
@Marc-Spector

This comment has been minimized.

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #2529 (1c4be75) into develop (a8dafe1) will increase coverage by 0.34%.
The diff coverage is 78.36%.

@@              Coverage Diff              @@
##             develop    #2529      +/-   ##
=============================================
+ Coverage      64.03%   64.38%   +0.34%     
- Complexity      4509     4582      +73     
=============================================
  Files            497      505       +8     
  Lines          19383    19481      +98     
  Branches        1114     1124      +10     
=============================================
+ Hits           12412    12542     +130     
+ Misses          6346     6304      -42     
- Partials         625      635      +10     
Impacted Files Coverage Δ
.../main/java/com/faforever/client/fx/JavaFxUtil.java 67.48% <ø> (+0.34%) ⬆️
...r/client/leaderboard/SubDivisionTabController.java 60.52% <0.00%> (ø)
...ava/com/faforever/client/replay/ReplayService.java 83.50% <ø> (+8.39%) ⬆️
...ver/client/ui/table/NoSelectionModelTableView.java 28.57% <28.57%> (ø)
...ever/client/fx/contextmenu/ContextMenuBuilder.java 91.42% <62.50%> (-5.13%) ⬇️
...om/faforever/client/replay/TrackingLiveReplay.java 66.66% <66.66%> (ø)
...com/faforever/client/replay/LiveReplayService.java 75.28% <75.28%> (ø)
...ver/client/vault/replay/WatchButtonController.java 86.53% <85.71%> (-2.10%) ⬇️
...t/fx/contextmenu/CancelActionNotifyMeMenuItem.java 90.00% <90.00%> (ø)
...menu/CancelActionRunReplayImmediatelyMenuItem.java 90.00% <90.00%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8dafe1...1c4be75. Read the comment docs.

@Marc-Spector Marc-Spector marked this pull request as ready for review January 22, 2022 01:25
@Marc-Spector Marc-Spector changed the title WIP: Smart live replays Smart live replays Jan 22, 2022
@Sheikah45
Copy link
Member

I don't understand why there are two separate buttons. Would it be possible just to have the one button? I think this is applicable for all watch buttons so see no harm in integrating it.

@Marc-Spector
Copy link
Collaborator Author

I do not understand you. Are you suggesting using the MenuButton class instead of the SplitMenuButton class?

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.

Yeah I don't think there is a reason to have two separate buttons

@Sheikah45
Copy link
Member

image
Here the buttons seem to always be either or. SO only one is enabled at a time. That doesn't make much sense. It would be better to have only one button. And have the onaction slightly different depending on if it is watchable or not.

@Marc-Spector
Copy link
Collaborator Author

Marc-Spector commented Jan 22, 2022

Eh, redo it again. I will continue working on the PR after #2513 is merged only. I need ContextMenuBuilder to work with PR

@Sheikah45
Copy link
Member

Untitled
Remove the added button and just put all the functionality into the main button

@Marc-Spector Marc-Spector marked this pull request as draft January 28, 2022 14:18
@Sheikah45 Sheikah45 linked an issue Jan 29, 2022 that may be closed by this pull request
@Marc-Spector Marc-Spector marked this pull request as ready for review January 30, 2022 01:52
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.

Just two minor things

@Sheikah45
Copy link
Member

Other small note. The accent color for the button should be blue like the rest of the client

This green doesn't match the theme
image

Sheikah45
Sheikah45 previously approved these changes Jan 31, 2022
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.

Will merge this after the release just so that it can have a chance to be tested in the next alpha

@Sheikah45 Sheikah45 changed the title Smart live replays Add Smart live replays Feb 5, 2022
@Sheikah45 Sheikah45 merged commit 185be83 into develop Feb 5, 2022
@Sheikah45 Sheikah45 deleted the feature/smart-live-replays branch February 5, 2022 15:16
mrchris2000 pushed a commit to mrchris2000/downlords-faf-client that referenced this pull request Apr 15, 2022
Co-authored-by: Ivan <v23620@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve live replays tab background blue bar interruption Smart live replays
3 participants