__Use case:__ I have a proxy plugin that wraps `GeanyProject` and re-emits signals on the project instance. In order for this to work, the proxy needs a signal before Geany destroys the project. [See this commit for details](https://github.com/codebrainz/geanyplusplus/commit/b251ab003ffdd3b9d5c9e8ac8...) You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1223
-- Commit Summary --
* Add "project-before-close" signal to API
-- File Changes --
M doc/pluginsignals.c (9) M src/geanyobject.c (6) M src/geanyobject.h (1) M src/plugindata.h (2) M src/project.c (2)
-- Patch Links --
https://github.com/geany/geany/pull/1223.patch https://github.com/geany/geany/pull/1223.diff
@@ -47,6 +47,7 @@ typedef enum GCB_PROJECT_OPEN, GCB_PROJECT_SAVE, GCB_PROJECT_CLOSE,
- GDB_PROJECT_BEFORE_CLOSE,
@b4n, @kugel- is it safe to change this enum in the middle, although its not officially in the API is it used by some object macro or something that makes it in the ABI?
Ammended commit to change API 229 to 230 since 094737533609dff5a2ff101f97e8c156bcca4979.
Do you have an actual plugin that needs this or is this just for geany++?
Both, Geany++ [is a plugin](https://github.com/codebrainz/geanyplusplus/blob/master/geany%2B%2B/proxy.cp...) :)
@@ -47,6 +47,7 @@ typedef enum GCB_PROJECT_OPEN, GCB_PROJECT_SAVE, GCB_PROJECT_CLOSE,
- GDB_PROJECT_BEFORE_CLOSE,
Yes, this OKAY. The enum values are only used if you use `g_signal_emit()` (instead of `g_signal_emit_by_name()`). Plugins shouldn't be emitting these at all.
If there are no objections, I will merge this "soon".
elextr approved this pull request.
LGTM
b4n commented on this pull request.
Mostly LGBI
@@ -47,6 +47,7 @@ typedef enum
GCB_PROJECT_OPEN, GCB_PROJECT_SAVE, GCB_PROJECT_CLOSE, + GDB_PROJECT_BEFORE_CLOSE,
Yeah, and this enum isn't in the API either.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
Wouldn't it be better if the signal received the `GeanyProject` as argument? OK, it'd be inconsistent with the other project signals, so maybe it's a bad idea. Or maybe it's a good idea to break one less thing in the future if we want to allow more than one project to be open? That might not make much sense because the other signals would have to be changed anyway, so meh.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
It would be better, but as you pointed out, I did it like this for consistency with the other signals. My actual use case is to be able to treat the project as a real instance of which there could (in theory) be many, which is why I need to know before it's destroyed. But since none of the other signals give the instance, it'd be weird to just have one that does.
kugel- commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
So is this the same problem you solved elsewhere with qdata? Why go a different path here?
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
No, it's not, I just access the global variable `geany_data->app->project`, but it's destroyed and set to `NULL` before the normal `project-close` signal is emitted, so I can't emit the `close` signal on the project instance.
kugel- commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
I wonder if we could just change project-close instead
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
That would be fine with me. I mostly just didn't want to mess with the existing signal in case some plugins somehow depended on the existing behaviour.
kugel- commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
Can't imagine someone truly depends on geany_data->app->project being already NULL.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
To do that would only require moving [the signal emission](https://github.com/geany/geany/blob/1.28.0/src/project.c#L467) to the top of the same function.
b4n commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
I wonder if we could just change project-close instead
Good point, I indeed can't see why someone would need the signal to be emitted *before* the data is freed.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
I do, it's the whole point of this PR :)
b4n commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
Oops, I meant *after*.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
I don't know why they would depend on that, but I can't be sure they don't, it would be pure speculation.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
Maybe a `@note` about the change in the signal documentation would suffice.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
See #1235
b4n commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
Hum, actually I just thought of a perfectly plausible use case: get notified the settings were changed back to the non-project one and read those. This requires the signal to be emitted after those were restored, and that currently mean when the project is actually closed.
So maybe we need that extra signal.
codebrainz commented on this pull request.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new ( + "project-before-close", + G_OBJECT_CLASS_TYPE (g_object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0);
@b4n yeah, though it would probably be nicer to actually add a new signal for that (ex. `settings-scope-changed`) than using the close signal by proxy.
Whichever you guys prefer, I'm rather indifferent. I poked around in Geany and Geany-Plugins and didn't see any problems with either approach.
b4n requested changes on this pull request.
LGTM apart the small typo and the merge conflict.
@@ -156,6 +156,12 @@ static void create_signals(GObjectClass *g_object_class)
G_SIGNAL_RUN_FIRST, 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + geany_object_signals[GDB_PROJECT_BEFORE_CLOSE] = g_signal_new (
typo: prefix should be `GCB_`, not `GDB_`
@@ -47,6 +47,7 @@ typedef enum
GCB_PROJECT_OPEN, GCB_PROJECT_SAVE, GCB_PROJECT_CLOSE, + GDB_PROJECT_BEFORE_CLOSE,
Ditto
@codebrainz pushed 2 commits.
f92f051 Merge branch 'master' into project-before-close 4d2f1d9 Fix typo (oops)
Fixed. Should probably be squashed into a single commit before merging.
b4n approved this pull request.
Closed #1223 via b7839a6e421431c8fa83204db351ccee3411cce1.
github-comments@lists.geany.org