GNOME Bugzilla – Bug 735886
OS native file dialogs
Last modified: 2014-09-06 09:58:01 UTC
The gtk+ dialogs on OS X look very out of place, and generally offer a significantly worse user experience than the native dialogs.
Created attachment 285134 [details] [review] Implement native OS open/save dialogs This patch implements native file open/save dialogs for OS X.
Review of attachment 285134 [details] [review]: some minor nitpicks, I trust you on the code part ::: gedit/gedit-encoding-items.c @@ +1,1 @@ +#include "gedit-encoding-items.h" copyright @@ +13,3 @@ +static GeditEncodingItem * +gedit_encoding_item_new (const GtkSourceEncoding *encoding, + gchar *name) api wise this should probably be const @@ +18,3 @@ + + item->encoding = encoding; + item->name = name; this should be g_strdup if we go for const @@ +19,3 @@ + item->encoding = encoding; + item->name = name; + return item; newline before return ::: gedit/gedit-encoding-items.h @@ +31,3 @@ + +void gedit_encoding_item_free (GeditEncodingItem *item); +const GtkSourceEncoding *gedit_encoding_item_get_encoding (GeditEncodingItem *item); wrong indentation? could be bz showing it wrong ::: gedit/gedit-file-chooser-dialog-gtk.c @@ +407,3 @@ + gtk_file_chooser_set_extra_widget (GTK_FILE_CHOOSER (dialog), + dialog->priv->extra_widget); + remove line
Review of attachment 285134 [details] [review]: ::: gedit/gedit-encoding-items.c @@ +13,3 @@ +static GeditEncodingItem * +gedit_encoding_item_new (const GtkSourceEncoding *encoding, + gchar *name) I had this const before, but then noticed that everywhere calling this I needed to free "name", so I took this "shortcut". As it is internal API, I think it's ok. But if you prefer to have it const + strdup that's fine by me.
Created attachment 285146 [details] [review] Implement native OS open/save dialogs Thanks for the quick comments and catches. New patch should fix those. Let me know if you want the const/strdup thingie to be changed.
Review of attachment 285146 [details] [review]: ok I guess we can go for what you have right now, just add the missing modeline. ::: gedit/gedit-encoding-items.c @@ +131,3 @@ + + return g_slist_reverse (ret); +} missing modeline
Created attachment 285148 [details] [review] Implement native OS open/save dialogs Added missing modeline
Review of attachment 285148 [details] [review]: ::: gedit/gedit-commands-file.c @@ -451,5 +451,5 @@ - open_dialog = gedit_file_chooser_dialog_new (_("Open"), - _("_Cancel"), GTK_RESPONSE_CANCEL, - window ? GTK_WINDOW (window) : NULL, ... 2 more ... + GEDIT_FILE_CHOOSER_OPEN | + window ? GTK_WINDOW (window) : NULL, + GEDIT_FILE_CHOOSER_ENABLE_ENCODING, ... 2 more ... Would it be possibile to keep the label/response pairs vararg in the API or is it hard to do for OSX? I'd prefer to keep the it in the caller so that it is clear which response value is used for each button ::: gedit/gedit-encoding-items.c @@ +27,3 @@ +struct _GeditEncodingItem +{ + const GtkSourceEncoding *encoding; I know it is far from a real performance/memory concern, but duplicating the string for the name of each encoding just to have a custom name for one or two seems a bit wasteful... Maybe we can just return a list of GtkSourceEncodings and then have an util gedit_encoding_get_display_name(GtkSourceEncoding *enc) that returns the custom name for some specific values of the enc pointer that we saved while building the list. The list is static anyway, no? As a purely cosmetic comment I do not like the "-items" in the file name a lot... I guess I'd just call it gedit-encodings.[ch] ::: gedit/gedit-file-chooser-dialog-osx.c @@ +69,3 @@ + NSString *title; + + switch (type) maybe factor out a get_new_line_type_label in the parent class so that we do not need to keep in sync the labels if they change? Though that's pretty unlikely... ::: gedit/gedit-file-chooser-dialog.h @@ -29,3 +27,3 @@ G_BEGIN_DECLS -#define GEDIT_TYPE_FILE_CHOOSER_DIALOG (gedit_file_chooser_dialog_get_type ()) +/* I think we have been phasing out these kind of comments from the header since they do not add much value
Review of attachment 285148 [details] [review]: ::: gedit/gedit-commands-file.c @@ -451,5 +451,5 @@ - open_dialog = gedit_file_chooser_dialog_new (_("Open"), - _("_Cancel"), GTK_RESPONSE_CANCEL, - window ? GTK_WINDOW (window) : NULL, ... 2 more ... + GEDIT_FILE_CHOOSER_OPEN | + window ? GTK_WINDOW (window) : NULL, + GEDIT_FILE_CHOOSER_ENABLE_ENCODING, ... 2 more ... On OS X we can't control the buttons, so the API translation to OS X would be kind of weird. I didn't see a reason for it, since we don't use this kind of dialog anywhere else, with another kind of configuration. We could still supply the response types, but to be honest I don't really see much point in it. ::: gedit/gedit-encoding-items.c @@ +27,3 @@ +struct _GeditEncodingItem +{ + const GtkSourceEncoding *encoding; The list is "dynamic", based on current locale and gsettings stuff. I really think that doing the dup is _a lot_ cleaner than keeping some magic pointers around and mapping those. This is only used in the file open dialog, there are only going to be a handful of encodings, and if you really want to optimize something I think it's the wrong place :) I can rename the files if you'd like. ::: gedit/gedit-file-chooser-dialog-osx.c @@ +69,3 @@ + NSString *title; + + switch (type) will do ::: gedit/gedit-file-chooser-dialog.h @@ -29,3 +27,3 @@ G_BEGIN_DECLS -#define GEDIT_TYPE_FILE_CHOOSER_DIALOG (gedit_file_chooser_dialog_get_type ()) +/* Fair enough, I guess I copy/pasted it. Will remove.
Turns out that we can set the label for the Open button, but not for the Cancel button. I know, common denominator things suck. I'll try to rework it so that we can keep the configuration of the two buttons as before (but probably without varargs, just two buttons).
> Turns out that we can set the label for the Open button, but not for the Cancel > button. I know, common denominator things suck. I'll try to rework it so that > we can keep the configuration of the two buttons as before (but probably > without varargs, just two buttons). Well, it is not that important, I just asked if it was possible without going crazy :-) It was juts because so it was clearer that tha caller code should handle GTK_RESPONSE_CANCEL instead of GTK_RESPONSE_CLOSE etc but we can survive. > The list is "dynamic", based on current locale and gsettings stuff. I really > think that doing the dup is _a lot_ cleaner than keeping some magic pointers > around and mapping those. This is only used in the file open dialog, there are > only going to be a handful of encodings, and if you really want to optimize > something I think it's the wrong place :) I can rename the files if you'd like. > Yeah, I said that I know it was not really an optimization, it is just that bugs me that we just moved this stuff to gsv and we already have to wrap it :-) Btw, to avoid some of the duplicated string you could set the internal name to null for all the items that do not have a custom name and then to the if (null) { return gsv_encoding_name() } in the getter Anyway, not really important...
(In reply to comment #10) > > Turns out that we can set the label for the Open button, but not for the Cancel > > button. I know, common denominator things suck. I'll try to rework it so that > > we can keep the configuration of the two buttons as before (but probably > > without varargs, just two buttons). > > Well, it is not that important, I just asked if it was possible without going > crazy :-) > > It was juts because so it was clearer that tha caller code should handle > GTK_RESPONSE_CANCEL instead of GTK_RESPONSE_CLOSE etc but we can survive. Ok, I've pushed this in b87dd7e8d2015923a2ff6940a63f77f85ed1f670. I've also pushed some further enhancements and made the style scheme chooser use the native dialog. I've also removed those comments. I think we can close this now, unless you still want I do something with the encoding items?
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.