This is an experiment to use GtkFileChooserNative on Windows and macOS to address issues like
https://github.com/geany/geany-osx/issues/14
where the GTK dialogs don't offer the best user experience.
I kept using GtkFileChooser on Linux because it adds extra widgets to the dialog like file encoding and filetype selection and these aren't supported when using native dialogs. To me at least this isn't the most important thing and using native dialogs under Windows and macOS is more important IMO.
This leads to a few ifdefs in the code but I'd say the result isn't too bad (GtkFileChooser is a GtkWidget while GtkFileChooserNative isn't so some things have to differ). Before continuing with the save dialog and project open dialog, my question is whether something like that would be acceptable for Geany - @b4n @eht16 what do you think?
@eht16 Do the native dialogs work correctly on Windows with this patch? It seems to work alright on macOS. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3861
-- Commit Summary --
* Use GtkFileChooserNative for opening files on Windows and macOS
-- File Changes --
M src/dialogs.c (75)
-- Patch Links --
https://github.com/geany/geany/pull/3861.patch https://github.com/geany/geany/pull/3861.diff
I'm curious though as I only use Linux, it won't change anything for me :D. But I'll give it a try on Windows, this will take a few days though.
Somebody who has Windows 11 and can build Geany with this needs to test as well.
Also I presume the native dialogs are themed by the platform, not by GTK themes, so they may look totally different to the rest of Geany. @techee for Macos and whoever tests on Windows 11 need to try.
Somebody who has Windows 11 and can build Geany with this needs to test as well.
No need to build yourself, a full installer with the changes can be downloaded from the CI builds: https://github.com/geany/geany/actions/runs/8869590862
Windows 10/11 feedback would be great, I can only test on Windows 7 which is unsupported and not representative.
Windows 10/11 feedback would be great
So far so good on a stock Win 10 (22H2) VM with native dark mode active (i.e. `AppsUseLightTheme` == `0`):<sup>1.</sup>
<img width="800" alt="geany-2.1_ci_20240428183001_d3bb7fe-2024-04-29-151630" src="https://github.com/geany/geany/assets/59004801/ef76157a-d5ae-4111-b147-ce6a4ce91945"><br>
Also I presume the native dialogs are themed by the platform, not by GTK themes
The screen capture and Registry values below confirm as much.
At least it's not launching the XP-era file dialogue that caused #3209
--- <sup>1.</sup> ~~~sh-session $ reg query "HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes\Personalize" /se #
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes\Personalize ColorPrevalence REG_DWORD 0x0 EnableTransparency REG_DWORD 0x0 AppsUseLightTheme REG_DWORD 0x0 SystemUsesLightTheme REG_DWORD 0x0 ~~~
No need to build yourself, a full installer with the changes can be
downloaded from the CI builds: https://github.com/geany/geany/actions/runs/8869590862
Nice, I didn't know about that.
The screenshot below comes from Windows 11, Geany on the left, Notepad++ on the right. The only difference I see is the file type selection that Notepad++ offers. I ifdef'ed that out in this patch thinking it wouldn't work but maybe it does - I'll try that.
@techee pushed 1 commit.
2bf55c0d6657fa0a80ffd573316c47308d9ba581 Use filters with GtkFileChooserNative
File filters work both on Windows and macOS:
 <img width="833" alt="Screenshot 2024-04-30 at 0 11 00" src="https://github.com/geany/geany/assets/713965/03616e82-594b-4921-be10-ce999eb38167">
@rdipardo @techee thanks for testing, so apart from the inconsistent theming issue it seems to work.
@techee you said in the OP "where the GTK dialogs don't offer the best user experience.". Can you expand on that and why " using native dialogs under Windows and macOS is more important IMO."?
Didn't we have native dialogs a while ago, at least on Windows? So this brings that back, basically?
To me at least this isn't the most important thing and using native dialogs under Windows and macOS is more important IMO.
You hard-coded the native dialogs. As they don't offer all the features of our dialog (renaming a file, for example) you should be able to use that. I can also imagine not everyone likes an "alien" dialog in a GTK application (it's native to Windows but alien Geany).
Since I can't try either of the native dialogs, the reason I (politely) asked what the advantages of the native dialogs are is because as they are hard coded at compile time (AFAICT) users on Windows and Macos are gonna be stuck with them. So its important to know what the advantages are vs the disadvantages.
@techee you said in the OP "where the GTK dialogs don't offer the best user experience.". Can you expand on that and why " using native dialogs under Windows and macOS is more important IMO."?
It's pretty much this: https://github.com/geany/geany-osx/issues/14#issuecomment-1988461271. Basically the GTK keyboard shortcuts don't work on macOS and even if they worked, users don't know this because they are used to normal macOS behavior. Then it's the matter of favorite folders in the sidebar which one can normally define once and then they arte used in all the dialogs - that doesn't work with the GTK dialog. Finally, when using the native dialog, on macOS at least, the operating system automatically grants read/write "protected folder" file access without additional permission popup - this doesn't happen for GTK dialogs because they are "some foreign code" and the system doesn't know it was the user's explicit file selection.
Didn't we have native dialogs a while ago, at least on Windows? So this brings that back, basically?
Yes, but I think the problem wasn't the dialogs themselves (apart from the fact they were some "old" open dialogs), but rather the fact they were implemented using Windows API that nobody understood well. The API of `GtkFileChooserNative` is pretty much like `GtkFileChooser` except it isn't a widget and you cannot put custom widgets inside it.
You hard-coded the native dialogs.
This isn't meant to be the final implementation - as said in the PR description, I asked whether using them would be acceptable in Geany and implemented it only for the open dialog for some testing. I don't want to spend much time on something that nobody likes so here I just wanted some initial feedback. Making this feature optional is surely an option but if it's done that way, I'd lean towards using the native dialogs by default.
As they don't offer all the features of our dialog (renaming a file, for example)
I didn't even know it was possible to rename a file using the Open dialog - how does one do that? I was talking about these options:
<img width="878" alt="Screenshot 2024-04-30 at 15 21 11" src="https://github.com/geany/geany/assets/713965/46cf8725-cf6d-4acf-9cad-51c65eed02b7">
which are a custom widget inserted to the open dialog and which I personally have never used (I just tend to open the file and modify what's needed afterwards in Geany - especially with file encoding one cannot be sure before seeing how the file displays).
I didn't even know it was possible to rename a file using the Open dialog - how does one do that?
OK, I guess you meant the Save as dialog where the Rename button is the added widget, which, indeed, won't be present in the native dialogs.
Yes, but I think the problem wasn't the dialogs themselves (apart from the fact they were some "old" open dialogs), but rather the fact they were implemented using Windows API that nobody understood well.
And #3209 crash.
But one thing positive they did was that they automounted remote servers IIRC, @techee @rdipardo do you have a configuration to check that on windows with the new native filechooser?
Like @kugel, I didn't understand that this was WIP (label added) and would be selectable by the user in the final PR, not compiled in. In that case its less of a problem that there are functionality and theme differences, the user can choose. It will need to be explained properly in the manual why this happens with a section head that I can link to and close the issue when users complain :smiling_imp:
The native file dialog on Windows 7 works very well after some testing. Non-ASCII path and file names seem to work well, too. 
I don't know how to set up "automounted remote servers", so can't test it. But what the native dialog offers is access to network drives, I tested it with a file on a network drive added via the RDP client (I think this uses SMB under the hood) and this works fine while the GTK dialog does not see those drives.
To me it seems fine to use those dialogs since it seems to work well and if it goes the same for "Save As" and maybe "Open Folder". Making it configurable is a good idea, I don't mind about the default. Most of the infrastructure for the setting we already had (see #3219 and #3791) which could be revived.
To me it seems fine to use those dialogs since it seems to work well and if it goes the same for "Save As" and maybe "Open Folder". Making it configurable is a good idea, I don't mind about the default.
Good to hear. I'll try to prepare the patches (after finishing the boring task of writing documentation for the lsp plugin, sigh).
Maybe one more question - should the open dialog show hidden files by default? This is configurable using the custom widget on GTK but we won't be able to add it so we should decide either to show hidden files or not. On macOS hidden files can be shown using some keyboard shortcut for the dialog so it wouldn't be completely necessary to always show hidden files, I'm not sure if it's possible on Windows too though (on the other hand Windows hidden files aren't so important I think because it's not those beginning with `.` like `.gitignore` that typical developer needs to edit).
If there's an option for it, sure (I didn't understand that was something you would be happy about). And I don't mind the default on non-Linux either, whatever people like best on those platforms. It would be nice to have this available on Linux as well (**but off by default**) for the sandboxing caps -- although I'm not really sure whether "sandboxing" an editor/IDE really makes much sense, but well, it has been mentioned quite a few times.
File filters work both on Windows and macOS
That's nice. And indeed, the [documentation](https://docs.gtk.org/gtk3/class.FileChooserNative.html#win32-details-gtkfile...) lead me to think it wouldn't work with out patterns, but [custom filters](https://docs.gtk.org/gtk3/method.FileFilter.add_custom.html) are actually something else.
Maybe one more question - should the open dialog show hidden files by default? [...] I'm not sure if it's possible on Windows too though (on the other hand Windows hidden files aren't so important I think because it's not those beginning with `.` like `.gitignore` that typical developer needs to edit).
I don't think it's necessary because as you said, hidden files on Windows are usually less relevant and have stronger means than on Linux. The dialog in the current configuration shows files like `.gitignore` and this is good, I think.
@techee pushed 3 commits.
91fb8850281979356cdc38b5ea2b97e1b7132461 Use GtkFileChooserNative depending on user configuration ee8f885acf554c494b00f0f3a80a3c17676838ca Remove usage of windows-native dialogs from ui_utils.c and win32.c/h 4dff3767b9f80719dab3d1a83443be52cd377fa8 Use GtkFileChooserNative in ui_utils.c
@techee pushed 1 commit.
2d8d4f2edacbfc82743a4d020d9c22bc6526b92b Use GTK_RESPONSE_ACCEPT which is also used by GtkFileChooserNative
Alright, I made the suggested changes: 1. I made GtkFileChooserNative configurable, by default enabled on macOS and Windows, disabled on Linux. 2. I went through all GtkFileChooser occurrences in Geany and added the native variant 3. Not sure if intentional or if it's some left-over but `ui_utils.c` still contains native win32 dialogs: https://github.com/geany/geany/blob/11b4a00a3020b1c9ace3d3ae65aa5ec7d5ff84e0... I replaced those with GtkFileChooserNative and eliminated the native code in win32.c. 4. For now I didn't modify plugins which also contain some GtkFileChooser code but I could do the same for them too if desired.
Alright, I made the suggested changes:
1. I made GtkFileChooserNative configurable, by default enabled on macOS and Windows, disabled on Linux.
Sweet, works perfectly.
3. Not sure if intentional or if it's some left-over but `ui_utils.c` still contains native win32 dialogs: https://github.com/geany/geany/blob/11b4a00a3020b1c9ace3d3ae65aa5ec7d5ff84e0/src/ui_utils.c#L2012 I replaced those with GtkFileChooserNative and eliminated the native code in win32.c.
I guess it's some sort of left-over because it wasn't as buggy as the full file dialogs and so they were out of my focus. Thanks for cleaning up my dirt :D.
4. For now I didn't modify plugins which also contain some GtkFileChooser code but I could do the same for them too if desired.
I think this improve the overall user experience. Though maybe this can be done later as well and maybe also by the respective plugin maintainers (at least for those plugins which still have an active maintainer).
I guess it's some sort of left-over because it wasn't as buggy as the full file dialogs and so they were out of my focus.
Thanks for cleaning up my dirt :D.
By the way, there's also native message dialog on Windows (`win32_message_dialog()`) called at several places inside `dialog.c` (hard-coded, not configurable) - is this one useful for something? Unlike open file dialogs, this is just a simple popup and doesn't bring any benefits and could be removed too IMO (it could actually be quite distracting if someone uses dark Windows theme and Geany is in light theme and the dark popup appears).
I think this improve the overall user experience. Though maybe this can be done later as well and maybe also by the respective plugin maintainers (at least for those plugins which still have an active maintainer).
I meant the built in plugins here - notably saveactions and export use GtkFileChooser now. I can update those to use native dialogs too. For geany-plugins it's up to the plugin maintainers I think.
@techee pushed 1 commit.
26a1143fe9ca55f1b2172569598b5b4d129a807e Convert the saveactions plugin to use native dialogs
I've modified the saveactions plugin to use the native dialogs.
The only missing place in Geany itself is the export plugin which I think is the only place where right now the custom widget contains some essential configuration: <img width="734" alt="Screenshot 2024-06-08 at 11 47 20" src="https://github.com/geany/geany/assets/713965/5a43b5c8-b301-45e1-a146-291f002c67a9">
I think modifying this to the native dialog would limit the ability of the plugin so I skipped it for now. In the future, the UI could be modified to show a popup with the settings before showing the file dialog.
@techee pushed 1 commit.
cf8c5c52d794ce39fc951075435689671d7f6af0 Make strings translatable
@b4n commented on this pull request.
Changes look pretty good, see inline comments.
Seems to work nicely as well. I didn't test it entirely thoroughly, but basic open/save/select folder seem to work find with or without the option on Linux.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property> + <property name="visible">True</property> + <property name="can-focus">True</property> + <property name="receives-default">False</property> + <property name="tooltip-text" translatable="yes">Defines whether to use the platform-native file dialogs or whether to use the GTK default dialogs</property>
We need asking @elextr, but wouldn't it be better English worded without the second "whether"? *Defines whether to use the platform-native file dialogs or the GTK default dialogs*
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property>
Could you please add a mnemonic? It's sometimes tricky to find one that's not used yet (here in LANGUAGE=C), but it's really nice to have them :)
I see other items around also lack it, but we could start now :) (or I'll try and do that after this landed)
- if (GTK_IS_WIDGET(dialog))
+ return gtk_dialog_run(GTK_DIALOG(dialog)); + + return gtk_native_dialog_run(GTK_NATIVE_DIALOG(dialog));
Wouldn't it make more sense to do something like: ```suggestion if (GTK_IS_NATIVE_DIALOG(dialog)) return gtk_native_dialog_run(GTK_NATIVE_DIALOG(dialog)); else return gtk_dialog_run(GTK_DIALOG(dialog)); ```
Or at least `GTK_IS_DIALOG()`, as you'd then could reasonably expect `gtk_dialog_run()` to work on it. My point is mostly that this relies on the fact `GtkNativeDialog` isn't a `GtkWidget`, and although it seems to make sense I wouldn't rule out a fallback implementation to be one at some point, but not necessarily be a `GtkDialog` (could be a window with a custom `gtk_native_dialog_run()` implementation).
Same for the other occurrence of this.
- gtk_dialog_set_default_response(GTK_DIALOG(dialog), GTK_RESPONSE_ACCEPT);
- - gtk_widget_set_size_request(dialog, -1, 460); - gtk_window_set_modal(GTK_WINDOW(dialog), TRUE); - gtk_window_set_destroy_with_parent(GTK_WINDOW(dialog), TRUE); - gtk_window_set_skip_taskbar_hint(GTK_WINDOW(dialog), FALSE); - gtk_window_set_type_hint(GTK_WINDOW(dialog), GDK_WINDOW_TYPE_HINT_DIALOG); - gtk_window_set_transient_for(GTK_WINDOW(dialog), GTK_WINDOW(main_widgets.window)); - gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), TRUE); - gtk_file_chooser_set_local_only(GTK_FILE_CHOOSER(dialog), FALSE); - - /* add checkboxes and filename entry */ - gtk_file_chooser_set_extra_widget(GTK_FILE_CHOOSER(dialog), add_file_open_extra_widget(dialog)); + if (interface_prefs.use_native_windows_dialogs) + dialog = GTK_FILE_CHOOSER(gtk_file_chooser_native_new(_("Open File"), + GTK_WINDOW(main_widgets.window), GTK_FILE_CHOOSER_ACTION_OPEN, _("_Open"), _("_Cancel")));
Any reason not to use the default labels here? They seem to be Open/Cancel just the same, doesn't them? Or is it problematic on macos/Windows? At least it works great on Linux, and avoids having additional strings here. Moreover, I would think leaving them default would help with platform consistency (which is the goal here, isn't it?) -- unless again if it's really problematic, but I'd be surprised it'd be given the basic nature of our use cases.
Same applies to all other uses.
```suggestion GTK_WINDOW(main_widgets.window), GTK_FILE_CHOOSER_ACTION_OPEN, NULL, NULL)); ```
@@ -461,18 +490,19 @@ void dialogs_show_open_file(void)
SETPTR(initdir, utils_get_locale_from_utf8(initdir));
dialog = create_open_file_dialog(); - open_file_dialog_apply_settings(dialog); + if (GTK_IS_WIDGET(dialog))
maybe here too
I think modifying this to the native dialog would limit the ability of the plugin so I skipped it for now.
Yeah makes sense.
In the future, the UI could be modified to show a popup with the settings before showing the file dialog.
I think that would be pretty bad UI… better re-think it a bit then. Maybe an option dialog with a GtkFileChooserButton with a reasonable default if the destination is not necessarily changed every time? Or have the prefs easily accessible if it makes more sense, etc. But two consecutive dialogs don't strike me as a nice UI :)
Anyway, that's separate question.
@techee commented on this pull request.
- if (GTK_IS_WIDGET(dialog))
+ return gtk_dialog_run(GTK_DIALOG(dialog)); + + return gtk_native_dialog_run(GTK_NATIVE_DIALOG(dialog));
OK.
@techee commented on this pull request.
- gtk_dialog_set_default_response(GTK_DIALOG(dialog), GTK_RESPONSE_ACCEPT);
- - gtk_widget_set_size_request(dialog, -1, 460); - gtk_window_set_modal(GTK_WINDOW(dialog), TRUE); - gtk_window_set_destroy_with_parent(GTK_WINDOW(dialog), TRUE); - gtk_window_set_skip_taskbar_hint(GTK_WINDOW(dialog), FALSE); - gtk_window_set_type_hint(GTK_WINDOW(dialog), GDK_WINDOW_TYPE_HINT_DIALOG); - gtk_window_set_transient_for(GTK_WINDOW(dialog), GTK_WINDOW(main_widgets.window)); - gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), TRUE); - gtk_file_chooser_set_local_only(GTK_FILE_CHOOSER(dialog), FALSE); - - /* add checkboxes and filename entry */ - gtk_file_chooser_set_extra_widget(GTK_FILE_CHOOSER(dialog), add_file_open_extra_widget(dialog)); + if (interface_prefs.use_native_windows_dialogs) + dialog = GTK_FILE_CHOOSER(gtk_file_chooser_native_new(_("Open File"), + GTK_WINDOW(main_widgets.window), GTK_FILE_CHOOSER_ACTION_OPEN, _("_Open"), _("_Cancel")));
Ah, I didn't notice one can pass NULL here for defaults, will do (and test).
@techee commented on this pull request.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property> + <property name="visible">True</property> + <property name="can-focus">True</property> + <property name="receives-default">False</property> + <property name="tooltip-text" translatable="yes">Defines whether to use the platform-native file dialogs or whether to use the GTK default dialogs</property>
Yours sounds better but let's wait for the expert :-)
@techee commented on this pull request.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property>
How can one even show the mnemonics for the dialog items? For menus, Alt seems to work, but not for dialog entries. And isn't it a bit an overkill as this is pretty much set once?
@techee pushed 2 commits.
da3442e2172c901762a74098655e0b1855b977a1 Use default labels in native dialogs 4b1be9a5776d1ca125a2b70177b618acee9b9db5 Use GTK_IS_NATIVE_DIALOG() in tests instead of GTK_IS_WIDGET()
@b4n commented on this pull request.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property>
Alt does work, but actually it seems out prefs dialog basically only has them for the *Editor → Indentation* tab, so ignore this for now.
As for whether it's useful or not… certainly less than in menus or buttons indeed, but so assistive technologies can report accessors of this kind which help some people. Likely just a tiny bonus in this case though, but it's not too hard for us to do (although now that I see it's basically not done, it'd affect a lot of translations…).
@techee commented on this pull request.
- gtk_dialog_set_default_response(GTK_DIALOG(dialog), GTK_RESPONSE_ACCEPT);
- - gtk_widget_set_size_request(dialog, -1, 460); - gtk_window_set_modal(GTK_WINDOW(dialog), TRUE); - gtk_window_set_destroy_with_parent(GTK_WINDOW(dialog), TRUE); - gtk_window_set_skip_taskbar_hint(GTK_WINDOW(dialog), FALSE); - gtk_window_set_type_hint(GTK_WINDOW(dialog), GDK_WINDOW_TYPE_HINT_DIALOG); - gtk_window_set_transient_for(GTK_WINDOW(dialog), GTK_WINDOW(main_widgets.window)); - gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), TRUE); - gtk_file_chooser_set_local_only(GTK_FILE_CHOOSER(dialog), FALSE); - - /* add checkboxes and filename entry */ - gtk_file_chooser_set_extra_widget(GTK_FILE_CHOOSER(dialog), add_file_open_extra_widget(dialog)); + if (interface_prefs.use_native_windows_dialogs) + dialog = GTK_FILE_CHOOSER(gtk_file_chooser_native_new(_("Open File"), + GTK_WINDOW(main_widgets.window), GTK_FILE_CHOOSER_ACTION_OPEN, _("_Open"), _("_Cancel")));
Just tested and it works both on macOS and Windows and also picks the correct localization.
@elextr commented on this pull request.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property> + <property name="visible">True</property> + <property name="can-focus">True</property> + <property name="receives-default">False</property> + <property name="tooltip-text" translatable="yes">Defines whether to use the platform-native file dialogs or whether to use the GTK default dialogs</property>
b4n's version is perhaps slightly better (and word shorter) but either is understandable.
@techee pushed 1 commit.
2337e3e9e4334b8c9ce44460b6f6f8ed94ab2986 Reword help tooltip for the native dialogs
@techee commented on this pull request.
@@ -1311,6 +1311,22 @@
<property name="position">3</property> </packing> </child> + <child> + <object class="GtkCheckButton" id="check_native_dialogs"> + <property name="label" translatable="yes">Use platform-native file dialogs</property> + <property name="visible">True</property> + <property name="can-focus">True</property> + <property name="receives-default">False</property> + <property name="tooltip-text" translatable="yes">Defines whether to use the platform-native file dialogs or whether to use the GTK default dialogs</property>
Renamed to the b4n's version.
@b4n commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
Can't we keep the active filter with the native dialogs? This seem to at least work with GTK portal implementation:
```diff diff --git a/src/dialogs.c b/src/dialogs.c index 2919f325b..150e28846 100644 --- a/src/dialogs.c +++ b/src/dialogs.c @@ -140,10 +140,10 @@ static gboolean open_file_dialog_handle_response(GtkFileChooser *dialog, gint re GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo");
filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); filesel_state.open.encoding_idx = ui_encodings_combo_box_get_active_encoding(GTK_COMBO_BOX(encoding_combo)); } + filesel_state.open.filter_idx = file_chooser_get_filter_idx(dialog);
/* ignore detect from file item */ if (filesel_state.open.filetype_idx >= 0) @@ -429,29 +429,34 @@ static GtkFileChooser *create_open_file_dialog(void) }
-static void open_file_dialog_apply_settings(GtkWidget *dialog) +static void open_file_dialog_apply_settings(GtkFileChooser *dialog) { static gboolean initialized = FALSE; - GtkWidget *check_hidden = ui_lookup_widget(dialog, "check_hidden"); - GtkWidget *filetype_combo = ui_lookup_widget(dialog, "filetype_combo"); - GtkWidget *encoding_combo = ui_lookup_widget(dialog, "encoding_combo"); - GtkWidget *expander = ui_lookup_widget(dialog, "more_options_expander");
/* we can't know the initial position of combo boxes, so retrieve it the first time */ if (! initialized) { - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(dialog);
initialized = TRUE; } else { - file_chooser_set_filter_idx(GTK_FILE_CHOOSER(dialog), filesel_state.open.filter_idx); + file_chooser_set_filter_idx(dialog, filesel_state.open.filter_idx); + } + + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *check_hidden = ui_lookup_widget(GTK_WIDGET(dialog), "check_hidden"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + + gtk_expander_set_expanded(GTK_EXPANDER(expander), filesel_state.open.more_options_visible); + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(check_hidden), filesel_state.open.show_hidden); + ui_encodings_combo_box_set_active_encoding(GTK_COMBO_BOX(encoding_combo), filesel_state.open.encoding_idx); + filetype_combo_box_set_active_filetype(GTK_COMBO_BOX(filetype_combo), filesel_state.open.filetype_idx); } - gtk_expander_set_expanded(GTK_EXPANDER(expander), filesel_state.open.more_options_visible); - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(check_hidden), filesel_state.open.show_hidden); - ui_encodings_combo_box_set_active_encoding(GTK_COMBO_BOX(encoding_combo), filesel_state.open.encoding_idx); - filetype_combo_box_set_active_filetype(GTK_COMBO_BOX(filetype_combo), filesel_state.open.filetype_idx); }
@@ -490,8 +495,7 @@ void dialogs_show_open_file(void) SETPTR(initdir, utils_get_locale_from_utf8(initdir));
dialog = create_open_file_dialog(); - if (!GTK_IS_NATIVE_DIALOG(dialog)) - open_file_dialog_apply_settings(GTK_WIDGET(dialog)); + open_file_dialog_apply_settings(dialog);
if (initdir != NULL && g_path_is_absolute(initdir)) gtk_file_chooser_set_current_folder(dialog, initdir); ```
@b4n commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
FWIW, the *show hidden file* would also be implemented that way, but isn't actually needed for the GTK portal implementation because it remembers this itself. Not sure if it's the same for Windows and macos? If it's not, it's easy enough to add.
@techee pushed 1 commit.
7a259e8ed7e76f3861d192afa083dae78de44e7b Apply Colomban's patch allowing to remember selected file filter
Also, I don't know if it's a limitation of the XDG portal protocol or an xdg-desktop-portals implementation quirk, but they reject empty file filters (not showing the dialog at all, which is bad). It's unlikely to happen because *usually* a filetype has a corresponding non-empty entry in *filetype_extensions.conf*, but that's not enforced, and I for example stumbled upon this with a leftover filetype from a former test run.
I suggest handling this case in this PR as without it it's not possible to use XDG portals, and with it and a "bad" config (and `GTK_USE_PORTAL=1`) it breaks selecting a file: ```diff diff --git a/src/dialogs.c b/src/dialogs.c index 2919f325b..d1fa65a54 100644 --- a/src/dialogs.c +++ b/src/dialogs.c @@ -372,6 +372,7 @@ static GtkWidget *add_file_open_extra_widget(GtkWidget *dialog) static GtkFileChooser *create_open_file_dialog(void) { GtkFileChooser *dialog; + GtkFileFilter *filter; GtkWidget *viewbtn; GSList *node;
@@ -408,18 +409,19 @@ static GtkFileChooser *create_open_file_dialog(void) }
/* add FileFilters(start with "All Files") */ - gtk_file_chooser_add_filter(dialog, - filetypes_create_file_filter(filetypes[GEANY_FILETYPES_NONE])); + if ((filter = filetypes_create_file_filter(filetypes[GEANY_FILETYPES_NONE]))) + gtk_file_chooser_add_filter(dialog, filter); /* now create meta filter "All Source" */ - gtk_file_chooser_add_filter(dialog, - filetypes_create_file_filter_all_source()); + if ((filter = filetypes_create_file_filter_all_source())) + gtk_file_chooser_add_filter(dialog, filter); foreach_slist(node, filetypes_by_title) { GeanyFiletype *ft = node->data;
if (G_UNLIKELY(ft->id == GEANY_FILETYPES_NONE)) continue; - gtk_file_chooser_add_filter(dialog, filetypes_create_file_filter(ft)); + if ((filter = filetypes_create_file_filter(ft))) + gtk_file_chooser_add_filter(dialog, filter); }
gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), TRUE); diff --git a/src/filetypes.c b/src/filetypes.c index b1913e368..883201352 100644 --- a/src/filetypes.c +++ b/src/filetypes.c @@ -1187,6 +1187,7 @@ GtkFileFilter *filetypes_create_file_filter_all_source(void) { GtkFileFilter *new_filter; guint i, j; + guint n_patterns = 0;
new_filter = gtk_file_filter_new(); gtk_file_filter_set_name(new_filter, _("All Source")); @@ -1200,7 +1201,16 @@ GtkFileFilter *filetypes_create_file_filter_all_source(void) { gtk_file_filter_add_pattern(new_filter, filetypes[i]->pattern[j]); } + n_patterns += j; } + + /* very unlikely case where there is *no* patterns at all */ + if (n_patterns == 0) + { + g_object_unref(new_filter); + new_filter = NULL; + } + return new_filter; }
@@ -1213,6 +1223,13 @@ GtkFileFilter *filetypes_create_file_filter(const GeanyFiletype *ft)
g_return_val_if_fail(ft != NULL, NULL);
+ /* unlikely case where the ft has no patterns */ + if (! ft->pattern[0]) + { + g_debug("Not creating filter for filetype %s that has no pattern", ft->name); + return NULL; + } + new_filter = gtk_file_filter_new(); title = ft->id == GEANY_FILETYPES_NONE ? _("All files") : ft->title; gtk_file_filter_set_name(new_filter, title); ```
PS: I can add a commit with this here if you prefer
@techee commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
So I've just tested this both on macOS and Windows and it doesn't work there unfortunately. I'd say it's not a big deal - would be just nice if it did.
@b4n commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
@techee which part, remembering the current filter or the hidden files setting?
@techee commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
Remembering the filter doesn't work.
@b4n commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
OK. Sad (as we try to keep it) but not the end of the world indeed. If it doesn't break anything I'd keep it for the sake of implementations that do support it.
@techee commented on this pull request.
gboolean ro = (response == GEANY_RESPONSE_VIEW); /* View clicked */
- filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); - filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog)); - filesel_state.open.filetype_idx = filetype_combo_box_get_active_filetype(GTK_COMBO_BOX(filetype_combo)); + if (!GTK_IS_NATIVE_DIALOG(dialog)) + { + GtkWidget *expander = ui_lookup_widget(GTK_WIDGET(dialog), "more_options_expander"); + GtkWidget *filetype_combo = ui_lookup_widget(GTK_WIDGET(dialog), "filetype_combo"); + GtkWidget *encoding_combo = ui_lookup_widget(GTK_WIDGET(dialog), "encoding_combo"); + + filesel_state.open.more_options_visible = gtk_expander_get_expanded(GTK_EXPANDER(expander)); + filesel_state.open.filter_idx = file_chooser_get_filter_idx(GTK_FILE_CHOOSER(dialog));
Agreed.
I suggest handling this case in this PR as without it it's not possible to use XDG portals, and with it and a "bad" config (and GTK_USE_PORTAL=1) it breaks selecting a file:
Looks good.
PS: I can add a commit with this here if you prefer
Should I squash some of the previous commits or do you want to keep it as it is? In any case, I think you can add your commit so I for once can't pretend it's my own work :-)
@techee pushed 1 commit.
f1a2870609d855c4ba3acbfc341bb314cc96b687 Remove some more win32 code that was previously used by native dialogs
I just noticed some more dead code inside `win32.c` that was used by the native implementation so I removed it.
@b4n pushed 1 commit.
3106a938506c3b73e775d055025fb774aa828b80 Do not create empty file filters
@b4n approved this pull request.
LGTM
LGTM
OK to merge as it is or do some squashing?
@techee merging as is is fine here I think, the history could be cleaned but it's not bad either. So I'd rather not worry about squashing smartly, which might prove harder than necessary.
(getting a build could be nice, but apparently GA has had some issues…)
Its stuck on download from msys2.org.
@elextr and yesterday it was just not running anything, but a retry a bit later worked for the non-windows jobs…
Neat, improvement!! :grinning:
Merged #3861 into master.
But finally msys2 downloads work now and it builds alright. Merging.
I just noticed some more dead code inside `win32.c` that was used by the native implementation so I removed it.
Love it that you clean up the (probably mine) old win32 code :heart:
github-comments@lists.geany.org