GNOME Bugzilla – Bug 680647
Screen Recorder doesn't add video recordings to Recently Used
Last modified: 2012-10-26 17:30:17 UTC
When taking a recorded video ( ctrl shift alt R , twice ) and trying to attach it in a bug report, I get the "recently used" by default, but the recently used recording isn't in the list. Suggestion: Either add the video to the list of "recently used" or change the default for the file browser to not be "recently used" if "recently used" doesn't do what it says it does.
Created attachment 219713 [details] [review] recorder: Clean up stage lifetime handling The stage is a floating object. We don't own a reference to it, so we shouldn't unref it. This commit removes the erroneous unref call and makes sure we call clutter_actor_destroy on the stage when we're done with it.
Created attachment 219714 [details] [review] recorder: keep recorder object alive until pipeline finishes We want to make sure the recorder isn't finalized until the saved recording hits disk. This means the pipeline object needs a hard reference on the recorder.
Created attachment 219715 [details] [review] recorder: keep test-recorder alive until done recording Recording continues for some time after the recorder object is closed, since closing isn't a synchronous operation. This commit defers quiting the test-recorder application until the recording is finished.
Created attachment 219716 [details] [review] recorder: save recorded video as recent item Often the first thing a user wants to do after making a recording, is posting it somewhere. This commit adds the video to recently used items, so that it shows up prominently in open file choosers.
Review of attachment 219713 [details] [review]: ::: src/test-recorder.c @@ +20,3 @@ } + clutter_actor_destroy (stage); This commit is talking about the unref call, but you also seem to have deleted the call to clutter_main_quit(). That doesn't seem right.
Review of attachment 219714 [details] [review]: The fact that the old code seems to have went out of its way to avoid a reference (with an explicit comment!) makes me nervous about this. I don't see a problem immediately with holding the ref; at least there's no apparent circularity. Probably Owen should review this one.
Review of attachment 219713 [details] [review]: ::: src/test-recorder.c @@ +23,1 @@ The code calls clutter_main_quit when the stage is destroyed. I could improve the commit message to mention that. clutter_main_quit is wrong, since it means the stage won't get destroyed (albeit that's probably harmless)
Review of attachment 219715 [details] [review]: ::: src/test-recorder.c @@ +23,3 @@ + (GWeakNotify) + clutter_actor_destroy, + stage); This is just test code so...whatever, but it seems like a cleaner fix would be to watch a "state" property on the recorder or something. Anyways this is fine though.
Review of attachment 219716 [details] [review]: Makes sense.
i'll hold off on committing attachment 219716 [details] [review] until owen chimes in on attachment 219714 [details] [review] (also i need to fix the s/, is posting/is post/ typo in the commit message)
Review of attachment 219714 [details] [review]: It's certainly safer and more natural to not have a reference loops - so that's probably why I wrote it without that. But if we need to have a reference loop until the pipelines finish, I don't see a problem with that.
Review of attachment 219716 [details] [review]: Previously, I just looked at ~/.local/share/recently-used.xbel to confirm an entry was added. Today before pushing I did some better testing and this patch needs more work (or we need one more prerequisite patch) ::: src/shell-recorder.c @@ +1465,3 @@ + recent_manager = gtk_recent_manager_get_default (); + + file = g_file_new_for_path (recorder->filename); recorder->filename is a file pattern with substitution variables not a filename.
Created attachment 227311 [details] [review] recorder: rename "filename" property to "file-template" The filename property is actually a template string with substitution variables, not a filename. This commit renames for clarity.
Created attachment 227312 [details] [review] recorder: save recorded video as recent item Often the first thing a user wants to do after making a recording is post it somewhere. This commit adds the video to recently used items, so that it shows up prominently in open file choosers.
Review of attachment 227312 [details] [review]: Untested? ;-) ::: src/shell-recorder.c @@ +1224,3 @@ + if (outfilename != NULL) + *outfilename = g_string_free (filename, FALSE); This is wrong - if filename is not an absolute path (like our default of "Screencast from some time"), get_absolute_path() will resolve to $XDG_VIDEOS_DIR/filename. You could do if (outfilename != NULL) *outfilename = path; else g_free (path); to avoid the extra strdup I guess
Review of attachment 227311 [details] [review]: This one looks good to me
(In reply to comment #15) > Review of attachment 227312 [details] [review]: > > Untested? ;-) Nope, I tested it, I promise! (see comment 12) For some reason my ~/.config/user-dirs.dirs has this in it: XDG_VIDEOS_DIR="$HOME/" no idea why. Also my XDG_MUSIC_DIR is $HOME... weird.
Created attachment 227369 [details] [review] recorder: save recorded video as recent item Often the first thing a user wants to do after making a recording is post it somewhere. This commit adds the video to recently used items, so that it shows up prominently in open file choosers.
Review of attachment 227369 [details] [review]: Looks good, thanks!
Attachment 219713 [details] pushed as f9819eb - recorder: Clean up stage lifetime handling Attachment 219714 [details] pushed as 9171bab - recorder: keep recorder object alive until pipeline finishes Attachment 219715 [details] pushed as 92033ce - recorder: keep test-recorder alive until done recording Attachment 227311 [details] pushed as fbeb446 - recorder: rename "filename" property to "file-template" Attachment 227369 [details] pushed as eb09f34 - recorder: save recorded video as recent item