GNOME Bugzilla – Bug 764879
Extract GtkApplicationAccels private class from GtkApplication
Last modified: 2016-04-22 11:50:07 UTC
In gtkapplication.c, there is the Accels semi-class. It was implemented like a class, with all its functions prefixed by "accels_", with the init() and finalize() functions, etc. So why not moving it to its own file, gtkapplicationaccels.c? This would have several benefits: - Less code in GtkApplication. The accels handling is something self-contained, and GtkApplication would delegate the work. - For the accels functions, there would be a distinction between static functions and functions in the gtkapplicationaccels.h header, which would make the code easier to understand, because we would have a good overview just by reading the header. - The struct _GtkApplicationPrivate would be easier to find instead of being in the middle of the file, although this is of course fixable independently. Doing this refactoring is *currently* easily possible. But might be less easily feasible in the future if the accels code becomes entangled with other features. Before proposing patches for this, I prefer to know if you agree with the idea.
And the "public" functions of GtkApplicationAccels would deal with detailed_action_names, like the public functions of GtkApplication. The normalization to action_and_target would be an implementation detail of GtkApplicationAccels. So there would be more code removed from GtkApplication than just the accels_ functions.
I'm no Gtk maintainer (which may be why I did it this way in the first place) but if you wanted to propose a patch doing what you described, I'd be happy to review and ACK it.
ok, lets do it then
Ok, patches will arrive sometime this week normally.
Created attachment 326430 [details] [review] app: write higher-level gtk_application_accels static functions These will become the functions present in the gtkapplicationaccels.h header. The gtk_application_accels functions deal with detailed_action_name's instead of action_and_target's. action_and_target is an implementation detail of Accels. The added function prototype is temporary, it'll be removed in a later commit.
Created attachment 326431 [details] [review] app: extract GtkApplicationAccels private class This has several benefits: - Less code in GtkApplication. The accels handling is something self-contained, and GtkApplication now delegates the work. - For the accels functions, there is now a distinction between static functions and functions in the gtkapplicationaccels.h header, which makes the code easier to understand, because we have a good overview just by reading the header. - The struct _GtkApplicationPrivate is now easier to find instead of being in the middle of the file.
Created attachment 326432 [details] [review] app-accels: rename static functions Remove the "accels_" prefix from the remaining static functions. The prefix no longer makes sense since the whole file is devoted to accels.
Created attachment 326433 [details] [review] app: share function to normalise detailed action name Put the function in gtkactionmuxer.c, where gtk_print_action_and_target() is implemented.
Created attachment 326434 [details] [review] app: replace private accels functions by get_application_accels() It's like gtk_application_get_action_muxer().
Review of attachment 326431 [details] [review]: By convention, we call noninstalled headers fooprivate.h. Otherwise, this looks good.
Review of attachment 326430 [details] [review]: ok
Review of attachment 326432 [details] [review]: ok
Review of attachment 326433 [details] [review]: Not sure this is much of an improvement, but ok
Review of attachment 326434 [details] [review]: ok
Thanks for the review. I've changed the header name and pushed the commits. commit e0c34fd028e249e5a52d2c8d90590c737f2aac4d commit bce4a0abdefa01a10c43da09c10a3526d2d03941 commit 3d182160bb366f441ef72b4d7365354d392661de commit 3b988ce5239252b0a6c64e0a77eb8ed666f081b2 commit 554de0be2a9ef1e0b87c406de030c1d018184cf7