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

Keep map selection when searching #3132

Merged

Conversation

obydog002
Copy link
Collaborator

Fix #3092

Now retains the selected option

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e3608b2) 59.37% compared to head (3f05b23) 59.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #3132   +/-   ##
==========================================
  Coverage      59.37%   59.37%           
+ Complexity      4506     4505    -1     
==========================================
  Files            574      574           
  Lines          20535    20539    +4     
  Branches        1019     1019           
==========================================
+ Hits           12193    12196    +3     
+ Misses          7803     7802    -1     
- Partials         539      541    +2     
Files Coverage Δ
...om/faforever/client/game/CreateGameController.java 71.67% <80.00%> (+1.08%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

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.

Instead of introducing a mutable variable that we have to manually remember to set and track, I think it would be better to use the biconsumer version of subscribe on the selectedItem property in initMapSelection. This provides the new and old selected values, then if the new value is null we can select the old value if it is contained by filtered maps.

Effectively moving that logic outside of the setSelectedMap function.

Which is better for the long term as well since most of those properties set in setSelected map will be changed to be bound to derived properties eventually.

Sheikah45
Sheikah45 previously approved these changes Feb 21, 2024
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.

The old item null check is superfluous but that is fine

@Sheikah45 Sheikah45 merged commit a5aca5d into FAForever:develop Feb 22, 2024
4 checks passed
@obydog002 obydog002 deleted the bug_fix/#3092_map_search_keeps_map branch July 18, 2024 14:45
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.

Typing in map search clears map selection and name preview
2 participants