GNOME Bugzilla – Bug 129789
starts playing when you click Next or Previous while paused
Last modified: 2018-05-24 10:27:40 UTC
This bug was originally reported in the Debian BTS: http://bugs.debian.org/218262 "After starting to play a song, and then pausing it, hitting next or previous will cause Rhythmbox to start playing. Rhythmbox should just select the next song and not start playing."
Created attachment 27893 [details] [review] A patch to keep the player paused With this patch, when moving to the next or previous songs, the state of the player is checked first (playing or not), and then either the song is loaded, or loaded and played.
Whoops. I should say that this behaviour is still present in CVS-HEAD, which is what the patch is against.
Created attachment 34760 [details] [review] keep playing state Here's the same patch updated against latest 0.8.
(Setting version accordingly.) I find the patch rather clean, I tried understanding the call stacks involved in rb_shell_player_do_next() and rb_shell_player_do_previous(), and while some things are completely obscure to me, I think that the approach of this patch is: - in _do_next() and _do_previous() replace calls to a function which did simply request for something to be played (rb_shell_player_set_playing_entry) by a function which do the same request but save playing state prior to the call, and restores it afterwards (rb_shell_player_set_entry_with_current_state), - remove all things which caused play to be started from rb_shell_player_set_playing_entry to make a rb_shell_player_set_entry out of it, - rb_shell_player_set_playing_entry is redefined to call rb_shell_player_set_entry and set state to playing. - remove the borken save playing state code from "rb_shell_player_open_location". Does anything prevents this patch from inclusion?
Actually the patch is broken, it breaks the windows title which always display "paused", playing or not. James, did you notice this problem too? Did you work around it?
This issue still in rhythmbox HEAD. It could be argued that this is a feature, not a bug, depending on the users expectations. Many CD players work the way rhythmbox does, i.e. if it's paused, and you press forward or backward it will start playing. Just my $0.02. Btw, the patch will probably not apply against HEAD. Please obsolete the patch/provide a new one if so.
Keeping the paused state is clearly a feature to me. When I play guitar to songs, it is useful that I can rewind to the begining of the song, and only one click is needed to start the song again. This is very useful for songs (like Queen's song "39") that have no silents in the begining, and timing is crusial.
So far from what I can tell, pressing pause and then previous does this, but the meter bar doesn't go back. So is this half fixed, with a UI bug remaining? PS: My CD player keeps the pause state when I hit previous, but not next.
*** Bug 481616 has been marked as a duplicate of this bug. ***
Created attachment 118748 [details] [review] fix to maintain the "playing/paused" state on next and previous button clicks I have added code to pass the "playing" state down the stack and if the player is in the paused state, then immediately after the play function is called a call is made to the pause function. I fear that this may a bit of a hack fix, but I did not want to rewrite the RBPlayerIter interface to accommodate this bug since it may have far reaching implications. Calling the play method defined in the interface was the only way I could find to update the GUI elements, so here we are with a pause command right after.
The 'error' parameter to rb_shell_player_get_playing cannot be removed. That function is part of the dbus interface, so it needs to return a boolean indicating success or failure, and to have a 'GError **' as its last parameter. If you're adding new parameters to a function that takes a 'GError **' parameter, they should go before it. By convention, a patameter used to return a GError is the last parameter. Actually, I don't think the 'open_and_play' parameter is required in most of the places it's used here. Instead, it should be a new value for the PlaybackStartType enum. Also, we don't use C++ style (// ...) comments. Details of the patch aside, I don't really see much of a reason to make this change. Why is it useful to move to the next song without playing it?
I only removed the error parameter because it was ignored at the lowest level *shrug*. Anyway, I am just going down the list of bugs so did I misinterpret the ticket request? If not then perhaps this bug should be resolved to "wont fix"? Just leave a comment on what further actions you wish to be taken on this issue :)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME'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.gnome.org/GNOME/rhythmbox/issues/23.