-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
base: master
Are you sure you want to change the base?
Conversation
<v-tabs-items v-model="tab"> | ||
<v-tab-item value="tab-1"> | ||
<v-list> | ||
<!-- General --> |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
</v-list-item-content> | ||
<v-list-item-action> | ||
<v-switch | ||
v-if="typeof dlnaSettings[dlnaSettingName] === 'boolean'" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
fix: remove deprecated BlastAliveMessageIntervalSeconds from config
feat: add snackbar for api call feedback chore: remove unused translation keys
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
this.selectedProfile = ( | ||
await this.$api.dlna.getProfile({ | ||
profileId: selectedDevice.Id as string | ||
}) | ||
).data; |
Check warning
Code scanning / CodeQL
Useless assignment to property
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. |
bcf0f7b
to
41c1745
Compare
acb6ecf
to
5d6a7c8
Compare
c360429
to
f3260d9
Compare
fe041a3
to
3fbf550
Compare
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:
For the visual design I tried sticking to the scheme which is defined by the pages of
settings
as well as thedevices
andapi 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 massiveNormally 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.