-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[video] select proper bookmark item based on current playtime #6812
Conversation
3eb9d64
to
f9be674
Compare
After a talk with @mkortstiege we decided to rewrite the whole bookmark/chapter list processing for the video bookmark dialog. Currently the bookmarks comes first followed by the chapters if available. With this we put everything together and sort them by the resume point. |
@xhaggi, mkortstiege@83cca0f adds formatted labels. For bookmarks it will be |
I don't think that the sorting/mixing is a good idea, at least when the bookmarks were created manually. Because I would use them for special locations in the video and I wouldn't want to search for them in a rather long list. |
@ace20022 together with the second commit and the commit from @mkortstiege which set a reasonable label for the bookmark to differ them from chapters it make sense. It's really annoying if you open the dialog that you have to scroll down the entire list if you are past an hour or so to reach the next chapters/bookmarks. IMO this improves the usability of this dialog a lot. BTW you always know where you want to go in time. Sure not exactly but nearly "somewhere in the middle" and if the items are sorted this way everyone should understand it at the first use. Currently you need to know "ahh the bookmarks comes first followed by the chapters and every type is sorted separately" |
@mkortstiege thanks for your work ;) .. i'm thinking about using Label1 for the human readable name "Chapter 1" or "Bookmark" and Label2 for the formatted time string so it's up to the skinner how to use/where to output it. What do you think? |
@xhaggi sounds good. We just need to agree on the sort order. |
For my own use, which is mainly episode bookmarks, it makes sense to see them sorted by time. I see bookmarks simply as "chapters" that the user can set on the fly. At the same time, I can see Ace's point. Bookmarks is a feature that is kind of hidden from "common" Kodi use, which can make it hard to see how various users utilize bookmarks. I don't really have a strong preference myself. Perhaps if bookmarks also had something else to help them jump out on long list, maybe something skinnable, like colored text? Something that helps to express that those are the user-set marks, and to quickly find them. Just a thought. A side note, it would also be nice if there could be one action ID that goes to the next bookmark or chapter, as next chapter and next bookmark (and previous for each) have their own action ID. |
sure something like that is possible but this is part of the skin. |
@ace20022 what do you think if we merge this in and wait what the rest of our community will say. the sorting is a one-liner and could easily reverted https://github.com/xbmc/xbmc/pull/6812/files#diff-35e0b71ef38804d3271341a7c7767950R318 |
I don't mind, just to be clear it's the mixing of bookmarks and chapter I don't like, not the sorting. I haven't reviewed the changes in detail, but what leaped out at me is: xhaggi@f9be674#diff-35e0b71ef38804d3271341a7c7767950R325 IMO it's not necessary to call |
Yep I know ;) The mixing is done by this std::sort. Without the list is bookmarks followed by chapters.
okay will move out the call to |
897187c
to
8825c15
Compare
@ace20022 done |
int itemPos = ((chapterIdx - 1) + m_chapterOffset); | ||
|
||
int itemPos = 0; | ||
for (auto& item : m_vecItems->GetList()) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
8825c15
to
7188e2d
Compare
I like this change as it totally makes sense to me. The only thing missing UX wise is to change Conflunce in a similar way your screenshot of Xperience1080 looks like - so making the bookmarks a horizontally scrolling thumbnail list and have a button for it in the OSD menu. I'd not show the bookmark/chapter name inside the thumbnail though, but in an easy to read (so large enough) font above or below the bar. |
7188e2d
to
71b06b1
Compare
@mkortstiege I've added a new condition to your check if we have a readable/valid chapter name. If the chapter name only contains a number we also fallback to our chapter label |
@mkortstiege do we need a squash? IMO it's fine as it is. @da-anda sure, if the bookmark item holds the info that it is a resume bookmark. |
@xhaggi nvm. Thought you added the numeric check on top ;) |
jenkins build this please. |
@xhaggi I changed my mind about the resume point. While it's possible to detect the bookmark type (see https://github.com/xhaggi/xbmc/blob/select-proper-bookmark-item/xbmc/video/dialogs/GUIDialogVideoBookmarks.cpp#L269) it's way to complicated to determine if it should be hidden or not, because the dialog could be triggered while not playing this video OR the video could be played from the beginning instead of the resume point and in all these situations the resume point should be kept. I'd probably adjust it's label though and not call it "Bookmark" like user generated ones. Maybe "last playback" or "resume playback" (@jjd-uk - suggestions?) btw - isn't the label split sort of a breaking skin change as it makes bookmarks for not adjusted skins pretty unusable (every bookmark only shows "Bookmark" and no timecode)? We probably should use the timecode as primary label and the type string as label2? @ronie? Also, could we forward the bookmark type as cFileItem property to skins so that they can apply custom styles depending on type? This would help to find custom bookmarks now that they are mixed with chapters. |
In that case the bookmark dialog stays empty. You can't use it without a video playback. Best we hide the resume bookmark as you suggested first IMO.
I've to disagree here, because we do not break the dialog or something else. We'll release a major version so skin changes like this are fine IMO.
currently it's possible to differ between chapters and bookmarks. you can check if property |
846dc8b
to
32060cd
Compare
32060cd
to
22f93c4
Compare
22f93c4
to
27f0faa
Compare
@da-anda addressed everything we discussed at IRC |
jenkins build this please |
@da-anda I see changes have to made since your ping and something has been discussed on IRC (via PM I guess since I can't see anything in chatlog), so are you still after naming suggestions? |
@jjd-uk there is no need for a naming suggestion from your side because we dropped the resume bookmark entry from the bookmark dialog. |
[video] select proper bookmark item based on current playtime
@mkortstiege @xhaggi @ronie Is it possible when jumping to a chapter to use that bookmark image in the seek bar? |
i would say no |
Is there no way the bookmark images could be made available to the other video windows then? |
The (auto) close after chapter selection is really annoying when you want to add episode bookmarks. |
if this is currently the case (never used bookmarks) than this needs to be changed IMO. Dialog should only close when a chapter is selected for playback, not in any other case. |
I tried that http://kodi.wiki/view/Bookmarks_and_chapters#Episode_bookmarks yesterday and used chapters to jump to the (fake/virtual) episode start times. Therefore the dialog was closed and I had to reopen it to set the episode bookmark, which ofc is then wrong for some seconds. Pausing and jumping to chapters didn't work at all ;) I'll take a look at that. |
This will select a proper (chapter) bookmark item based on the playtime of the current media file.
@ace20022, @mkortstiege mind taking a look.