Bug 317385 - Command line option --queue does not work and behaves more like --append
Summary: Command line option --queue does not work and behaves more like --append
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: 2.7.0
Platform: Other Linux
: NOR major
Target Milestone: 2.8
Assignee: Amarok Developers
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2013-03-26 16:23 UTC by Diego Agulló
Modified: 2013-05-16 17:48 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.8
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Agulló 2013-03-26 16:23:33 UTC
When calling Amarok via the command line with amarok --queue <url>, it just appends given urls to the playlist.

Reproducible: Always

Steps to Reproduce:
1. Call amarok with amarok --queue <url>
Actual Results:  
The current behaviour apparently is just a copy of the --apend (-a) option.

Expected Results:  
I expect Amarok to:
 1) look for the actual song/url on the current playlist, or appending that song to the playlist if not found; and
 2) queue that song



The behaviour should be OK for some situations, but it's broken for most of them, like if the playlist is automatically sorted or the track order is random, it's basically useless, as the appended song will not be the "next in line".
Comment 1 Anmol Ahuja 2013-05-08 13:13:21 UTC
I'm working on this bug.
Comment 2 Anmol Ahuja 2013-05-08 13:31:24 UTC
Actually, according to in-source documentation:
Append     = 1,     ///< inserts media after the last item in the playlist
Queue      = 2,     ///< inserts media after the currentTrack
Unique     = 16,    ///< don't insert anything already in the playlist

But the this Unique enum doesn't seem to be used, should I just accept a -unique argument as well?
Comment 3 Diego Agulló 2013-05-08 14:02:30 UTC
Thanks Anmol for working on this bug!

Just dropping a wild thought here: why is the "Queue" argument inserting media after the currentTrack instead of using the built-in queuing system?

As a user, when I read "queue" docs, I expect it to really use that feature as I have the auto-sorting thing, which screws up the insertion order as far as playback concerns.
Comment 4 Anmol Ahuja 2013-05-08 14:43:32 UTC
You're welcome :)

And as for your question, I believe that is the current behavior, the inserted tracks should be sorted. Did you check it?
Comment 5 Mark Kretschmann 2013-05-08 19:07:52 UTC
I've looked a bit into this bug. It's a regression that has been there for a while, and it does not only affect command line functions.

The whole append-and-play logic is broken for any media that does /not/ come from the "Local Collection" tab. If you double-click on a file in the "Files" tab it doesn't play, and it also doesn't work for radio streams.
Comment 6 Anmol Ahuja 2013-05-09 14:20:26 UTC
Double clicking on a file from the Local Collection also just appends it to the playlist, though looking at the code, it is apparently supposed to append and play.

I might not be able to do much for now though, practical exams and all at college.
Comment 7 Mark Kretschmann 2013-05-09 17:50:45 UTC
Here we can see why it's not playing. The URL we put into EngineController is empty:


amarok:   [Playlist::Controller] engine playing?:  false 
amarok:   BEGIN: void Playlist::Actions::play(const int) 
amarok:     BEGIN: void Playlist::Actions::play(const quint64, bool) 
amarok:       BEGIN: void EngineController::play(Meta::TrackPtr, uint) 
amarok:         BEGIN: void EngineController::stop(bool, bool) 
amarok:           [EngineController] slotTrackFinishedPlaying( "Armin van Buuren" - "Mirage" - "Coming Home" , 0.00673421 ) 
void Phonon::MediaObject::setCurrentSource(const Phonon::MediaSource&) 5  QUrl( "" )  "" 
amarok:         END__: void EngineController::stop(bool, bool) [Took: 0.008s] 
amarok:         [EngineController] play: bounded is  QObject(0x0)  current "04 The Last Rose Of Summer.mp3" 
amarok:         [EngineController] Just a normal, boring track... :-P 
amarok:         BEGIN: void EngineController::playUrl(const KUrl &, uint) 
amarok:           [EngineController] URL:  KUrl("") "" 
amarok:           [EngineController] Offset:  0 
void Phonon::MediaObject::setCurrentSource(const Phonon::MediaSource&) -1  QUrl( "" )  "" 
Trying to set an invalid MediaSource -> ignoring.
amarok:         END__: void EngineController::playUrl(const KUrl &, uint) [Took: 0s] 
amarok:       END__: void EngineController::play(Meta::TrackPtr, uint) [Took: 0.008s] 
amarok:     END__: void Playlist::Actions::play(const quint64, bool) [Took: 0.008s] 
amarok:   END__: void Playlist::Actions::play(const int) [Took: 0.008s]
Comment 8 Anmol Ahuja 2013-05-09 20:09:29 UTC
It seems to try to play the track before it's loaded. Playing from the FileView works if you use BlockingLoading for the directory loader.
So maybe we can introduce a cachedUrl?
Comment 9 Mark Kretschmann 2013-05-09 20:19:09 UTC
@Anmol: Yep, that could be the problem. Matej has more clue about this code than me, though, let's ask him.

