Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove native file and color dialogs on Windows #3219

Merged
merged 1 commit into from Sep 17, 2022

Conversation

eht16
Copy link
Member

@eht16 eht16 commented Jun 12, 2022

Their implementation was buggy, use very old Windows APIs and require
double implementation and maintenance efforts for us
The GTK dialogs work well for all other users already, so they probably
will also for Windows users.

Closes #3209.

@@ -63,7 +63,7 @@ typedef struct GeanyInterfacePrefs
gboolean msgwin_compiler_visible; /**< whether message window's compiler tab is visible */
gboolean msgwin_messages_visible; /**< whether message window's messages tab is visible */
gboolean msgwin_scribble_visible; /**< whether message window's scribble tab is visible */
/** whether to use native Windows' dialogs (only used on Windows) */
/** whether to use native Windows' dialogs - ignored and not used anymore */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this field is part of the API (and ABI), we cannot easily remove it.

Is it enough to mark it as ignored in the comment or are there better ways to handle this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suspect that there are not many Windows specific plugins out there, so probably ok.

Copy link
Member

@elextr elextr Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually so long as it is still initialized to say "Windows dialogs not used" then its definitely ok, since its not lying :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not initialized explicitly at all. But at least on my Debian Testing, the resulting value when Geany is running is FALSE.

Their implementation was buggy, use very old Windows APIs and require
double implementation and maintenance efforts for us
The GTK dialogs work well for all other users already, so they probably
will also for Windows users.

Closes geany#3209.
@eht16 eht16 force-pushed the remove_native_windows_dialogs branch from e0b8bff to f793f72 Compare June 12, 2022 12:46

if (app->project && !EMPTY(app->project->base_path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is adding this shortcut removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah!! ignore comment, githubs annoying handling of changed indent confused me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry. The diffs are bit hard to read because of indentation changes :(:

@elextr
Copy link
Member

elextr commented Jun 13, 2022

Just remembered that there was #2794 that was "fixed" by using native dialogs once after install?

@eht16
Copy link
Member Author

eht16 commented Jun 15, 2022

Just remembered that there was #2794 that was "fixed" by using native dialogs once after install?

Yes. This PR is the most easiest solution to solve the one problem in #3209.
Fixing the dialog crash or even better port the Windows native code to more modern APIs would be far better without a doubt. But I won't do this (not only because after I would have done there were probably more bugs in the code than before :D).
For me, it's a trade-off how to spend the available time (as usual and as you most probably agree on).

I'm happy to close this PR unmerged if we could find anyone else volunteering to do the good way.

@elextr
Copy link
Member

elextr commented Jun 15, 2022

I'm happy to close this PR unmerged if we could find anyone else volunteering to do the good way.

If it wasn't that #3209 is a crash I'd say leave the windows dialogs until GTK4 forces them out, after all they have been used for a long time without issues.

But since its a crash I'd say merge it, if any white knight rides in on their trusty steed I suspect the olde code would not be much help since the Windows dialog APIs have changed, but they can always choose to start with the commit before merge of after, their choice.

Or perhaps someone will try https://docs.gtk.org/gtk3/class.FileChooserNative.html and see if it works better on windows/macos and flatpack (see docs).

@kugel-
Copy link
Member

kugel- commented Jun 29, 2022

Interesting, didn't know about GtkFileChooserNative. This seems to be available in gtk4 as well.

@eht16
Copy link
Member Author

eht16 commented Sep 11, 2022

If nobody stops me, I would merge this in a week or so.
This should not hinder any experiements with GtkFileChooserNative at all.

@eht16 eht16 merged commit 465e60b into geany:master Sep 17, 2022
@b4n b4n added this to the 1.39/2.0 milestone Apr 28, 2023
@eht16 eht16 deleted the remove_native_windows_dialogs branch October 21, 2023 21:37
eht16 added a commit to eht16/geany that referenced this pull request Mar 17, 2024
This is a left-over from geany#3219 and should have been removed already.

Fixes geany#3627.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geany crashes when opening many files at once
4 participants