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

add button to MainPlayer to go to Detail view #2430

Closed
wants to merge 4 commits into from

Conversation

csicar
Copy link

@csicar csicar commented Jun 28, 2019

Changes

  • Added a <- button to the top of the MainVideoPlayer

Reason

The reason this button is useful is, that when opening videos via a link (and having selected "Open Video directly"), you can see the comments and related videos.

Example

example

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I think the code is good, since it has the same structure as the code around it. The style inconsistencies I pointed out are nothing important :-)
Thank you for this feature! 😄

app/src/main/res/layout/activity_main_player.xml Outdated Show resolved Hide resolved
@csicar
Copy link
Author

csicar commented Jun 30, 2019

Requested changes have been implemented. Is there something else missing?

@Stypox
Copy link
Member

Stypox commented Jun 30, 2019

Please wait for a maintainer to review your changes. This could take a while since they are often busy in real life and there are more important and complex PRs on the way to be merged.

@AdKiller
Copy link

AdKiller commented Jul 5, 2019

Always wanted this feature. thank you very much for implementing it!

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Code looks good 👍 Will test it later.

@theScrabi theScrabi added the feature request Issue is related to a feature in the app label Jul 21, 2019
@Phoenix616
Copy link

I feel like making the button look like a back button kinda breaks expectations: Back implies that you actually go back to the previous screen (like with the device back button), not that you always go to the info page.

I feel like a design that clearly shows the info page (e.g. an i in a circle) and a different placement would solve that.

@Stypox
Copy link
Member

Stypox commented Aug 17, 2019

@Phoenix616 you are right, an info button would be better. This expectation-breaking already exists with other back button: #2358. The proposed fix is basically the same as yours: #2358 (comment).
@csicar would you mind changing the back button to some type of info button?

@csicar
Copy link
Author

csicar commented Aug 17, 2019

The bahaviour oft the back butto is how Android UIs are intended to behave (as intended by the ui guidelines):

  • the Back Button in the toolbar of an app should navigate hierachically upwards
  • the android's system back button navigates chronologically backwards.

Reference: https://material.io/design/navigation/understanding-navigation.html#reverse-navigation

I personally perfer to think of the Button as navigating upwards, but I can understand the Argument. Maybe the down arrow from YouTube's app could be a compromise?

If not, i'll be happy to implement to info button.

@Stypox
Copy link
Member

Stypox commented Aug 17, 2019

For me the back button is ok, then 😉

@Phoenix616
Copy link

Interesting that they would suggest such a confusing way of handling things. (Duplicate back buttons with different functionality) I feel like the down arrow would be a lot better and more intuitive. (Especially for people coming from the original app ;))

@csicar
Copy link
Author

csicar commented Aug 17, 2019

Example of how it looks with the "expand" button:
2019-08-17-214332_398x582_scrot

patch.txt

@csicar
Copy link
Author

csicar commented Aug 17, 2019

@Phoenix616 I guess having very similar symbols for the different actions doesn't help :)

@TobiGr
Copy link
Contributor

TobiGr commented Aug 17, 2019

I am completely fine with the behaviour and icons you used in the example video 👍

@Phoenix616
Copy link

Phoenix616 commented Aug 21, 2019

I realise that the downwards arrow causes another usability problem: The arrow at the right side looks the same but it is used to expand the view of available video options instead.

So a backwards arrow like the Android docs suggest might actually be the better option here :S (unless a different icon can be found for either going to the info view or expanding the option view I guess)

@TobiGr
Copy link
Contributor

TobiGr commented Oct 1, 2019

@Stypox is there anything which prevents this PR from being merged?

@Stypox
Copy link
Member

Stypox commented Oct 1, 2019

@TobiGr no, this is ready, I forgot to approve changes at last ;-)

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

I got the following exception with autoplay enabled and watching three videos (so the initial video was not opened, but another one).

Exception

  • User Action: ui error
  • Request: App crash, UI failure
  • Content Language: GB
  • Service: none
  • Version: 0.17.3
  • OS: Linux ,release-keys 6.0 - 23
Crash log

java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.res.Resources android.view.View.getResources()' on a null object reference
	at org.schabi.newpipe.util.AnimationUtils.slideUp(AnimationUtils.java:372)
	at org.schabi.newpipe.fragments.list.comments.CommentsFragment.handleResult(CommentsFragment.java:96)
	at org.schabi.newpipe.fragments.list.comments.CommentsFragment.handleResult(CommentsFragment.java:25)
	at org.schabi.newpipe.fragments.list.BaseListInfoFragment.lambda$startLoading$0(BaseListInfoFragment.java:120)
	at org.schabi.newpipe.fragments.list.-$$Lambda$BaseListInfoFragment$prYfqK19mXhZc0JDsOoy9kGzYpE.accept(lambda)
	at io.reactivex.internal.observers.ConsumerSingleObserver.onSuccess(ConsumerSingleObserver.java:62)
	at io.reactivex.internal.operators.single.SingleObserveOn$ObserveOnSingleObserver.run(SingleObserveOn.java:81)
	at io.reactivex.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:119)
	at android.os.Handler.handleCallback(Handler.java:746)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:148)
	at android.app.ActivityThread.main(ActivityThread.java:5443)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:728)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:618)


@Stypox Stypox linked an issue Mar 9, 2020 that may be closed by this pull request
@Stypox
Copy link
Member

Stypox commented Mar 9, 2020

@csicar could you check the bug report from TobiGr?

@Stypox
Copy link
Member

Stypox commented Aug 4, 2020

Closing as this isn't needed anymore after #2907

@Stypox Stypox closed this Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open video info from a playing video
6 participants