@Matej: Do you have an idea why this happens? If the bug report is unclear, let us know.
Comment 10 Anmol Ahuja 2013-05-09 20:21:57 UTC
Turns out there is already a cached url field, but it's not being used:

 /* don't return d->url here, it may be something like
         * amarok-sqltrackuid://2f9277bb7e49962c1c4c5612811807a1 and Phonon may choke
         * on such urls trying to find a codec and causing hang (bug 308371) */
Comment 11 Matěj Laitl 2013-05-09 22:09:47 UTC
(In reply to comment #5)
> The whole append-and-play logic is broken for any media that does /not/ come
> from the "Local Collection" tab. If you double-click on a file in the
> "Files" tab it doesn't play, and it also doesn't work for radio streams.

Mark, this is an entirely different (but existing) problem. The cause is introduction of MetaProxy::Track a year ago so that track loading is not blocking the UI. The solution is to introduce some RAII class (perhaps DelayedPlayer?) to watch on a Meta:Proxy track and play it once it becomes playable (with some timeout, 5 or 10 seconds).
Comment 12 Matěj Laitl 2013-05-09 22:22:28 UTC
Reverting to the original and more accurate title. (was changed by Myriam after the bug was reported to something I believe is misleading)
Comment 13 Anmol Ahuja 2013-05-10 11:01:28 UTC
(In reply to comment #11)
> The solution is to introduce some RAII class (perhaps
> DelayedPlayer?) to watch on a Meta:Proxy track and play it once it becomes
> playable (with some timeout, 5 or 10 seconds).

So we create this DelayedPlayer every time the EngineController receives a Proxy Track with a blank playable url?
Comment 14 Matěj Laitl 2013-05-10 11:09:04 UTC
(In reply to comment #13)
> So we create this DelayedPlayer every time the EngineController receives a
> Proxy Track with a blank playable url?

Nonono, just the method that calls trackForUrl() and immediately plays it (command line handling) should use DelayedPlayer. (which should be coded in a way that it plays the track immediately if it is already playable)
Comment 15 Matěj Laitl 2013-05-11 22:44:05 UTC
Git commit ecf6557cd611d442653719a266ca304aff2f6e14 by Matěj Laitl.
Committed on 12/05/2013 at 00:30.
Pushed by laitl into branch 'master'.

PlaylistController: fix inability to play just loaded tracks

...that was caused by asynchronism of playlists and MetaProxy::Tracks.
Now that we have TrackLoader which will magically solve all our
problems. :-)

This fixes Amarok not playing tracks passed to it via command line.

BUGFIXES:
 * Fix `amarok --play file.mp3` option didn't actually start playback.

@Anmol, now you can fix the "core" part of the bug 317385 as I outlined
in http://git.reviewboard.kde.org/r/110362/ (please reopen the review).

@Ralf, this is the "recommended" way to handle asynchronism in tracks
and playlists now.

CCMAIL: Chris <cjd@internode.on.net>
DIGEST: Amarok fixes playing of tracks passed on command line

M  +1    -0    ChangeLog
M  +56   -31   src/playlist/PlaylistController.cpp
M  +2    -1    src/playlist/PlaylistController.h

http://commits.kde.org/amarok/ecf6557cd611d442653719a266ca304aff2f6e14
Comment 16 Matěj Laitl 2013-05-11 22:45:47 UTC
(In reply to comment #15)
> @Ralf, this is the "recommended" way to handle asynchronism in tracks
> and playlists now.

Oh, this should have been @Markey, too furious when writing commit message.
Comment 17 Matěj Laitl 2013-05-16 15:51:52 UTC
Git commit 6a3f1c67f761803479f87a3080415057e61c49be by Matěj Laitl.
Committed on 16/05/2013 at 16:46.
Pushed by laitl into branch 'master'.

Playlist{Actions,Controller}: refactor code, fix queueing

The actual fix for bug was to remove the isPlayable() check in
TrackNavigator, however this fixes more potential problems (like the
queueing being affected by the current playlist filter) and cleans up
the code.

BUGFIXES:
 * Fix `amarok --queue` which didn't actually queue the tracks.
FIXED-IN: 2.8

M  +1    -0    ChangeLog
M  +13   -11   src/playlist/PlaylistActions.cpp
M  +12   -2    src/playlist/PlaylistActions.h
M  +44   -64   src/playlist/PlaylistController.cpp
M  +1    -13   src/playlist/navigators/TrackNavigator.cpp
M  +0    -1    src/playlist/navigators/TrackNavigator.h

http://commits.kde.org/amarok/6a3f1c67f761803479f87a3080415057e61c49be
Comment 18 Anmol Ahuja 2013-05-16 17:48:27 UTC
Thanks for fixing this :)
Sorry for not getting to it sooner.