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 670149 - Automatically add a callback when user double-clicks a signal in the glade signal editor.
Automatically add a callback when user double-clicks a signal in the glade si...
Status: RESOLVED FIXED
Product: anjuta
Classification: Applications
Component: plugins: glade
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Anjuta maintainers
Anjuta maintainers
Depends on:
Blocks:
 
 
Reported: 2012-02-15 16:54 UTC by Marco Diego Aurélio Mesquita
Modified: 2012-02-24 13:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Automatically add callbacks on signal activation. (7.38 KB, patch)
2012-02-15 16:54 UTC, Marco Diego Aurélio Mesquita
none Details | Review
Automatically add callbacks on signal activation. (8.08 KB, patch)
2012-02-16 20:38 UTC, Marco Diego Aurélio Mesquita
none Details | Review
Send signal activated signal. (3.52 KB, patch)
2012-02-16 20:41 UTC, Marco Diego Aurélio Mesquita
none Details | Review
Automatically add callbacks on signal activation. (8.08 KB, patch)
2012-02-16 21:17 UTC, Marco Diego Aurélio Mesquita
needs-work Details | Review
Automatically add callbacks on signal activation. (10.32 KB, patch)
2012-02-17 19:25 UTC, Marco Diego Aurélio Mesquita
none Details | Review
Automatically add callbacks on signal activation. (9.77 KB, patch)
2012-02-22 18:12 UTC, Marco Diego Aurélio Mesquita
needs-work Details | Review
Fix signal disconnection for member widgets feature. (5.43 KB, patch)
2012-02-24 11:53 UTC, Marco Diego Aurélio Mesquita
none Details | Review

Description Marco Diego Aurélio Mesquita 2012-02-15 16:54:25 UTC
Created attachment 207681 [details] [review]
Automatically add callbacks on signal activation.

The attached patch does (basically) the same as https://bugzilla.gnome.org/show_bug.cgi?id=667861 but for callbacks instead of widgets. Now, when the user double-clicks a (non-dummy) signal in the glade-signal editor, anjuta will automatically find a (currently opened and compatible) source file and will add the callback to its end.

The attached patch depends on https://bugzilla.gnome.org/show_bug.cgi?id=669272 .

Please review it.
Comment 1 Marco Diego Aurélio Mesquita 2012-02-16 20:38:03 UTC
Created attachment 207806 [details] [review]
Automatically add callbacks on signal activation.

The attached updates the previous to reflect latest changes in https://bugzilla.gnome.org/show_bug.cgi?id=669272 .

Please review it.
Comment 2 Marco Diego Aurélio Mesquita 2012-02-16 20:41:03 UTC
Created attachment 207808 [details] [review]
Send signal activated signal.

The attached patch fixes a check in the previous one. Please commit it.
Comment 3 Marco Diego Aurélio Mesquita 2012-02-16 21:17:31 UTC
Created attachment 207810 [details] [review]
Automatically add callbacks on signal activation.

Fixes last patch. Please review it.
Comment 4 Johannes Schmid 2012-02-16 21:22:14 UTC
Review of attachment 207810 [details] [review]:

See below, has to wait for the glade patch to hit master, anyway.

::: plugins/language-support-cpp-java/plugin.c
@@ +897,3 @@
+	gchar *mark = generate_widget_member_init_marker (ui_filename);
+	IAnjutaIterable* mark_position;
+    mark_position = language_support_get_mark_position (editor, mark);

wrong indentation

@@ +991,3 @@
 		}
 
+		// Since these signals are not disconnected on plugin uninstall, we have to prevent multiple connection.

I think we discussed before that they *should* be disconnected on uninstall. No need for this work-around code.
Comment 5 Marco Diego Aurélio Mesquita 2012-02-16 21:46:00 UTC
Without this check, the signal is connected multiple times.
Comment 6 Johannes Schmid 2012-02-16 21:59:23 UTC
The signal handler should be correctly disconnected in the uninstall() handler and as such there shouldn't be any need to check if it is connected.
Comment 7 Marco Diego Aurélio Mesquita 2012-02-16 23:42:15 UTC
Actually, there is a good reason for not to disconnect the signal on uninstall() : The uninstall is called whenever a tab is switched. If we disconnect it on uninstall, we'll only be able to add the callback if the source file is currently being viewed (as was in the first version of the member widgets feature).

