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

Refactor preferences #260

Open
wants to merge 24 commits into
base: master
from

Conversation

@nielsvanvelzen
Copy link
Member

nielsvanvelzen commented Jan 9, 2020

Fixes #250

@nielsvanvelzen

This comment has been minimized.

Copy link
Member Author

nielsvanvelzen commented Jan 11, 2020

I'm considering this PR done. Ideally I wanted to store integers/longs not as string but that caused issues in the preference fragment (it only supports lists for strings...).

So for now this is it. The migration code will migrate the settings from the currently released app version, not the master branch.

@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review Jan 11, 2020
@thornbill

This comment has been minimized.

Copy link
Member

thornbill commented Jan 11, 2020

CI build is failing with the following error:

  Errors found:
  
  /home/vsts/work/1/s/app/src/main/java/org/jellyfin/androidtv/playback/ExternalPlayerActivity.java:68: Error: Overriding method should call super.onActivityResult [MissingSuperCall]
      protected void onActivityResult(int requestCode, int resultCode, Intent data) {
@anthonylavado anthonylavado requested a review from thornbill Jan 16, 2020
import org.jellyfin.androidtv.util.apiclient.AuthenticationHelper
import java.io.IOException

class UserPreferencesFragment : PreferenceFragmentCompat(), OnSharedPreferenceChangeListener {

This comment has been minimized.

Copy link
@thornbill

thornbill Jan 16, 2020

Member

Should we be using LeanbackPreferenceFragmentCompat?

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 17, 2020

Author Member

LeanbackPreferenceFragmentCompat unfortunately doesn't work, when I tried it previously it crashed the application. So for now I didn't change the ui much. I might do that in the future though.

This comment has been minimized.

Copy link
@thornbill

thornbill Jan 19, 2020

Member

I don’t like that this doesn’t match the app theme. Maybe we should extend the default theme you used to apply the Jellyfin colors as a temporary solution?

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 19, 2020

Author Member

I managed to get the leanback style preferences working. It now looks like this:

image

@thornbill thornbill added this to In progress in v0.12.0 Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.12.0
  
In progress
2 participants
You can’t perform that action at this time.