GNOME Bugzilla – Bug 740995
Starting totem with a file as parameter adds the file twice to the playlist since 3.14.1
Last modified: 2015-04-22 11:39:58 UTC
Make sure totem is currently not running and then start it via "totem somefile.mp4". The file has now been added to the playlist twice.
Correct me Sebastian if i am wrong but how did u saw the playlist because i don’t know if there there is any option to see the playlist in totem ,and also when i played a file using totem i don’t see it playing again once finished. I am currently using opensuse 13.2 64 bit and gnome 3.14.0 and totem 3.14
I'm not aware of any way to display the playlist directly, but the previous/next buttons go through the entries in the playlist. When opening a file using the totem binary directly rather than using bus activation, the "next" button gets enabled and clicking it plays the same file again, so it got added to the playlist twice. Opening a file in nautilus doesn't run the totem binary directly but rather uses bus activation, so this won't show the problem. But if you use something like gpodder (or the commandline) to launch totem, this runs the binary and results in the file being added twice. You also can't see this behavior in 3.14.0 because the bug got introduced in 3.14.1. I put 3.14.2 in the title of this report by mistake, sorry for the confusion.
I can confirm this also happens with the latest totem git version. I played a bit with gdb and noticed that when totem is not already running and is launched with some files as arguments, totem_pl_parser_parse_async is called twice resulting in duplicates items in the playlist. Here are the two stacks: $ gdb --args totem foo.m3u (gdb) b totem_pl_parser_parse_async (gdb) r Starting program: /home/spasche/jhbuild/install/bin/totem foo.m3u Breakpoint 1, totem_pl_parser_parse_async (parser=0xca5ab0, uri=0x1226ad0 "file:///home/spasche/foo.m3u", fallback=0, cancellable=0x0, callback=0x7ffff7b86eb4 <add_mrls_cb>, user_data=0x1223900) at totem-pl-parser.c:2149 2149 totem_pl_parser_parse_with_base_async (parser, uri, NULL, fallback, cancellable, callback, user_data); (gdb) bt
+ Trace 234481
Created attachment 293988 [details] [review] main: Fix Totem doubling files added on the cmd line
Attachment 293988 [details] pushed as 751c1da - main: Fix Totem doubling files added on the cmd line
Reopening this bug as I can still reproduce the problem in 3.14.2.
Created attachment 301834 [details] [review] Fix Totem doubling files added on the cmd line (bgo#740995). This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1). The problem results from the fact that the command line files are both processed in totem_options_process_for_server() and totem_object_app_activate(). When I traced the program, totem_options_process_for_server() always seemed to be called before totem_object_app_activate(), so I'm not sure if my solution is a little overkill. I don't know if there would ever be cases where the handlers would be called in reverse, or even repeatedly called. The solution should be sufficient if this is the case.
(In reply to Carlos Maddela from comment #7) > Created attachment 301834 [details] [review] [review] > Fix Totem doubling files added on the cmd line (bgo#740995). > > This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1). > > The problem results from the fact that the command line files are both > processed in totem_options_process_for_server() and > totem_object_app_activate(). That should be: totem_object_app_activate and totem_object_app_handle_local_options Both of which are GApplication class methods. > When I traced the program, > totem_options_process_for_server() always seemed to be called before > totem_object_app_activate(), so I'm not sure if my solution is a little > overkill. From the docs, ->handle_local_options is called after parsing of command-line options has occurred. Looking at g_application_real_local_command_line() in glib/gio/gapplication.c ->handle_local_options will be called before activate (if activate is called rather than ->open). > I don't know if there would ever be cases where the handlers would > be called in reverse, or even repeatedly called. The solution should be > sufficient if this is the case. I don't think that those 2 functions can be called in parallel in any case, so I'm not too sure what this patch is trying to achieve. Can you explain how you got to this fix?
(In reply to Bastien Nocera from comment #8) > (In reply to Carlos Maddela from comment #7) > > Created attachment 301834 [details] [review] [review] [review] > > Fix Totem doubling files added on the cmd line (bgo#740995). > > > > This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1). > > > > The problem results from the fact that the command line files are both > > processed in totem_options_process_for_server() and > > totem_object_app_activate(). > > That should be: > totem_object_app_activate > and > totem_object_app_handle_local_options > > Both of which are GApplication class methods. > > > When I traced the program, > > totem_options_process_for_server() always seemed to be called before > > totem_object_app_activate(), so I'm not sure if my solution is a little > > overkill. > > From the docs, ->handle_local_options is called after parsing of > command-line options has occurred. Looking at > g_application_real_local_command_line() in glib/gio/gapplication.c > ->handle_local_options will be called before activate (if activate is called > rather than ->open). > > > I don't know if there would ever be cases where the handlers would > > be called in reverse, or even repeatedly called. The solution should be > > sufficient if this is the case. > > I don't think that those 2 functions can be called in parallel in any case, > so I'm not too sure what this patch is trying to achieve. > > Can you explain how you got to this fix? I did say that it may be a little overkill, but I just went for the safer option. My original solution was just to remove the code that handled the command line parameters from totem_object_app_activate(). While this worked fine for the limited tests I did, I thought it had a good chance of introducing side effects. So my second solution was based on your attempt to fix this bug: by nullifying optionstate.filenames to indicate the command line has already been processed. Your attempt didn't work because totem_object_app_handle_local_options() is called before totem_object_app_activate(). I've just realised though that my solution resorts to accessing the file names through the global optionstate struct, when it should be accessing it through a const pointer. So it's introduced some ugly code. As for concurrency issues, my first attempt to remove the atomic pointer calls from my solution resulted in a crash. It seems like you need to nullify optionstate.filenames very early on before other function calls are made. I'm still not entirely convinced that this would be safe code. I'll attach these patches for you to review.
Created attachment 302100 [details] [review] Original Fix
Created attachment 302102 [details] [review] Fix Without Atomic Calls
Created attachment 302141 [details] [review] main: Fix (again) doubling files added on the cmd line When handling local command-line options, we would process all the options and send them as "remote" commands through totem. But the "remote-command" action would activate the application first. Thus we would do something like: - process command-line options in totem_object_app_handle_local_options() which calls totem_options_process_for_server(), which sends out "remote-command" actions - when handling "remote-command", to make sure that the application is actually ready, we call "activate". - but we didn't clear the filenames struct member in optionstate, so we process it again in "activate" With help from Carlos Maddela <maddela@labyrinth.net.au>
Attachment 302141 [details] pushed as ea29374 - main: Fix (again) doubling files added on the cmd line