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

feat(DLNA): create DLNA settings #1759

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

LPkkjHD
Copy link
Contributor

@LPkkjHD LPkkjHD commented May 7, 2022

This PR tracks the progress of the implementation of the DLNA settings page. Required for this to be complete the following things have to be done:

  • create page
  • allow creation of DlnaProfiles (currently only saving profiles with a new name/saving server profiles creates a new custom one)
  • allow editing for DlnaProfiles (apart from setting configurations in arrays every other setting works)
    • Info
    • Identification
    • Subtitle
    • Direct Play
    • Transcoding
    • Containers
    • Codecs
    • Responses
    • XML
  • adding translation strings

For the visual design I tried sticking to the scheme which is defined by the pages of settings as well as the devices and api keys. With this card based approach I'm planning to unify the "devices" and "settings" pages which used to be separated by a drawer into a single one which should provide a better user experience.

Things I'm currently unsure/unsatisfied with

SelectedDlnaProfile's template is massive

Normally I'd split the template into multiple subcomponents, however, this approach doesn't seem very feasible here as the currentProfile holding the information we want to edit/bind to components cannot be passed around easily and working with $emit hooks doesn't seem very elegant to me.

API interaction is a mess

Currently every component I wrote calls the API and processes the data as it pleases. I think this should be centralized somewhere, but I'm uncertain of the way to do this properly.

DLNA settings are visually weird

As it stands the DLNA settings (the ones on the left side) change the mouse to a weird pointer (and would highlight an entry if clicked) which is undesirable in my opinion. What I was trying to achieve was an effect similar to the one where the server information is displayed (light up on mouse hover, however, not clickable). Also the text boxes aren't aligned properly. Maybe there is another component from Vuetify which makes more sense here.

Undocumented deprecation and removal of X-DLNA cap and X-DLNA doc

Those two flags can still be set by the stable client of jellyfin-web, however, they don't appear in any api documentation and setting them doesn't actually result in anything (the server doesn't process them). I dropped them as a result.

@jellyfin-bot jellyfin-bot added the vue Pull requests that edit or add Vue files label May 7, 2022
<v-tabs-items v-model="tab">
<v-tab-item value="tab-1">
<v-list>
<!-- General -->
Copy link
Member

Choose a reason for hiding this comment

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

As you say, this file is massive. However, why it can't be v-for looped? You can loop over currentProfile with Object.entries like (key, value) in Object.entries(currentProfile) and bind the value to v-model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently working on this, however, the way you suggested is not feasible as we would modify data of the loop within it. (v-modeling value results in this eslint warning being thrownhttps://eslint.vuejs.org/rules/valid-v-model.html)

Copy link
Member

Choose a reason for hiding this comment

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

Try ignoring the rule, might be a false positive but work perfectly during runtime.

frontend/components/System/SelectedDlnaProfile.vue Outdated Show resolved Hide resolved
frontend/pages/settings/dlna.vue Outdated Show resolved Hide resolved
</v-list-item-content>
<v-list-item-action>
<v-switch
v-if="typeof dlnaSettings[dlnaSettingName] === 'boolean'"
Copy link
Member

Choose a reason for hiding this comment

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

Why we need typeof here? Do we know if the API can return something else that it's not a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need typeof here as we want to display different control elements (Switches for boolean values, Textfields for strings, Selectors for Enum based values) based on which type the data has we want to control.

This seemed the best approach from my perspective as it allows us to loop through all relevant data without needing to care which exact control we want to display for each field.

frontend/components/System/SelectedDlnaProfile.vue Outdated Show resolved Hide resolved
frontend/pages/settings/dlna.vue Outdated Show resolved Hide resolved
@ferferga
Copy link
Member

ferferga commented May 7, 2022

DLNA itself might be moved to a plugin, this is why the API might not be so much love. Still a good idea to have this page, just I would say not to worry so much about it looking extremely pretty, just good enough is good I think.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

frontend/locales/en-US.json Outdated Show resolved Hide resolved
feat: add snackbar for api call feedback

chore: remove unused translation keys
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

Comment on lines +233 to +237
this.selectedProfile = (
await this.$api.dlna.getProfile({
profileId: selectedDevice.Id as string
})
).data;

Check warning

Code scanning / CodeQL

Useless assignment to property

This write to property 'selectedProfile' is useless, since [another property write](1) always overrides it.
@LPkkjHD
Copy link
Contributor Author

LPkkjHD commented Jun 22, 2022

I think that's enough tinkering around for the moment. As I hit a wall and thought of a better way to implement this I'll do a rewrite from scratch in the next couple of days.

@ferferga ferferga force-pushed the master branch 12 times, most recently from bcf0f7b to 41c1745 Compare November 15, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Something has merge conflicts vue Pull requests that edit or add Vue files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants