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 788488 - GFile-based API for g_build_filename()
GFile-based API for g_build_filename()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-10-04 01:28 UTC by Cosimo Cecchi
Modified: 2017-11-07 16:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gio: add g_file_new_build_filename() convenience API (6.20 KB, patch)
2017-11-04 00:43 UTC, Cosimo Cecchi
none Details | Review
gfileutils: factor out g_build_filename_va() (1.83 KB, patch)
2017-11-04 19:03 UTC, Cosimo Cecchi
none Details | Review
gfile: add g_file_new_build_filename() (6.73 KB, patch)
2017-11-04 19:04 UTC, Cosimo Cecchi
none Details | Review
gfileutils: factor out g_build_filename_va() (1.83 KB, patch)
2017-11-06 20:49 UTC, Cosimo Cecchi
committed Details | Review
glib: add g_build_filename_valist() (2.49 KB, patch)
2017-11-06 20:49 UTC, Cosimo Cecchi
committed Details | Review
gfile: add g_file_new_build_filename() (5.06 KB, patch)
2017-11-06 20:49 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2017-10-04 01:28:27 UTC
I find myself writing a lot of code like

char *path = g_build_filename (...);
GFile *file = g_file_new_for_path (path);

It would be nice to have an e.g. g_file_build () API that combined the two for convenience.
Comment 1 Philip Withnall 2017-10-04 07:38:47 UTC
Agreed. Patch very much welcome, although I would perhaps call it g_file_new_build_path() instead so it gets recognised as a constructor in bindings.
Comment 2 Cosimo Cecchi 2017-11-04 00:43:07 UTC
Created attachment 362942 [details] [review]
gio: add g_file_new_build_filename() convenience API

This is equivalent to calling g_file_new_for_path() on the result of
g_build_filename(), and is provided for convenience to C clients.
Comment 3 Cosimo Cecchi 2017-11-04 00:44:15 UTC
Here's a first attempt; not exactly sure why it does not build, but posting here for initial feedback (and possibly help with the build issue!)
Comment 4 Colin Walters 2017-11-04 13:39:49 UTC
Review of attachment 362942 [details] [review]:

::: gio/gfile.h
@@ +609,3 @@
+GLIB_AVAILABLE_IN_2_56
+GFile *                 g_file_new_build_filename         (const gchar                *first_element,
+                                                           ...);

Wants G_GNUC_NULL_TERMINATED.
Comment 5 Cosimo Cecchi 2017-11-04 19:03:55 UTC
Created attachment 362987 [details] [review]
gfileutils: factor out g_build_filename_va()

This will be used in a later commit.
Comment 6 Cosimo Cecchi 2017-11-04 19:04:01 UTC
Created attachment 362988 [details] [review]
gfile: add g_file_new_build_filename()

This is a convenience C API that combines g_build_filename() with
g_file_new_for_path().
Comment 7 Philip Withnall 2017-11-06 11:36:10 UTC
Review of attachment 362987 [details] [review]:

Sure.
Comment 8 Philip Withnall 2017-11-06 11:46:22 UTC
Review of attachment 362988 [details] [review]:

I’m not so happy about needing to expose g_build_filename_va() as a private function between GLib and GIO. Could we add a proper g_build_filename_va(const gchar *first_argument, va_list args) which is public, and use that instead?

::: gio/gfile.c
@@ +6464,3 @@
+  gchar *str;
+  GFile *file;
+  va_list args;

Needs g_return_val_if_fail (first_element != NULL, NULL);

::: gio/tests/file.c
@@ +45,1 @@
+  file = g_file_new_build_filename (".", "some", "directory", "testfile", NULL);

Should probably also include a test where there’s just the first element in the filename, and the NULL terminator.

i.e. g_file_new_build_filename ("testfile", NULL);
Comment 9 Cosimo Cecchi 2017-11-06 20:49:39 UTC
Created attachment 363088 [details] [review]
gfileutils: factor out g_build_filename_va()

This will be used in a later commit.
Comment 10 Cosimo Cecchi 2017-11-06 20:49:44 UTC
Created attachment 363089 [details] [review]
glib: add g_build_filename_valist()

A new public API convenience to build a filename from a va_list.
Comment 11 Cosimo Cecchi 2017-11-06 20:49:52 UTC
Created attachment 363090 [details] [review]
gfile: add g_file_new_build_filename()

This is a convenience C API that combines g_build_filename() with
g_file_new_for_path().
Comment 12 Cosimo Cecchi 2017-11-06 20:50:24 UTC
Updated the patchset to make g_build_filename_valist() also available and public.
Comment 13 Philip Withnall 2017-11-06 21:45:27 UTC
Review of attachment 363088 [details] [review]:

Yup.
Comment 14 Philip Withnall 2017-11-06 21:45:27 UTC
Review of attachment 363089 [details] [review]:

Yes.
Comment 15 Philip Withnall 2017-11-06 21:45:34 UTC
Review of attachment 363090 [details] [review]:

For sure.
Comment 16 Cosimo Cecchi 2017-11-07 16:25:53 UTC
Attachment 363088 [details] pushed as 68d62c3 - gfileutils: factor out g_build_filename_va()
Attachment 363089 [details] pushed as 374ade1 - glib: add g_build_filename_valist()
Attachment 363090 [details] pushed as 44d6052 - gfile: add g_file_new_build_filename()