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

Sort list of external players #4036

Merged
merged 4 commits into from
Sep 12, 2023
Merged

Sort list of external players #4036

merged 4 commits into from
Sep 12, 2023

Conversation

trostboot
Copy link
Contributor

@trostboot trostboot commented Sep 11, 2023 •
edited by kommunarr
Loading

Sort list of external players

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

As more players get added, the list should be sorted for better UX. This PR sorts the elements before display and keeps the default entry for no external player at the top.

Screenshots

{4A13AAC2-618E-41ED-B76D-6BE209D6D40C}

Testing

Has been checked by opening the settings and it works as designed.

Desktop

  • OS: Windows
  • OS Version: 11
  • FreeTube version: v0.19.0

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 11, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 11, 2023 16:14
@trostboot
Copy link
Contributor Author

Actually I just realized, this would also potentially break the setting for a user anytime a new player is added, so the way the setting is saved needs to be reworked. I'll turn this into a draft for now.

@trostboot trostboot marked this pull request as draft September 11, 2023 16:55
auto-merge was automatically disabled September 11, 2023 16:55

Pull request was converted to draft

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 11, 2023
absidue
absidue reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@absidue absidue 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 leaving None in the list while sorting and then correcting the position afterwards, removing it from the list, sorting and then adding it back might save you a bit of code.

absidue
absidue reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Also to answer your comment about it breaking the setting, it's because you are only changing the order of the names but not reordering the values to match

so instead of it being vlc -> vlc, it might now be smplayer -> vlc

src/renderer/components/external-player-settings/external-player-settings.js Outdated Show resolved Hide resolved
@absidue
Copy link
Member

absidue commented Sep 11, 2023

If you do the sorting in src/renderer/store/modules/utils.js in getExternalPlayerCmdArgumentsData instead, before it gets split up into invididual names and values arrays, you won't have the problem with the setting breaking.

@trostboot
Copy link
Contributor Author

All excellent suggestions, thank you. I had already realized the value issue, but doing it in utils.js instead was clearly the better choice to begin with.
It now works as it is, but the code is a bit ugly, I'm sure there's a nicer way to do it. I'll sleep on it and go over it again tomorrow.

@trostboot trostboot marked this pull request as ready for review September 12, 2023 06:50
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 12, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 12, 2023 06:51
kommunarr
kommunarr approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

LGTM!

PikachuEXE
PikachuEXE approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Confirmed option values matching labels
image

@FreeTubeBot FreeTubeBot merged commit b4223e5 into FreeTubeApp:development Sep 12, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 12, 2023
@trostboot trostboot deleted the ext-player-sort branch September 12, 2023 18:56
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Sep 13, 2023
@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05
934b584 
* development:
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Polish)
  Translated using Weblate (Czech)
  Translated using Weblate (Turkish)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Fix localization of counts (FreeTubeApp#4011)
  Sort list of external players (FreeTubeApp#4036)
  Translated using Weblate (French)
  Translated using Weblate (Icelandic)
  Translated using Weblate (Estonian)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Italian)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommunarr kommunarr kommunarr approved these changes

@absidue absidue absidue approved these changes

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc Awaiting requested review from efb4f5ff-1298-471a-8973-3d47447115dc

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

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

Successfully merging this pull request may close these issues.
5 participants
@trostboot @absidue @PikachuEXE @kommunarr @FreeTubeBot