If we really have to make sure the signal is disconnected, maybe it should be handled (disconnected) when the tab is closed, but the check (to prevent multiple connections) would have to remain there. Do you still oppose this approach?
Comment 8 Marco Diego Aurélio Mesquita 2012-02-17 19:25:14 UTC
Created attachment 207898 [details] [review]
Automatically add callbacks on signal activation.

The attached solves the problem of leaving a signal connected. Now, we get notified when the editor is destroyed, then the signals for the member widgets feature and automatic callback addition are disconnected.

Please review it.
Comment 9 Abderrahim Kitouni 2012-02-17 20:37:50 UTC
(In reply to comment #7)
> Actually, there is a good reason for not to disconnect the signal on
> uninstall() : The uninstall is called whenever a tab is switched. If we
> disconnect it on uninstall, we'll only be able to add the callback if the
> source file is currently being viewed (as was in the first version of the
> member widgets feature).

This is something I wanted to bring up on the mailing list but...

Anyway, in the vala plugin I'm disconnecting the signal when the editor is changed, and as such one needs to have the editor open when inserting member widgets.

I don't know what's the best way to deal with this, as the plugin shouldn't (and cannot, in the case of vala) keep doing things after it has been deactivated, so we probably need to do something else.
Comment 10 Johannes Schmid 2012-02-18 08:50:08 UTC
Right, the language-support-plugin is unloaded when you change the editor as it always operates on the current editor, so any cleanup should be done in uninstall().

I think it is totally intransparent to the user to add code to hidden editor windows. In the case you have to add something you have to switch to the correct editor first (probably by goto_file_line_mark()) and listen to the "opened" signal of the editor.

Further for the patch, "glade-callback-add" shouldn't have this ugly string signature, it should pass the information necessary in different arguments.
Comment 11 Marco Diego Aurélio Mesquita 2012-02-22 18:12:19 UTC
Created attachment 208208 [details] [review]
Automatically add callbacks on signal activation.

The attached patch addresses concerns from previous comment. Please review it.
Comment 12 Johannes Schmid 2012-02-24 10:28:20 UTC
Review of attachment 208208 [details] [review]:

::: plugins/language-support-cpp-java/plugin.c
@@ +1014,3 @@
+						  lang_plugin);
+
+		// Since this signal is not disconnected on plugin uninstall, we have to prevent multiple connection.

It should(!) be disconnected on plugin uninstall - as I said before.
Comment 13 Marco Diego Aurélio Mesquita 2012-02-24 11:06:37 UTC
It is disconnected on plugin uninstall:

+	g_signal_handlers_disconnect_by_func (lang_plugin->current_editor,
+					                	  G_CALLBACK (on_glade_callback_add),
+	                                      lang_plugin);
+

The comment is still there because of the member widgets feature. If you don't mind, I'd like to fix that one with a proper future patch instead of pushing the fix together with this feature. Is that acceptable?
Comment 14 Johannes Schmid 2012-02-24 11:13:57 UTC
If you attach the future patch here, than yes.
Comment 15 Marco Diego Aurélio Mesquita 2012-02-24 11:53:46 UTC
Created attachment 208339 [details] [review]
Fix signal disconnection for member widgets feature.

The attached patch fixes signal disconnection for the member widgets feature and fixes some whitespaces problems with the previous patch. Please review it.
Comment 16 Johannes Schmid 2012-02-24 12:28:36 UTC
Review of attachment 208208 [details] [review]:

Sorry, I missed some pieces in my last review.

::: plugins/glade/plugin.c
@@ +116,3 @@
+    gchar *path = glade_project_get_path (project);
+
+    /* String format: widgettypename:signalname:handler_name:object:swap:after

Is this need anymore here?

@@ +150,3 @@
+													  glade_signal_get_handler (signal),
+													  glade_signal_get_userdata (signal),
+													  glade_signal_get_swapped (signal)?"1":"0",

glade_signal_get_swapped() and glade_signal_get_after() return a gboolean and you should definitly not pass a "gchar*" for a boolean.
Comment 17 Marco Diego Aurélio Mesquita 2012-02-24 12:51:57 UTC
This part of the code is commented. It is removed by the other patch (208339). AFAIK there's no need to worry about this.