! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
Previous Track button behaves incorrectly when in shuffle mode
Status:
RESOLVED: MOVED

Comments

Description woob 2017-07-29 09:20:28 CEST
When Parole is set to shuffle, pressing the Previous Track button will play the track previously listed on the playlist, rather than the true previously played track.
Comment 1 woob 2018-04-04 02:04:17 CEST
Created attachment 7660 
Patch for track shuffle history

Adds a track history feature used by shuffle when cycling back and forth through previously played tracks.
Comment 2 woob 2018-04-04 02:05:08 CEST
I've put together my own possible fix for this, which adds a track history mechanic to Parole, which I've attached a sort-of-maybe-WIP patch for. The track history mechanic may also be useful for bug 13667.

I am definitely a novice to C and GTK, so please excuse any glaring issues my code might have, especially with respect to proper memory management. I've also left some testing code in there that I don't expect to actually make it into Parole.
Comment 3 alexxcons editbugs 2019-02-02 22:31:28 CET
Created attachment 8280 
improved patch

Thanks alot for the patch, nice work !

Basics work fine here,  in terms of functionallity just found a minor flaw:
- If I travel via "pevious" further than the first track, I would expect that a random track is played. Currently nothing is played, and I cannot press "next" any more.

I did some testing in the code to see why certain thing are needed ... and it looks like I found out that encapsulation of "parole_player_media_activated_cb" is not, but I am not so sure yet. Finally I came up with a patch, which fixes the above mentioned bug. Could you please take a try for it ?

I am not sure, because I am now getting this one now on each  next/prev:
(parole:23212): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()
So obviously I did something wrong :)  ... any idea ?

As well added some comments on your code:
https://github.com/alexxcons/parole/commit/ee89cc9534ff54c70b2422ff79e2fa81747317c9

Memory management / free allocated memory looks fine as far as I can tell.

Build the patch with "git format-patch HEAD -1" .. (please as well do so, it simplifies handling for me) I added you as author .. please check if your name / mail is fine like that.
Comment 4 alexxcons editbugs 2019-02-02 22:50:02 CET
Created attachment 8281 
improved patch

ok, found my bug .. I erased a minus sign while putting several lines into one ;) 

Now it should work fine, without spitting Warnings.
Comment 5 woob 2019-02-03 20:45:58 CET
Thank you very much for taking a look, and for the very constructive commentary and fixes! There are a couple things from my previous patch that I would suggest be left in, as follows:


For encapsulating `parole_player_media_activated_cb`, this is done to handle when the user selects a track directly from the playlist/sidebar, in which case the selected track will be bumped to the front of the history chain.
Without wrapping it as such, history won't be updated when a track is selected directly, causing one of the following couple side effects:
  -  If navigating to an unplayed track, the user will lose their place in the history chain, unable to navigate back to the track they were previously playing
  -  If navigating to a played track, the user will jump in the history chain to whatever point that track is on


Calling `parole_media_list_remove_history` when switching next/prev tracks outside of shuffle mode is done to prevent anything janky from happening if a user switches shuffle on/off while going though a playlist.
Without this, moving via sequential navigation to a track that was previously played on shuffle mode, and then turning shuffle mode back on will drop the user into whatever part of the history chain that track was previously recorded as. So, isolating from history every track played in sequential mode is a simple way to prevent this.
There's even an argument to be had for keeping the track history fully up-to-date outside of shuffle mode as well, considering that just isolating tracks from history bears the side effect of losing all the user's shuffle history should they choose to switch back to shuffle mode later. However, that caveat isn't of as great concern in my opinion. :)


I will also try to keep better patch formatting in mind henceforth. Name/contact is all fine, thank you!
Comment 6 alexxcons editbugs 2019-02-04 00:21:39 CET
Ok, for encapsulating, I did not see that use-case :) So yes, now it makes sense for me / now it is fine for me !
Possibly would be good to have your reasoning as comment in the code.

For modifying the history on non-shuffle:  As well ok for me, tough I dont think it would be too bad to re-gain the old shuffle history ;)
Yes, my user-expectation probably would be to get a different random ahuffle sequence.
As well would be nice to have some reasoning about it in the code.

Thank you for working on it !
Comment 7 Git Bot editbugs 2020-05-24 01:44:55 CEST
-- GitLab Migration Automatic Message --

This bug has been migrated to xfce.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.xfce.org/apps/parole/-/issues/34.

Please create an account or use an existing account on one of our supported OAuth providers. 

If you want to fork to submit patches and merge requests please continue reading here: https://docs.xfce.org/contribute/dev/git/start#gitlab_forks_and_merge_requests

Also feel free to reach out to us on the mailing list https://mail.xfce.org/mailman/listinfo/xfce4-dev

Bug #13749

Reported by:
woob
Reported on: 2017-07-29
Last modified on: 2020-05-24

People

Assignee:
Simon Steinbeiss
CC List:
2 users

Version

Attachments

Patch for track shuffle history (13.87 KB, patch)
2018-04-04 02:04 CEST , woob
no flags
improved patch (9.82 KB, patch)
2019-02-02 22:31 CET , alexxcons
no flags
improved patch (9.82 KB, patch)
2019-02-02 22:50 CET , alexxcons
no flags

Additional information