After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 657322 - GtkPrintSettings requires every application to work out file type
GtkPrintSettings requires every application to work out file type
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.1.x
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 587053
 
 
Reported: 2011-08-25 11:27 UTC by Timothy Arceri
Modified: 2012-08-22 08:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to provide more flexable setting of filename, and directory (3.05 KB, patch)
2012-01-27 08:10 UTC, Timothy Arceri
needs-work Details | Review
Small test program (2.54 KB, text/x-c++src)
2012-01-27 08:18 UTC, Timothy Arceri
  Details
Patch to provide more flexable setting of filename, and directory v2 (3.07 KB, patch)
2012-02-23 07:51 UTC, Timothy Arceri
none Details | Review
Patch to provide more flexable setting of filename, and directory v3 (2.92 KB, patch)
2012-02-23 08:07 UTC, Timothy Arceri
none Details | Review
Patch to provide more flexable setting of filename, and directory v4 (3.06 KB, patch)
2012-02-23 08:11 UTC, Timothy Arceri
none Details | Review
Patch to provide more flexable setting of filename, and directory v5 (1.81 KB, patch)
2012-02-23 08:45 UTC, Timothy Arceri
none Details | Review
atch to provide more flexable setting of filename, and directory v6 (3.21 KB, patch)
2012-02-23 08:55 UTC, Timothy Arceri
reviewed Details | Review
Patch to provide more flexable setting of filename, and directory v7 (4.13 KB, patch)
2012-03-06 08:37 UTC, Timothy Arceri
reviewed Details | Review
Patch to provide more flexable setting of filename, and directory v8 (4.14 KB, patch)
2012-03-06 10:20 UTC, Timothy Arceri
committed Details | Review

Description Timothy Arceri 2011-08-25 11:27:36 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
Comment 1 Timothy Arceri 2012-01-27 08:10:54 UTC
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.
Comment 2 Timothy Arceri 2012-01-27 08:11:53 UTC
The GTK_PRINT_SETTINGS_OUTPUT_URI setting could be depreciated in favor of the two new settings.
Comment 3 Timothy Arceri 2012-01-27 08:18:50 UTC
Created attachment 206243 [details]
Small test program

I'm attached a small program to test with.
Comment 4 Timothy Arceri 2012-01-27 08:20:00 UTC
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
Comment 5 Lars Karlitski 2012-02-22 17:37:35 UTC
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);
Comment 6 Lars Karlitski 2012-02-22 17:38:02 UTC
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);
Comment 7 Timothy Arceri 2012-02-23 07:51:22 UTC
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
Comment 8 Timothy Arceri 2012-02-23 08:07:19 UTC
Created attachment 208232 [details] [review]
Patch to provide more flexable setting of filename, and directory v3

Forgot to update the last diff before uploading.
Comment 9 Timothy Arceri 2012-02-23 08:11:50 UTC
Created attachment 208233 [details] [review]
Patch to provide more flexable setting of filename, and directory v4

Removed a couple more white spaces
Comment 10 Timothy Arceri 2012-02-23 08:45:30 UTC
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.
Comment 11 Timothy Arceri 2012-02-23 08:55:14 UTC
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
Comment 12 Emmanuele Bassi (:ebassi) 2012-03-05 15:34:33 UTC
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().
Comment 13 Timothy Arceri 2012-03-06 08:37:12 UTC
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
Comment 14 Emmanuele Bassi (:ebassi) 2012-03-06 08:46:52 UTC
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.
Comment 15 Timothy Arceri 2012-03-06 10:20:48 UTC
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.
Comment 16 Timothy Arceri 2012-03-13 03:53:38 UTC
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?
Comment 17 Emmanuele Bassi (:ebassi) 2012-03-13 07:27:29 UTC
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.
Comment 18 Marek Kašík 2012-05-14 12:32:36 UTC
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
Comment 19 Sebastian 2012-08-20 07:37:53 UTC
Could someone please post the commit id of this patch here?
Comment 20 Javier Jardón (IRC: jjardon) 2012-08-20 08:30:48 UTC
(In reply to comment #19)
> Could someone please post the commit id of this patch here?

8d1f32aaafedce6326b0dbd837dc5503e889a60e
Comment 21 Sebastian 2012-08-20 14:29:11 UTC
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.
Comment 22 Sebastian 2012-08-22 08:41:22 UTC
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