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

Rewrite navigation #2132

Merged
merged 7 commits into from
Oct 7, 2022
Merged

Rewrite navigation #2132

merged 7 commits into from
Oct 7, 2022

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Oct 1, 2022 •
edited
Loading

After years of work we're finally merging (most) activities into a single activity with proper navigation code.

Some parts are not migrated to the new navigation code:

  • Picture viewer - will be migrated when fully replaced with new picture viewer
  • Video player & Next Up - will be part of the playback rewrite
  • Authentication - not sure how to migrate yet, the existing code for it works fine so it may just stay as-is

Some future changes include:

  • Re-enable display mirroring in the SocketListener
  • Update PlaybackLauncher to return destination instead of a "playbackActivityClass"

This PR introduces a major change to the code and it has some known issues:

  • Fragments can crash the application if not using lifecycle scope
    • This happens for all legacy apiclient calls when they complete after the fragment is closed
  • The back stack from the FragmentManager is not used, this forces fragments to refresh when going back
    • I'm investigating this issue before marking the PR as ready

Changes

  • Merge most activities into MainActivity
  • Add NavigationRepository which can be used to navigate the MainActivity
  • Add Destination(s) used for navigating
  • Add HomeFragment that combines HomeRowsFragment (previously HomeFragment) and HomeToolbarFragment
  • Convert SearchActivity to SearchFragment
  • Implement display mirroring

Issues

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Oct 1, 2022
@nielsvanvelzen nielsvanvelzen added this to the v0.15.0 milestone Oct 1, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 1, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/navigation/Destinations.kt
import org.jellyfin.sdk.model.api.SeriesTimerInfoDto
import java.util.UUID

object Destinations {

Check warning

Code scanning / detekt

Too many functions inside a/an file/class/object/interface always indicate a violation of the single responsibility principle. Maybe the file/class/object/interface wants to manage too many things at once. Extract functionality which clearly belongs together.

Object 'Destinations' with '15' functions detected. Defined threshold inside objects is set to '11'
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeRowsFragment.kt
private val nowPlaying by lazy { HomeFragmentNowPlayingRow(mediaManager) }
private val liveTVRow by lazy { HomeFragmentLiveTVRow(requireActivity(), userRepository, navigationRepository) }

override fun onCreate(savedInstanceState: Bundle?) {

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand.

The function onCreate is too long (67). The maximum length is 60.
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeRowsFragment.kt
private val nowPlaying by lazy { HomeFragmentNowPlayingRow(mediaManager) }
private val liveTVRow by lazy { HomeFragmentLiveTVRow(requireActivity(), userRepository, navigationRepository) }

override fun onCreate(savedInstanceState: Bundle?) {

Check warning

Code scanning / detekt

Prefer splitting up complex methods into smaller, easier to understand methods.

The function onCreate appears to be too complex (23). Defined complexity threshold for methods is set to '15'
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeRowsFragment.kt
super.onResume()

//React to deletion
if (activity != null && !requireActivity().isFinishing && currentRow != null && currentItem != null && currentItem!!.getItemId() != null && currentItem!!.getItemId().equals(dataRefreshService.lastDeletedItemId)) {

Check warning

Code scanning / detekt

Complex conditions should be simplified and extracted into well-named methods if necessary.

This condition is too complex (6). Defined complexity threshold for conditions is set to '4'
app/src/main/java/org/jellyfin/androidtv/ui/navigation/Destinations.kt Outdated
Extras.Folder to Json.Default.encodeToString(item),
)

fun displayPreferences(displayPreferencesId: String, allowViewSelection: Boolean) = preferenceDestination<DisplayPreferencesScreen>(

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style.

Line detected, which is longer than the defined maximum line length in the code style.
app/src/main/java/org/jellyfin/androidtv/ui/search/SearchFragment.kt
SpeechRecognizer.isRecognitionAvailable(this)
&& ContextCompat.checkSelfPermission(this, Manifest.permission.RECORD_AUDIO) != PackageManager.PERMISSION_DENIED
SpeechRecognizer.isRecognitionAvailable(requireContext())
&& ContextCompat.checkSelfPermission(requireContext(), Manifest.permission.RECORD_AUDIO) != PackageManager.PERMISSION_DENIED

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style.

Line detected, which is longer than the defined maximum line length in the code style.
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeRowsFragment.kt
super.onResume()

//React to deletion
if (activity != null && !requireActivity().isFinishing && currentRow != null && currentItem != null && currentItem!!.getItemId() != null && currentItem!!.getItemId().equals(dataRefreshService.lastDeletedItemId)) {

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style.

Line detected, which is longer than the defined maximum line length in the code style.
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeRowsFragment.kt

private fun refreshCurrentItem() {
currentItem?.let { item ->
if (item.getBaseItemType() == BaseItemKind.USER_VIEW || item.getBaseItemType() == BaseItemKind.COLLECTION_FOLDER) return

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style.

Line detected, which is longer than the defined maximum line length in the code style.
@nielsvanvelzen nielsvanvelzen added the refactor Improvements to code realiability, readability and quality label Oct 1, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 1, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
}

public static void createUserViewIntent(final org.jellyfin.sdk.model.api.BaseItemDto baseItem, final Context context, final Consumer<Intent> callback) {
public static void launchUserView(final org.jellyfin.sdk.model.api.BaseItemDto baseItem) {

Check notice

Code scanning / Android Lint

Unknown nullness

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
}

public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity, final boolean noHistory) {
public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity) {

Check notice

Code scanning / Android Lint

Unknown nullness

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
}

public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity, final boolean noHistory) {
public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity) {

Check notice

Code scanning / Android Lint

Unknown nullness

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
}

public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity, final boolean noHistory) {
public static void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Activity activity) {

Check notice

Code scanning / Android Lint

Unknown nullness

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
@nielsvanvelzen nielsvanvelzen force-pushed the navigation branch 2 times, most recently from 657d025 to c959355 Compare October 3, 2022 19:41
@nielsvanvelzen
Copy link
Member Author

I've fixed all issues I could find except for the lifecycle thing (will be done in separate future pull requests). There might still be issues I haven't found myself, it's quite a big change after all.

@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review October 3, 2022 20:03
@nielsvanvelzen nielsvanvelzen merged commit 0a372fe into jellyfin:master Oct 7, 2022
@nielsvanvelzen nielsvanvelzen deleted the navigation branch October 7, 2022 06:06
@nielsvanvelzen nielsvanvelzen mentioned this pull request Oct 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornbill thornbill thornbill approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request refactor Improvements to code realiability, readability and quality
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nielsvanvelzen @thornbill