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

Add new experimental picture viewer #1829

Merged
merged 3 commits into from
Aug 11, 2022
Merged

Add new experimental picture viewer #1829

merged 3 commits into from
Aug 11, 2022

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Jul 9, 2022 •
edited
Loading

I started work on this around 2 months ago but didn't completely finish it yet. It already uses a feature flag which is disabled in releases so it won't hurt to merge it. I've made a new column in my board (https://github.com/jellyfin/jellyfin-androidtv/projects/15#column-19021853) that shows the remaining tasks.

The biggest feature is that it doesn't depend on the media manager anymore so it's sort of a part of the playback rewrite (#1057).

Changes

  • Add preference to enable new picture viewer
    • Like the playback rewrite; only shows in development builds
  • Add KeyHandler DSL to write readable key press handlers
  • Add new picture viewer
    • Supports portrait mode
    • Sort of supports touch
    • Supports slideshows
    • Shows controls to make it easy to start slideshows and navigate forward/backward

Issues

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 9, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/picture/PictureViewerViewModel.kt
parentId = itemResponse.albumId,
includeItemTypes = setOf(BaseItemKind.PHOTO),
fields = setOf(ItemFields.PRIMARY_IMAGE_ASPECT_RATIO),
sortBy = listOf(ItemFields.SORT_NAME.name), // TODO: Order should be consistent with the stdgridview the user comes from, which allows to change the order

Check notice

Code scanning

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.
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 9, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/picture/PictureViewerFragment.kt
.condition { !binding.actions.isVisible }
.body { pictureViewerViewModel.showPrevious() }

keyDown(KeyEvent.KEYCODE_MEDIA_SKIP_BACKWARD, KeyEvent.KEYCODE_MEDIA_PREVIOUS)

Check warning

Code scanning / Android Lint

Using inlined constants on older versions

Field requires API level 23 (current min is 21): android.view.KeyEvent#KEYCODE_MEDIA_SKIP_BACKWARD
app/src/main/java/org/jellyfin/androidtv/ui/picture/PictureViewerFragment.kt
.condition { !binding.actions.isVisible }
.body { pictureViewerViewModel.showNext() }

keyDown(KeyEvent.KEYCODE_MEDIA_SKIP_FORWARD, KeyEvent.KEYCODE_MEDIA_NEXT)

Check warning

Code scanning / Android Lint

Using inlined constants on older versions

Field requires API level 23 (current min is 21): android.view.KeyEvent#KEYCODE_MEDIA_SKIP_FORWARD
app/src/main/res/layout/fragment_picture_viewer.xml
app:layout_constraintStart_toStartOf="parent"
tools:visibility="visible">

<ImageButton

Check warning

Code scanning / Android Lint

Image without contentDescription

Missing contentDescription attribute on image
app/src/main/res/layout/fragment_picture_viewer.xml
android:layout_width="12dp"
android:layout_height="wrap_content" />

<ImageButton

Check warning

Code scanning / Android Lint

Image without contentDescription

Missing contentDescription attribute on image
app/src/main/res/layout/fragment_picture_viewer.xml
android:layout_width="12dp"
android:layout_height="wrap_content" />

<ImageButton

Check warning

Code scanning / Android Lint

Image without contentDescription

Missing contentDescription attribute on image
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jul 11, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jul 11, 2022
@nielsvanvelzen nielsvanvelzen linked an issue Jul 12, 2022 that may be closed by this pull request
Photo Slideshow #697
Closed
thornbill
thornbill reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

I'm not sure how perfect you are striving to get this for the first implementation, but I noticed a few issues:

  1. There seems to be an issue when changing photos (either manually or via the slideshow) with gifs. The image will change briefly then go back to displaying the previous image.
  2. In large photo libraries it seems like the current "position" is getting lost somehow. For example, I started playback on photo 301, click next, and a previous photo is displayed instead of photo 302.

It would probably be a good idea to show the photo name at least when the controls are visible (this would also help diagnose issue 2 further since I have many copies of the same photo in that album).

It might be good to show the "playback" controls by default to help with discoverability and hide them after a short time. I also didn't find pushing "up" to show the controls intuitive... I'm more accustomed to services using the "down" button to display them. That is more of a personal preference probably and there is some inconsistency among other services on that.

@nielsvanvelzen nielsvanvelzen added the next-release Pull request related to a future release which is not being worked on yet label Jul 18, 2022
@nielsvanvelzen
Copy link
Member Author

There seems to be an issue when changing photos (either manually or via the slideshow) with gifs. The image will change briefly then go back to displaying the previous image.

I haven't experienced this issue but I'd say it's out of scope for this initial PR.

In large photo libraries it seems like the current "position" is getting lost somehow. For example, I started playback on photo 301, click next, and a previous photo is displayed instead of photo 302.

I'll need to create a bigger album (biggest is about 40 items right now) to test this but I've already made an task for lazy loading which might fix this issue.

It would probably be a good idea to show the photo name at least when the controls are visible (this would also help diagnose issue 2 further since I have many copies of the same photo in that album).

Agreed, this was one of the things I wanted to add but not in this initial PR. This one too is already a task on the big list, it's not a feature in the current viewer either (except if you count that browse thingy that you can open while viewing).

It might be good to show the "playback" controls by default to help with discoverability and hide them after a short time. I also didn't find pushing "up" to show the controls intuitive... I'm more accustomed to services using the "down" button to display them. That is more of a personal preference probably and there is some inconsistency among other services on that.

Added support for KEYCODE_DPAD_DOWN to open the controls, I think opening the controls makes sense when the timer to auto-close it is added. I've updated the task to describe this behavior.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jul 22, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jul 23, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 23, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/picture/PictureViewerViewModel.kt
parentId = itemResponse.parentId,
includeItemTypes = setOf(BaseItemKind.PHOTO),
fields = setOf(ItemFields.PRIMARY_IMAGE_ASPECT_RATIO),
sortBy = listOf(ItemFields.SORT_NAME.name), // TODO: Order should be consistent with the stdgridview the user comes from, which allows to change the order

Check notice

Code scanning

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 this to the v0.15.0 milestone Jul 29, 2022
@nielsvanvelzen nielsvanvelzen removed the next-release Pull request related to a future release which is not being worked on yet label Jul 29, 2022
@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 11, 2022
@nielsvanvelzen nielsvanvelzen merged commit fa94dba into jellyfin:master Aug 11, 2022
@nielsvanvelzen nielsvanvelzen deleted the picture-viewer branch August 11, 2022 18:56
@nielsvanvelzen nielsvanvelzen added the release-highlight Pull request might be suitable for mentioning in the release blog post label Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornbill thornbill Awaiting requested review from thornbill

Assignees
No one assigned
Labels
enhancement New feature or request release-highlight Pull request might be suitable for mentioning in the release blog post
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

Photo Slideshow
3 participants
@nielsvanvelzen @thornbill @jellyfin-bot