After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764879 - Extract GtkApplicationAccels private class from GtkApplication
Extract GtkApplicationAccels private class from GtkApplication
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkApplication
3.20.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-04-11 08:32 UTC by Sébastien Wilmet
Modified: 2016-04-22 11:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
app: write higher-level gtk_application_accels static functions (12.22 KB, patch)
2016-04-20 16:26 UTC, Sébastien Wilmet
accepted-commit_now Details | Review
app: extract GtkApplicationAccels private class (34.93 KB, patch)
2016-04-20 16:27 UTC, Sébastien Wilmet
needs-work Details | Review
app-accels: rename static functions (2.21 KB, patch)
2016-04-20 16:27 UTC, Sébastien Wilmet
accepted-commit_now Details | Review
app: share function to normalise detailed action name (5.29 KB, patch)
2016-04-20 16:27 UTC, Sébastien Wilmet
accepted-commit_now Details | Review
app: replace private accels functions by get_application_accels() (6.03 KB, patch)
2016-04-20 16:27 UTC, Sébastien Wilmet
accepted-commit_now Details | Review

Description Sébastien Wilmet 2016-04-11 08:32: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.
Comment 1 Sébastien Wilmet 2016-04-11 09:10:14 UTC
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.
Comment 2 Allison Karlitskaya (desrt) 2016-04-11 13:11:22 UTC
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.
Comment 3 Matthias Clasen 2016-04-11 13:14:25 UTC
ok, lets do it then
Comment 4 Sébastien Wilmet 2016-04-11 16:45:52 UTC
Ok, patches will arrive sometime this week normally.
Comment 5 Sébastien Wilmet 2016-04-20 16:26:53 UTC
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.
Comment 6 Sébastien Wilmet 2016-04-20 16:27:02 UTC
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.
Comment 7 Sébastien Wilmet 2016-04-20 16:27:08 UTC
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.
Comment 8 Sébastien Wilmet 2016-04-20 16:27:20 UTC
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.
Comment 9 Sébastien Wilmet 2016-04-20 16:27:28 UTC
Created attachment 326434 [details] [review]
app: replace private accels functions by get_application_accels()

It's like gtk_application_get_action_muxer().
Comment 10 Matthias Clasen 2016-04-22 03:22:00 UTC
Review of attachment 326431 [details] [review]:

By convention, we call noninstalled headers fooprivate.h.
Otherwise, this looks good.
Comment 11 Matthias Clasen 2016-04-22 03:23:17 UTC
Review of attachment 326430 [details] [review]:

ok
Comment 12 Matthias Clasen 2016-04-22 03:34:49 UTC
Review of attachment 326432 [details] [review]:

ok
Comment 13 Matthias Clasen 2016-04-22 03:37:01 UTC
Review of attachment 326433 [details] [review]:

Not sure this is much of an improvement, but ok
Comment 14 Matthias Clasen 2016-04-22 03:39:12 UTC
Review of attachment 326434 [details] [review]:

ok
Comment 15 Matthias Clasen 2016-04-22 03:39:14 UTC
Review of attachment 326434 [details] [review]:

ok
Comment 16 Sébastien Wilmet 2016-04-22 11:50:07 UTC
Thanks for the review. I've changed the header name and pushed the commits.

commit e0c34fd028e249e5a52d2c8d90590c737f2aac4d
commit bce4a0abdefa01a10c43da09c10a3526d2d03941
commit 3d182160bb366f441ef72b4d7365354d392661de
commit 3b988ce5239252b0a6c64e0a77eb8ed666f081b2
commit 554de0be2a9ef1e0b87c406de030c1d018184cf7