GNOME Bugzilla – Bug 595867
Use names based on date instad of wort "Untitled".
Last modified: 2012-06-15 17:37:37 UTC
Created attachment 143628 [details] [review] Use-names-based-on-date-instad-of-wort-Untitled.patch I found for my self really useful to have files named by date by default instead of have it "Untitled". For example: was - Untitled.ogg now - 2009-09-21-190728.ogg
Created attachment 143832 [details] [review] v2 Use-names-based-on-date.patch This patch have more cleans
Review of attachment 143832 [details] [review]: ::: grecord/src/gnome-recorder.c @@ +137,3 @@ + char date[21]; + time_t tm; + struct tm *ptr; interesting indentation @@ +144,3 @@ + + ptr = localtime (&tm); + tm = time (NULL); %F is C99 - I guess we could require it... but let's just have %Y-%m-%d @@ -148,3 @@ - * name as default value. */ - * there is no active sound sample. Any newly - /* Translator comment: untitled here implies that is it really safe to remove the utf8 part? I understand that strftime might be in local, even if you don't seem to use localizable part it in your format string. ::: grecord/src/gsr-window.c @@ -1740,3 @@ - * recorded samples will be saved to disk with this - * there is no active sound sample. Any newly - /* Translator comment: untitled here implies that why it doesn't generate a new name anymore? @@ -1742,3 @@ - * recorded samples will be saved to disk with this - * there is no active sound sample. Any newly - /* Translator comment: untitled here implies that why? why do you remove the title update?
but I like the idea otherwise :)
for grecord/src/gnome-recorder.c, no problem can change it. for grecord/src/gsr-window.c, this is point less. Old code was broken. File name is set only one time in gsr_window_set_property (priv->filename) on the start. If we change here windowname, file name will be still the same.
(In reply to comment #6) > for grecord/src/gnome-recorder.c, no problem can change it. > for grecord/src/gsr-window.c, this is point less. Old code was broken. File > name is set only one time in gsr_window_set_property (priv->filename) on the > start. If we change here windowname, file name will be still the same. thanks for your quick reply. I think it works, at least, when I open a new window and start recording on 2.28.1, it changes the title and windowtitle. Please make sure you don't break such usecase, although it's arguable why gnome-sound-recorder handle multiple document...
Try this steps: 1. Start gsrecord -> window titel is "Untitled - Sound Recorder" 2. start recording -> Windoew titel is now "Untitled - 2" () 3. do the step 2 more times... (you'll get "Untitled - 5" or so) and pres save. I expect to have untitled-5.spx but you will get just untitled.spx So this part is broken. If you after this step press "New" you will get "untitled-6" and after record you'll get untitled-6 as expected. So i just remove not working part, you'll get same window titel and same file name. After you press new, you'll get new filename(date).
Created attachment 151275 [details] [review] v3 patch Here is the patch, i use linux kernel coding style. Hope it is ok :)
(In reply to comment #9) > Created an attachment (id=151275) [details] [review] > v3 patch > > Here is the patch, i use linux kernel coding style. Hope it is ok :) no, :) it's not - not really. Even if the GNOME code (and GNOME Media) is not always consistent, try to follow how it is indented/spaced/named.. overall. I can't find a good pointer, perhaps http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html, but this does not address what I have in mind, like the space between ( and function name in calls... Please try to follow a more GNOME/Gtk+ style
Created attachment 151308 [details] [review] v4 patch Here is the patch with spaces between function names and parentheses. If you have some free time read this http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle;h=8bb37237ebd25b19759cc47874c63155406ea28f;hb=HEAD
(In reply to comment #11) > Created an attachment (id=151308) [details] [review] > v4 patch > > Here is the patch with spaces between function names and parentheses. > thanks, will take a look soon. > If you have some free time read this > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle;h=8bb37237ebd25b19759cc47874c63155406ea28f;hb=HEAD It's not the GNOME coding style.
Review of attachment 151308 [details] [review]: ::: grecord/src/gnome-recorder.c @@ +133,3 @@ + char date[18]; + time_t tm; + struct tm *ptr; please avoid this kind of indentation. ::: grecord/src/gsr-window.c @@ -1710,3 @@ record_start (gpointer user_data) { static gboolean so now, when a new recording start, it keeps the same old name?
> It's not the GNOME coding style. from gnome coding style reference: If you do not have any personal preference for a style, we recommend the Linux kernel coding style, or the GNU coding style. (But it is not the point for me now.) > please avoid this kind of indentation. Ok, sending new patch. > so now, when a new recording start, it keeps the same old name? yes. it is the way it should be. Better no action when a buggy action. Any way, it should be handled with separate patch.
Created attachment 151492 [details] [review] v5 patch
(In reply to comment #14) > > so now, when a new recording start, it keeps the same old name? > yes. it is the way it should be. Better no action when a buggy action. Any way, > it should be handled with separate patch. I disagree, a new recording is a new recording, like a new document, it should not delete the old one. At least not without a big warning. Can we fix this?
There is two types of new recording: new in a new window - this will create a new name and no warning is needed. and "new" rerecord in same window - this one will have same name. There is no sense to fix it. If you start second time the record in same window you will lost your data any way. gsr will warn you about it. The "fix" will be redesign of gsrecorder, removing save button and save the record automatically, user should have possibility remove the record.
Any progress on this ?
No jet, I'll start ASAP. Any one who has time to do it now, are welcome.
(In reply to comment #19) > No jet, I'll start ASAP. > Any one who has time to do it now, are welcome. Doesnt seem like anyone else is working on it. :)
Created attachment 165671 [details] [review] v6 patch Here is new patch, it generate new name on each record, so we do not record to the same file.
here is video example, how it's work: http://www.youtube.com/watch?v=7_TP5JpoxF0
Review of attachment 165671 [details] [review]: Thanks for looking at it again. Minor comment about the patch: ::: grecord/src/gnome-recorder.c @@ +139,3 @@ + strftime (date, 18, "%Y-%m-%d-%H%M%S", ptr); + + utf8_name = g_strdup (("%s", date)); Can we put those 8 lines to generate the name in a seperate function, instead of duplicating the code in 2 places?
Created attachment 165686 [details] [review] v7 patch created separate function.
Review of attachment 165686 [details] [review]: ::: grecord/src/gsr-window.c @@ +1697,3 @@ +char * +gsr_gen_date (char *date) +{ where is the function declared in the header? Please change the name and the arguments of the function: gsr_gen_date(char*) -> gchar* gsr_generate_filename(void) It should return a newly allocated filename.
Created attachment 165697 [details] [review] v8 patch new reworked version. I still learn to work with pointers. Hope it is what you mean.
Review of attachment 165697 [details] [review]: ::: grecord/src/gnome-recorder.c @@ +133,3 @@ if (filename == NULL) { + date = gsr_generate_filename (); + name = g_strdup (("%s", date)); Why not just "name = gsr_generate_filename()" ? ::: grecord/src/gsr-window.c @@ +1701,3 @@ + time_t tm; + + gchar *date = malloc (18); We usually use g_malloc() @@ +1721,3 @@ + + gtk_window_set_title (GTK_WINDOW(window), date); + You shouldn't set the window title here, you are setting again just a few lines later. @@ +1723,3 @@ + + g_free (window->priv->filename); + window->priv->filename = g_strdup (date); You could also avoid an extra copy here. g_free (window->priv->filename); window->priv->filename = gsr_generate_filename (); @@ +1728,3 @@ + title = g_strdup_printf (_("%s — Sound Recorder"), date); + gtk_window_set_title (GTK_WINDOW (window), title); + and why do you set the window title twice?
Created attachment 165708 [details] [review] v9 patch updated. Thank you for your patience.
Review of attachment 165708 [details] [review]: Well, thank you for *your* patience :) Last comment, and then I think it's ready for commit: ::: grecord/src/gsr-window.h @@ +61,3 @@ +gchar* gsr_generate_filename (void); + oops, I missed that: please put that declaration before the #endif also, please try to keep the same indentation as with the above functions
Created attachment 165721 [details] [review] v10 patch Hope this is last edition :)
Hmm , the review - new patch volley stopped! Did the latest patch turn out very bad... ;)
If comment 29 was last thing todo, than the patch is ready. ping Marc-Andre
I see some one writing gsr in vala. But i think it will be ok if we add this patch till gsr.vala will be ready. Hallo !!! any one !!!
Hi, Could a gnome-media developer comment on the patch here?
just for information the patch is used in ubuntu now
Review of attachment 165721 [details] [review]: ack
Comment on attachment 165721 [details] [review] v10 patch This has been committed in commit 8ccd7a9b2bb4ca31fc2056c705411b14c9ed21b9