- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 52
 
Implement chapter time over full audiobook duration #1090
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
base: main
Are you sure you want to change the base?
Conversation
4acc267    to
    27df1b9      
    Compare
  
    | 
           I fixed the possible undefined errors during build, note that this is the first time I ever touch TS and Vue, so let me know if there are more standard ways to do what I did 😅  | 
    
| 
           Ready for another lint run/review  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great enhancement! Just some small comments on non-null assertion
d78c4e6    to
    205819d      
    Compare
  
    205819d    to
    2b3f74b      
    Compare
  
    | 
           Having just started to play with Audiobooks there are a number of considerations here. For audiobooks with not so many chapters having the full timeline means you can click on the chapter mark to jump to that chapter. Obviously in the example above this is not that relevant as there are so many chapters so maybe not something to worry about. However, while on this subject one thing the UI lacks is an indication of which chapter is currently playing. Is there anything we can do there? P.S. Check the lint probs...  | 
    
2b3f74b    to
    fad9aaa      
    Compare
  
    | 
           Whoops indeed I pushed to the wrong branch again, pushed to the one I use for my own build, but forgot to also push to the MR one 🙈 
 Indeed, though out of scope for this MR, maybe next one :)  | 
    
| 
           Without showing the user that this is a chapter playing, this is going to cause unclarity. We need to figure out the UX how to display/select chapters. Silently only showing the current chapter makes the progress bar nicer maybe not perse more easier.  | 
    
          
 At least for me, now it takes it from completely unusable and dangerous (misclick means I lose my place and jump to a random place in the book), to useful as I can seek around. I agree about the indication though - but I'd say this can be a first step to be merged as it's an opt-in and users should be aware that they have switched it on. I do intend to look at the second part after.  | 
    
| 
           You can just touch a chapter in the list to jump there  | 
    
cb33d2f    to
    302270f      
    Compare
  
    | 
           I just added audiobook chapter subtitles, this should be better now.  | 
    
| 
           Have you got some screenshots?  | 
    
302270f    to
    08025d8      
    Compare
  
    | const tempTime = ref(0); | ||
| const chapterTime = computed(() => | ||
| localStorage.getItem("frontend.settings.audiobook_chapter_time") == "true" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we certain a computed property is the right way to do this ?
Like, will it only be computed once and not over and over ?
There was a problem hiding this 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, have not used vue before this. I'm open to ideas.
On the other hand: Does it matter? How much overhead is a map lookup?
| 
           @marcelveldt This subtitle idea is somewhat aligned with the concept of providing another line in the now playing view to provide extra information for the Radio Paradise provider? Should we review that for a consolidated approach?  | 
    
666373f    to
    119fafc      
    Compare
  
    | 
           @marcelveldt Ready for another review round  | 
    


Why? A picture is a thousand words: