search

MightyCreak / diffuse

Diffuse is a graphical tool for comparing and merging text files. It can retrieve files for comparison from Bazaar, CVS, Darcs, Git, Mercurial, Monotone, RCS, Subversion, and SVK repositories.
http://mightycreak.github.io/diffuse/
GNU General Public License v2.0
260 stars 46 forks source link

Use GTK3's GApplication/GtkApplication #178 #181

Closed yuriiz closed 1 year ago

yuriiz commented 1 year ago

This PR implements https://github.com/MightyCreak/diffuse/issues/178

Most of main() is moved to Gtk.Application.do_command_line as it is standard to handle command line options in Gtk Apps. Gtk.MenuBar is removed and replaced with Gio.Menu to provide native MacOS menu.

Closes #178

MightyCreak commented 1 year ago

Thank you so much @yuriiz for your contribution!

Could you edit the description so that we could figure out what led you to do these changes? Be as explicit as possible please ;) I know this part is tedious, but it will save a lot of time for the review!

yuriiz commented 1 year ago

Thanks. This looks good on my machine, except for the application menu that says "Python" instead of "Diffuse". Also, the "Preferences" menu could be part of the application and not the "Edit" menu on macOS.

Moving "Preferences" to app menu will make it inaccessible on platforms without app menu. I've checked apps from GNOME suite and even it has app menu support they still keep "Preferences" in window menu for compatibility with other DEs:

Screenshot from 2022-11-13 08-06-45

Screenshot from 2022-11-13 08-08-35

krlmlr commented 1 year ago

Thanks, agreed. We could remove the "Edit/Preferences..." menu only on MacOS.

MightyCreak commented 1 year ago

There is an issue with the menu icons. On master, there are no icons in the menu, but here it looks like this:

image

MightyCreak commented 1 year ago

There is an issue with the menu icons. On master, there are no icons in the menu, but here it looks like this:

Ok, I managed to add the icon (the Gtk.STOCK_* constants are deprecated), but the result is.. bad:

image

So I guess the best move is simply to remove the icons (which would end up the same as what we have on master right now)

MightyCreak commented 1 year ago

Give me a few minutes, I'll push some commits to remove icons (and fix a few flake8 warnings)

MightyCreak commented 1 year ago

Ok, I pushed some fixes

MightyCreak commented 1 year ago

Ok, I fixed a bunch of stuff (I've split the changes in several, easy to understand commits). I've noticed that Ctrl+C/Ctrl+V doesn't work when editing a line and selecting some characters.

I get this warning in the console:

(diffuse:2): Gtk-WARNING **: 16:27:25.313: Accelerator 'c' tries to invoke action 'win.copy' without target, but action expects parameter with type 's'

yuriiz commented 1 year ago

@MightyCreak I've fixed Ctrl+C/Ctrl+V. See recent commit. Please suggest how to handle App Menu/Preferences.

MightyCreak commented 1 year ago

Please suggest how to handle App Menu/Preferences.

@yuriiz: are you referring to @krlmlr's comment?

We could remove the "Edit/Preferences..." menu only on MacOS.

If you do, then I would keep it as it is for now.

@krlmlr: would you mind sharing some screenshots of Diffuse on your Mac explaining why having the preferences at two places is an issue? Thx :wink:

MightyCreak commented 1 year ago

I think this PR is big enough as it is :sweat_smile:

@yuriiz, @krlmlr: are you ok with the changes? more PRs will come after this one, but this one will close #178 and that is already a huge step forward!

krlmlr commented 1 year ago

It's not an issue to have "Preferences" in two places. I'm only suggesting that the application menu is the preferred "native" location.

krlmlr commented 1 year ago

It's still greyed out in the application menu, though, and the application name is still "Python":

Screen Shot 2022-11-21 at 05 06 12

Agree to tackle this in a separate PR, perhaps @hugoholgersson has an idea?

yuriiz commented 1 year ago

application name is still "Python"

I've added set_application_name. Please let me know if it helps.

krlmlr commented 1 year ago

I just double-checked, no dice.

Will this work better with GTK 4 (#143)? At least this is what https://gitlab.gnome.org/GNOME/gtk-mac-integration/ seems to suggest.

Provides integration for Gtk+ applications into the Mac desktop, with support for the Mac menubar, the Dock, and for finding resources in bundles. GtkosxApplication is gobject-introspectable.

GtkosxApplication compiles and works with i386, x86_64 and arm64. New Gtk3 applications should prefer the GApplication/GtkApplication and GMenuModel/GMenu APIs which make this library unnecessary. Gtk4 applications must use those facilities as the older API on which this libary depends have been remobed.

If so, we should merge and move on?

MightyCreak commented 1 year ago

I've asked a question in the Python GTK Matrix room. I'll wait a bit before merging, to see if we have a clear answer on that. Otherwise, I agree with you @krlmlr, I'm keen to merge and move on. GTK4 will probably provide a better compatibility with macOS.

What I'd like to do is change the current source code so that it respects better the Python GTK guidelines/conventions, and then try and tackle the GTK 4 migration.

MightyCreak commented 1 year ago

@krlmlr, @hugoholgersson, @yuriiz : I've had an answer:

According to some of my searches it seems that you need to set CFBundleName in a Info.plist file (part of a Mac App Bundle). Apparently, there is an undocumented function to change it after program start, but it is undocumented stuff. (Note that I am not a Mac developer or user)

So I'll revert the GLib.set_application_name (as it doesn't work), and I'll merge this PR! :rocket:

MightyCreak commented 1 year ago

Thank you so much everyone for your contributions! Thanks to you I've learned a lot more about GTK and Diffuse (as I'm not the original author, I still have some blind spots here and there :sweat_smile: )