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

VideoPlayer: drop unmaintained code in favour of fixes #10706

Merged
merged 1 commit into from Oct 15, 2016

Conversation

@FernetMenta
Copy link
Member

commented Oct 14, 2016

this code is obsolete, we have additive seek

we don't need seek functions all over the place.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2016

I consider this a fix because this is pre-requisite for fixing a bug related to seek.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2016

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

any objections?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

nope

@FernetMenta FernetMenta merged commit 887c85f into xbmc:master Oct 15, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@FernetMenta FernetMenta deleted the FernetMenta:dead branch Oct 15, 2016
@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

Please correct me if I'm wrong, now there is no a way to seek to specific time with digits on remote/keyboard?
hehe.. already merged while I'm writing comment

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

If this was the function of this code, yes. If this is still a desired feature, it has to be implemented in CSeekHandler. Otherwise the code is not maintainable.

@jjd-uk

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

After answering http://forum.kodi.tv/showthread.php?tid=294731 it's just been pointed out to me that this PR has caused http://kodi.wiki/view/skip_steps#Time_skipping to no longer work, seems there were users still using it as it allowed to jump to a precise moment in time.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2016

I didn't even know what this code did when I created this pr. I was fixing a bug related to seeking and noticed that we had seek handler and this very old code, obviously not used by many and not migrated to new seek handler.
Sometimes we have to leave things behind that are not maintained.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

Why "obviously not used by many"? I miss this feature, it's very useful while testing. I know other users on the forum have noticed it is now missing, and of course they're just the users testing nightlies. While it may have been old code, it still worked and shouldn't have been removed without plans to migrate it before the release of Kodi 17 (IMHO).

@jjd-uk

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Perhaps it was old untouched code because it simply worked and since this in not in our Beta!s yet then yes this removal would have gone unnoticed. I know there's certainly one than that one user who will miss this.

@jjd-uk

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Btw your PR title was not at all helpful as I completely missed this removal of seek functionality.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2016

It was unmaintained code is it did harm fixing bugs in maintained code. Feel free to reimplement or suggest users to stay with v16 if they think this was the most important feature ever.
You can't expect to me to migrate unmaintained and decayed code that was left forgotten when seekhandler and additive seek was implemented, do you? Do you want to to revert all my recent fixed and get this old code back?

@ronie

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

not wanting to be a part of this discussion, but i'm seeing a trend lately where pr's are being submitted and are merged within a few hours without any code review by a secondary member.

perhaps we can improve this a bit?
no idea who would qualify to review video related code... and i wouldn't be surprised if there is no one due to lack of team devs.
but we can at least keep pr's open for a few days, so everyone has a change to leave feedback i think?

@jjd-uk

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

@xhaggi was the direct seeking functionality deliberately left out of seekhandler? or was it simple missed?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2016

please move further discussion to internal forum.

@FernetMenta FernetMenta changed the title VideoPlayer: drop dead code VideoPlayer: drop unmaintained code infavour of fixes Oct 24, 2016
@FernetMenta FernetMenta changed the title VideoPlayer: drop unmaintained code infavour of fixes VideoPlayer: drop unmaintained code in favour of fixes Oct 24, 2016
@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

We should revert this PR because it removes a feature in a stage where we shouldn't. I left this code untouched while working on the adaptive seeking stuff because it was unrelated to it. I agree with @FernetMenta that we need to move it over to our seek handler, but we should not remove it without an alternative implementation IMO.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

BTW if we decide to drop support for it we should do it for v18 instead.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2016

I can't think of much contributions to videoplayer recently apart from myself. You can either support my tansformation of videoplayer or try to tell my what I should do or not. Decide for the latter and you are on your own on this journey.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Don't get me wrong I don't want to tell you anything. You are right. You are the only one currently maintaining this code part but we should decide together or better respect our policies while in beta stage, or don't we? If you decide for us it's also fine by me because I don't want to open an endless discussion about a feature I don't use very often by myself.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

Cut whatever features you like, no point discussing it really, is there?

@fritsch

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

This (in general) was discussed and agreed at DevCon. Sorry guys. Everyone is free to reimplement a feature. Remember the architecture presentation fernetmenta gave at devcon, the broken house image. Sometimes the only way forward is to tear down a wall. Oh, it hit your feature this time?

So then add it back in a maintainable way but blaming the cleaning up guy for throwing away things laying on the floor after a party is just unfair.

Again: after the seek fixes, an improvement, someone needs to fix the old crap. So then step up and contribute.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

So then add it back in a maintainable way but blaming the cleaning up guy for throwing away things laying on the floor after a party is just unfair.

We only want to know why he dropped a feature while fixing an issue, nothing else. No one is blaming him. For my part, I thought we should not drop a feature while in beta stage. I just wanted to clarify that and got that.

You can either support my tansformation of videoplayer or try to tell my what I should do or not. Decide for the latter and you are on your own on this journey.

Take it or leave it 😯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.