GNOME Bugzilla – Bug 657322
GtkPrintSettings requires every application to work out file type
Last modified: 2012-08-22 08:41:22 UTC
On the GnomeGoals/Print to File page (http://live.gnome.org/GnomeGoals/PrintToFile) there is a huge list of applications that can implement a document name rather than just rely on the default "output.pdf/ps/svg" currently the GTK api forces each application to have to check for the file type to set the output file extention. This mean a massive amount of duplicate code needs to be created and maintained in each application. It would be better to improve GTK+ to handle this. An example of the code required is: (from here: http://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/printing.c?id=3.1.12) settings = gtk_print_settings_new (); dir = g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS); if (dir == NULL) dir = g_get_home_dir (); if (g_strcmp0 (gtk_print_settings_get (settings, GTK_PRINT_SETTINGS_OUTPUT_FILE_FORMAT), "ps") == 0) ext = ".ps"; else if (g_strcmp0 (gtk_print_settings_get (settings, GTK_PRINT_SETTINGS_OUTPUT_FILE_FORMAT), "svg") == 0) ext = ".svg"; else ext = ".pdf"; uri = g_strconcat ("file://", dir, "/", "gtk-demo", ext, NULL); gtk_print_settings_set (settings, GTK_PRINT_SETTINGS_OUTPUT_URI, uri); Suggestion: ----------- gtk could have print settings for OUTPUT_DIR_URI and OUTPUT_BASENAME and figure out the extension itself
Created attachment 206242 [details] [review] Patch to provide more flexable setting of filename, and directory I've attached my patch to provide more flexable setting of filename, and directory. It allows each to be set individually while reducing the amount of code an application needs to write to implement a new default filename.
The GTK_PRINT_SETTINGS_OUTPUT_URI setting could be depreciated in favor of the two new settings.
Created attachment 206243 [details] Small test program I'm attached a small program to test with.
To compile the test program: g++ simple-gtk3.cpp -o simple-gtk `pkg-config gtk+-3.0 --cflags --libs` -I/usr/include/gtk-3.0/unix-print
Review of attachment 206242 [details] [review]: Thanks for taking the time to write this patch, Timothy. Unfortunately, I can't apply it cleanly to gtk trunk and thus couldn't test it yet. Which version of gtk did you patch? I've made comments on some issues that I already noticed, anyway. In addition, the patch contains inconsistent indentation and trailing whitespace in some lines (the rest of the file isn't much better, but there's no need to introduce even more of that). ::: modules/printbackends/file/gtkprintbackendfile.c @@ +248,3 @@ } + + basename = g_strdup (gtk_print_settings_get (settings, GTK_PRINT_SETTINGS_OUTPUT_BASENAME)); No need to allocate memory for basename, just declare it as `const gchar *` @@ +257,3 @@ + { + name = g_strdup_printf (_("%s.%s"), basename, extension); + } When basename is a const string (see above), this can be rewritten as: if (basename == NULL) basename = _("output"); name = g_strdup_printf ("%s.%s", basename, extension);
Created attachment 208231 [details] [review] Patch to provide more flexable setting of filename, and directory v2 I have updated the patch with your suggestions, and removed any unneeded white spaces. However I'm not sure why you cannot apply the patch as I'm working in git head
Created attachment 208232 [details] [review] Patch to provide more flexable setting of filename, and directory v3 Forgot to update the last diff before uploading.
Created attachment 208233 [details] [review] Patch to provide more flexable setting of filename, and directory v4 Removed a couple more white spaces
Created attachment 208235 [details] [review] Patch to provide more flexable setting of filename, and directory v5 Ok, I've fixed the issue with not applying to trunk. I thought I had trunk but I mustn't have. Anyway I've tested my new patch against a fresh trunk and it applies.
Created attachment 208236 [details] [review] atch to provide more flexable setting of filename, and directory v6 Missed some of the changes in the last one
Review of attachment 208236 [details] [review]: thanks for your patch. it would be brilliant to have a proper git commit instead of a flat patch, so we can credit you properly. ::: modules/printbackends/file/gtkprintbackendfile.c @@ +251,3 @@ + basename = _("output"); + + name = g_strdup_printf ("%s.%s", basename, extension); you can use g_strconcat() here; it's easier, and doesn't hit the allocator as much: name = g_strconcat (basename, ".", extension, NULL); @@ +278,3 @@ + else + { + uri = g_build_filename (uri_dir, locale_name, NULL); you cannot really use g_build_filename() with a URI; I mean, the result may be correct on Linux and *nix, but the separator for files on Windows is \, which may lead to: file:///Users\Documents\foo\foo.text which is hardly a valid file:// URI. I'd make the settings a directory, instead of a URI, build the path to the file name, then convert it to a URI with g_filename_to_uri().
Created attachment 209053 [details] [review] Patch to provide more flexable setting of filename, and directory v7 Hi Emmanuele, Thanks for the review. I have included the suggestions you have made. I think the directory setting makes much more sense than a uri_directory. This is the first time I've committed to git and made a patch from it so let me know if I've done something wrong. Thanks for your help, Tim
Review of attachment 209053 [details] [review]: thanks for the patch; the commit log looks good. ::: modules/printbackends/file/gtkprintbackendfile.c @@ +278,3 @@ + else + { + uri = g_filename_to_uri (g_build_filename (output_dir, locale_name, NULL), NULL, NULL); this is incorrect: g_build_filename() will return a newly-allocated string; if you pass it to g_filename_to_uri(), which will make a copy of its first argument, you will leak the memory allocated for it. you have to separate the g_build_filename() from the g_filename_to_uri() calls.
Created attachment 209059 [details] [review] Patch to provide more flexable setting of filename, and directory v8 Sorry about that I've been programming in Java for the last six years.
Hi Emmanuele, Thanks for all your help. Is this likely to make it in before 3.4? Or do you think I will have to wait until 3.5?
Review of attachment 209059 [details] [review]: looks okay to me, now. thanks for updating the patch. if a gtk+ maintainer does a second review and thinks that the new settings are okay, it can be pushed to master/3.4.
Hi Timothy, thank you for the patch. It seems ok to me too. I've committed it to the master. But since the gtk+ 3.4 was already released in the mean time and the patch changes a localized string + it changes a header file I haven't committed it to the 3.4 branch. Regards Marek
Could someone please post the commit id of this patch here?
(In reply to comment #19) > Could someone please post the commit id of this patch here? 8d1f32aaafedce6326b0dbd837dc5503e889a60e
Javier, Marek or Timothy. could you have a look at bug #668529 and tell me if my patches conflict in anyway this this one? Im currently doing something like this: /* Preset the base name in the dialog */ existing_uri = gtk_print_settings_get (settings, GTK_PRINT_SETTINGS_OUTPUT_URI); if(existing_uri != NULL) gtk_file_chooser_set_uri(GTK_FILE_CHOOSER(dialog), existing_uri); which gets the current uri and uses it as a preset for the file chooser dialog.
Never mind about my previous comment, I just debugged it, and it appears as if everything works as supposed. I just had to change my patch a little: https://bugzilla.gnome.org/show_bug.cgi?id=668529#c16