GNOME Bugzilla – Bug 700383
Anjuta should automatically associate .ui and source files
Last modified: 2013-06-05 19:15:54 UTC
Created attachment 244312 [details] [review] Automatic association of .ui with source files patch Attached patch adds infra-structure and enables automatic association between .ui (glade) files and source code files. Current implementation is preliminary, bu I'd like to get some comments on the approach used. The attached patch adds a hashtable in document manager with companion accessory methods. Though it is language independent, the current patch only enables the feature for the C language. Adding support for other languages is not complicated though. Association is derived using mark comments. In the case of a C-source file, the mark /* ANJUTA: Widgets declaration for gtk_foobar.ui - DO NOT REMOVE */ associates it with the gtk_foobar.ui ui file. The feature currently making use of the added infra-structure is automatic glade member and callback addition. Now, when editing an interface in glade inside anjuta, just double-click a widget in the inspector and a private member will be created in the associated source file it. Also, double-clicking a recently added signal handler, will automatically create the callback function in the associated source file. Comments please!
Created attachment 244439 [details] [review] Automatic association of .ui with source files patch Attached patch applies cleanly to anjuta master. It is still far from clean but already allows us to test it. Please test it.
Created attachment 244460 [details] [review] Automatic association of .ui with source files patch There was a little problem on the way glade plugin was connecting the "signal-editor-created" signal. The ordering has been fixed. Attached patch should apply to anjuta master. It is still missing some cleanups, but otherwise is working beautifully. Please test it or comment on the approach used.
Created attachment 244561 [details] [review] Automatic association of .ui with source files patch First round of cleanups: debug printfs and commented code removed. Please comment it.
Created attachment 244562 [details] [review] Automatic association of .ui with source files patch Second round of cleanups: removed duplicated code.
Created attachment 244564 [details] [review] Automatic association of .ui with source files patch Third round of cleanups: constantified widgets declaration marker prefix and suffix.
Created attachment 244565 [details] [review] Automatic association of .ui with source files patch Fourth round of cleanups. Improved checking when adding members and callbacks. I think it is good to go now. Please review it.
(In reply to comment #6) > Please review it. Thank for your work, I have seen your patches and I will review them. But it isn't a simple fix, so I need some time to understand the issue and your solution. I will try to do it this week end.
Review of attachment 244565 [details] [review]: ::: libanjuta/interfaces/libanjuta.idl @@ +2920,3 @@ + GList* get_associated_with (gchar *slave); + + /** I don't really like these functions. I think a new simple IAnjutaLanguageSupport interface would be better. It would only have one function GFile * ianjuta_language_support_get_source_for_ui_file (GFile *ui_file); language-support-cpp-java would then implement this interface and store the ui <--> source mappings itself.
Review of attachment 244565 [details] [review]: First a few questions to understand the scope of this: Currently, you can already add a callback by dragging the callback function name from the glade property pane to the editor pane, right? So, this patch allows to do this by just double clicking on the callback function name, right? Currently, it is working only if the C source code containing the callback is already open. Wouldn't it better if it works even if the document is not open? First I think you could keep the association even if the document is closed. But making it works even if the file hasn't been open is a quite different approach. Is it possible to keep the source name inside the glade file? or read all the source files and keep the association inside a cache like for the symbol database? I think the change in plugins/file-loader/plugin.c is only for debugging and can be removed. It's not working with Scintilla but I can take care of this. You have added an update function in the symbol-db plugin but I think the symbol database is automatically updated when the file is modified, no? Why do you need this change? It's probably better to move it in a different patch.
(In reply to comment #8) > I think a new simple IAnjutaLanguageSupport interface would be better. It > would only have one function > GFile * > ianjuta_language_support_get_source_for_ui_file (GFile *ui_file); I think the issue is that you can have several language support plugins loaded at the same time while you have only one document manager. You don't know which language support plugin is used for the slave file needed by the master ui file. You will have to check each language support plugin one by one, which is a bit tedious. But I agree with you, it is perhaps not the best place to have this in the document manager. Isn't it possible to add this in the glade plugin? I think it can take care of the association like the document manager. Or do you plan to use these associations outside the glade plugin? Anyway, I think the main point is to define if we are limited to the open documents or if it could work for any project files as I have asked above.
Created attachment 244772 [details] [review] Automatic association of .ui with source files patch Fifth round of cleanups: removed remaining debug printf and unused interface functions.
Created attachment 244907 [details] [review] Automatic association of .ui with source files patch Sixth round of cleanups: association infra-structure has been moved to the glade plugin. I think it is good to go now. Please review it.
(In reply to comment #9) > Review of attachment 244565 [details] [review]: > > First a few questions to understand the scope of this: > Currently, you can already add a callback by dragging the callback function > name from the glade property pane to the editor pane, right? Right. > So, this patch allows to do this by just double clicking on the callback > function name, right? Right, but not only that. It also switches to the tab with associated document. > Currently, it is working only if the C source code containing the callback is > already open. Wouldn't it better if it works even if the document is not open? Sure, but it would involve a very different patch than it is now. I think the current approach is a good compromise. > First I think you could keep the association even if the document is closed. > But making it works even if the file hasn't been open is a quite different > approach. Is it possible to keep the source name inside the glade file? or read > all the source files and keep the association inside a cache like for the > symbol database? According to Tristan, "[...] in the bright future of composite widgets defined with <template> xml, there is already an implied relationship between the class name and the xml data"[1]. While this "bright future" has not come yet, I think we should stay with my approach. I'm willing to improve the glade anjuta integration to take composite widget into accounts and make the association between .ui and source files without needing marker comments, but we'll have to wait a bit. I think the current suggestion is am idea worth of been accepted. > I think the change in plugins/file-loader/plugin.c is only for debugging and > can be removed. Removed in the latest iteration. > It's not working with Scintilla but I can take care of this. Really don't know why. I almost never use the Scintilla plugin. Thanks for taking care of this. > You have added an update function in the symbol-db plugin but I think the > symbol database is automatically updated when the file is modified, no? Why do > you need this change? It's probably better to move it in a different patch. Without the change, symbols are not updated when glade members or callbacks are added. This saves the user from accidentally adding the same callback or member multiple times. It is a simple and small change. I think there is no problem in committing it with the current patch. Thanks! [1] https://mail.gnome.org/archives/anjuta-devel-list/2013-May/msg00012.html
Created attachment 244912 [details] [review] Automatic association of .ui with source files patch Small code improvement in association detection.
Created attachment 244940 [details] [review] Automatic association of .ui with source files patch The current patch improves symbol db behavior. I think it is good enough now. Please review it.
Created attachment 244983 [details] [review] Automatic association of .ui with source files patch Latest patch has been broken up in two. This one holds the automatic association part.
Created attachment 244984 [details] [review] Improve symbol-de behavior Latest patch has been broken up in two. This one holds the symbol-db part.
Review of attachment 244984 [details] [review]: Thanks for splitting your patch. I have looked at the code and I have seen that there is a signal "code-added" which is doing something similar to the new update_symbol function. Do you know this signal? Isn't it possible to use it instead of adding a new function? Or do you think it's better to add this function and in this case, isn't it possible to remove this signal?
(In reply to comment #18) > Review of attachment 244984 [details] [review]: > > Thanks for splitting your patch. I have looked at the code and I have seen that > there is a signal "code-added" which is doing something similar to the new > update_symbol function. Do you know this signal? Isn't it possible to use it > instead of adding a new function? Or do you think it's better to add this > function and in this case, isn't it possible to remove this signal? I created such signal. To respect it semantic, I think it is better to create this new function. [1] https://git.gnome.org/browse/anjuta/commit/?id=96303702a574dfd7ee72d05d16785f29388731c7
(In reply to comment #19) > I created such signal. To respect it semantic, I think it is better to create > this new function. I'm not surprise, it is looking really similar. What do you mean to respect it semantic?
(In reply to comment #20) > (In reply to comment #19) > > I created such signal. To respect it semantic, I think it is better to create > > this new function. > > I'm not surprise, it is looking really similar. What do you mean to respect it > semantic? I mean it is called "code-added" so, it doesn't makes sense to emit it when code is removed as is the case when cutting and, usually, undoing. But I do agree that the "code-added" signal and the "update_symbols" function are looking redundant. If we decide to use only the signal, it should be renamed to "update-needed"; if we decide to stay with the "update_symbols" function, the signal should be retired. I'll see what path looks better and come up with an updated patch.
Created attachment 245040 [details] [review] Improve symbol-de behavior This patch changes the "code-added" signal to "code-changed". It improves the behavior of the symbol-db plugin when cutting, pasting, unduing, redoing and when code is added through the glade plugin. Because of some strange bugs in the symbol-db plugin, it doesn't works perfectly. Considering that the old behavior wasn't ideal either, I argue that it should be committed as is and these bugs be fixed at a later moment (probably with Massimo help). Please commit it.
(In reply to comment #22) > Created an attachment (id=245040) [details] [review] > Improve symbol-de behavior > > This patch changes the "code-added" signal to "code-changed". It improves the > behavior of the symbol-db plugin when cutting, pasting, unduing, redoing and > when code is added through the glade plugin. > > Because of some strange bugs in the symbol-db plugin, it doesn't works which bugs are you referring to? > perfectly. Considering that the old behavior wasn't ideal either, I argue that > it should be committed as is and these bugs be fixed at a later moment > (probably with Massimo help). > There is a mechanism which is using TIMEOUT_INTERVAL_SYMBOLS_UPDATE to check for updates on the editor on a scheduled interval. Do you need to see changes of symbols in realtime when glade adds code to the editor?
(In reply to comment #23) > (In reply to comment #22) > > Created an attachment (id=245040) [details] [review] [details] [review] > > Improve symbol-de behavior > > > > This patch changes the "code-added" signal to "code-changed". It improves the > > behavior of the symbol-db plugin when cutting, pasting, unduing, redoing and > > when code is added through the glade plugin. > > > > Because of some strange bugs in the symbol-db plugin, it doesn't works > > which bugs are you referring to? Add a glade C callback to your code while having the .h header file opened (by dragging and dropping the signal name to the source file). Switch to the .h header file tab to see how symbol list has been updated. Now switch back to the .c source file and press ctrl-z: symbol list gets confused. Retry it with many combinations of adding the signal callback, pressing ctrl-z and switching tabs. > > > perfectly. Considering that the old behavior wasn't ideal either, I argue that > > it should be committed as is and these bugs be fixed at a later moment > > (probably with Massimo help). > > > > There is a mechanism which is using TIMEOUT_INTERVAL_SYMBOLS_UPDATE to check > for updates on the editor on a scheduled interval. > Do you need to see changes of symbols in realtime when glade adds code to the > editor? What I'd like is a mechanism to say the symbol plugin "hey, there's a change in this editor. Please update its symbol list (preferably now!)." that works reliably.
Created attachment 245117 [details] [review] Solution to symbol-db inconssitencies problem The attached patch (if applied on top of my patches) drastically improves the symbol-db behavior. The idea is to smartly modify sdb_plugin->current_editor to the editor which had changes (and correctly update its value when update is finished) and keep an association hash table between proc_id and editor. That way, when an scan ends, the correct editor will be updated. If there's no disagreement on the solution found, I'd can clean it up to request its inclusion with the other patches. Please comment it.
Review of attachment 245117 [details] [review]: + if(sdb_plugin->editors){ + printf("Will now add association between proc_id and editor.\n"); + g_hash_table_insert(sdb_plugin->editors, GINT_TO_POINTER (proc_id), editor); + printf("Added association between %d and %p.\n", proc_id, editor); + } I think it's better to use a GPtrArray: buffer_update_editor because it's already done by buffer_update_files and buffer_update_ids or replace everything with a hash table but I think it could be done later. + IAnjutaEditor *old_editor = sdb_plugin->current_editor; + sdb_plugin->current_editor = editor; sdb_plugin->need_symbols_update = TRUE; editor_buffer_symbols_update (editor, sdb_plugin); + sdb_plugin->current_editor = old_editor; I can understand why you have written this but this code doesn't look good. Why is it necessary to the current_editor variable? As the editor_buffer_symbols_update already get the editor, it shouldn't rely on the current_editor variable. + /* Remove editor from list of editors */ + /*gpointer key = g_hash_table_find (sdb_plugin->editors, NULL, sdb_plugin->editors); + if(key) + g_hash_table_remove(sdb_plugin->editors, key);*/ + /*if(sdb_plugin->editors) + { + GList *keys = g_hash_table_get_keys (sdb_plugin->editors); + GList *iterator = keys; + for(; iterator->data; iterator = iterator->next) + { + gpointer editor = g_hash_table_lookup (sdb_plugin->editors, iterator->data); + if(sdb_plugin->current_editor == editor) + g_hash_table_remove(sdb_plugin->editors, iterator->data); + } + g_list_free(keys); + }*/ You can do this by using the function g_hash_table_foreach_remove, you need to pass a function with one parameter checking your condition. In your case, the function should be value == user_data and the parameter should be editor. Anyway, I think it's probably better to use a GPtrArray instead of a hash table. A hash table is not efficient for such operation.
(In reply to comment #26) > Review of attachment 245117 [details] [review]: > > + if(sdb_plugin->editors){ > + printf("Will now add association between proc_id and editor.\n"); > + g_hash_table_insert(sdb_plugin->editors, GINT_TO_POINTER > (proc_id), editor); > + printf("Added association between %d and %p.\n", proc_id, editor); > + } > > I think it's better to use a GPtrArray: buffer_update_editor because it's > already done by buffer_update_files and buffer_update_ids or replace everything > with a hash table but I think it could be done later. I vote for doing it at a later moment. > > > + IAnjutaEditor *old_editor = sdb_plugin->current_editor; > + sdb_plugin->current_editor = editor; > sdb_plugin->need_symbols_update = TRUE; > editor_buffer_symbols_update (editor, sdb_plugin); > + sdb_plugin->current_editor = old_editor; > > I can understand why you have written this but this code doesn't look good. Why > is it necessary to the current_editor variable? As the > editor_buffer_symbols_update already get the editor, it shouldn't rely on the > current_editor variable. I'm afraid on_editor_buffer_symbols_update_timeout may be called in the meantime, that could confuse the symbol-db plugin. I think that current_editor member should be removed (in the future), but we have to take extra care with it for now. > > > + /* Remove editor from list of editors */ > + /*gpointer key = g_hash_table_find (sdb_plugin->editors, NULL, > sdb_plugin->editors); > + if(key) > + g_hash_table_remove(sdb_plugin->editors, key);*/ > + /*if(sdb_plugin->editors) > + { > + GList *keys = g_hash_table_get_keys (sdb_plugin->editors); > + GList *iterator = keys; > + for(; iterator->data; iterator = iterator->next) > + { > + gpointer editor = g_hash_table_lookup (sdb_plugin->editors, > iterator->data); > + if(sdb_plugin->current_editor == editor) > + g_hash_table_remove(sdb_plugin->editors, iterator->data); > + } > + g_list_free(keys); > + }*/ > > You can do this by using the function g_hash_table_foreach_remove, you need to > pass a function with one parameter checking your condition. In your case, the > function should be value == user_data and the parameter should be editor. > Anyway, I think it's probably better to use a GPtrArray instead of a hash > table. A hash table is not efficient for such operation. I'll try to use g_hash_table_foreach_remove and come up with a cleaner patch.
Created attachment 245226 [details] [review] Solution to symbol-db inconsistencies problem The attached is a cleaned up version of my previous patch to improve fix inconsistencies in the symbol-db plugin. Please commit it.
The following code can be used to get the current editor: How to get current editor: " IAnjutaDocumentManager *docman; IAnjutaDocument *doc; docman = anjuta_shell_get_interface (ANJUTA_PLUGIN (plugin)->shell, IAnjutaDocumentManager, NULL); if (!docman) return; doc = ianjuta_document_manager_get_current_document (docman, NULL); if(!doc) return; current_editor = IANJUTA_IS_EDITOR (doc) ? IANJUTA_EDITOR (doc) : NULL; " Now we can get rid of the ->current_editor in symbol-db.
(In reply to comment #27) > I vote for doing it at a later moment. I think it could have been done now, but that's ok. > I'm afraid on_editor_buffer_symbols_update_timeout may be called in the > meantime, that could confuse the symbol-db plugin. I think we have to check if it is a problem or not. I have looked at the code and I don't think this will confuse symbol-db plugin. In the editor_buffer_symbols_update function, there is a check to avoid launching another scan if one is already running. Have you see that it's working better with this code? > I think that current_editor > member should be removed (in the future), but we have to take extra care with > it for now. Ok.
(In reply to comment #29) > The following code can be used to get the current editor: > doc = ianjuta_document_manager_get_current_document (docman, NULL); > Now we can get rid of the ->current_editor in symbol-db. Yes, I know this function. As I have said above I'm not sure this current_editor member is an issue. I would prefer to keep it if you can removed your temporary change of this variable.
(In reply to comment #24) > > which bugs are you referring to? > > Add a glade C callback to your code while having the .h header file opened (by > dragging and dropping the signal name to the source file). Switch to the .h > header file tab to see how symbol list has been updated. Now switch back to the > .c source file and press ctrl-z: symbol list gets confused. Retry it with many > combinations of adding the signal callback, pressing ctrl-z and switching tabs. > ctrl-z to undo the adding of signal? Is the file saved when you drag&drop the function? > > > > > perfectly. Considering that the old behavior wasn't ideal either, I argue that > > > it should be committed as is and these bugs be fixed at a later moment > > > (probably with Massimo help). > > > > > > > There is a mechanism which is using TIMEOUT_INTERVAL_SYMBOLS_UPDATE to check > > for updates on the editor on a scheduled interval. > > Do you need to see changes of symbols in realtime when glade adds code to the > > editor? > > What I'd like is a mechanism to say the symbol plugin "hey, there's a change in > this editor. Please update its symbol list (preferably now!)." that works > reliably. well the symbols updating process is not in real time, due to the use of ctags. What's wrong with waiting N seconds before update the list? We can lower the update-time if you'd like. Or, better, you can force a save of the file when you've dragged your signal: that would force symbol-db to automatically update the symbols of the file and the view list. What do you think?
(In reply to comment #32) > Or, better, you can force a save of the file when you've dragged your signal: > that would force symbol-db to automatically update the symbols of the file and > the view list. I think it's better avoid saving the file if it's not necessary because it's easier to cancel just close the editor without saving. Else, I think it is can be better to update the symbol on request instead of waiting a time out. I plan to commit these patches so tell me if you think something is not working. By the way, I think one improvement would be to keep the association between the .ui file and the source file in the symbol database so it will work even if the source file is not loaded in the editor.
(In reply to comment #33) > (In reply to comment #32) > > Or, better, you can force a save of the file when you've dragged your signal: > > that would force symbol-db to automatically update the symbols of the file and > > the view list. > > I think it's better avoid saving the file if it's not necessary because it's > easier to cancel just close the editor without saving. > > Else, I think it is can be better to update the symbol on request instead of > waiting a time out. I plan to commit these patches so tell me if you think > something is not working. ok go ahead with patches. I'm just worrying a little bit about the high coupling which is happening between symbol-db and the editor(s). > > > By the way, I think one improvement would be to keep the association between > the .ui file and the source file in the symbol database so it will work even if > the source file is not loaded in the editor. uhm this doesn't seem a good approach to me: you're trying to mix pure symbols and source files info with some more logic info: the .ui file, which has anything to do. IMHO though such a change for a side plugin as the glade one wouldn't be worth.
(In reply to comment #34) > ok go ahead with patches. I'm just worrying a little bit about the high > coupling which is happening between symbol-db and the editor(s). I think the role of Anjuta is to allow some coupling between plugins else why not use any text editor and glade alone. Then I agree that we should keep simple interfaces to allow changing the plugins independently. So we can discuss each case. > this doesn't seem a good approach to me: you're trying to mix pure symbols > and source files info with some more logic info: the .ui file, which has > anything to do. IMHO though such a change for a side plugin as the glade one > wouldn't be worth. I think the glade plugin is quite important, most programs need an user interface and glade is the way to edit them. Associating a source file and a user interface description is useful and not new. I have used it in Borland IDE perhaps 15 years ago. Then, I haven't really thought about this, there are other way to do it.
(In reply to comment #32) > (In reply to comment #24) > > > which bugs are you referring to? > > > > Add a glade C callback to your code while having the .h header file opened (by > > dragging and dropping the signal name to the source file). Switch to the .h > > header file tab to see how symbol list has been updated. Now switch back to the > > .c source file and press ctrl-z: symbol list gets confused. Retry it with many > > combinations of adding the signal callback, pressing ctrl-z and switching tabs. > > > > ctrl-z to undo the adding of signal? Yes. Ctrl-z and shift-ctrl-z with changes of tabs will frequently confuse the symbol-db plugin. > Is the file saved when you drag&drop the function? No. AFAICS, saving the file is the best way to have symbols correctly updated. > > > > > > > > perfectly. Considering that the old behavior wasn't ideal either, I argue that > > > > it should be committed as is and these bugs be fixed at a later moment > > > > (probably with Massimo help). > > > > > > > > > > There is a mechanism which is using TIMEOUT_INTERVAL_SYMBOLS_UPDATE to check > > > for updates on the editor on a scheduled interval. > > > Do you need to see changes of symbols in realtime when glade adds code to the > > > editor? > > > > What I'd like is a mechanism to say the symbol plugin "hey, there's a change in > > this editor. Please update its symbol list (preferably now!)." that works > > reliably. > > well the symbols updating process is not in real time, due to the use of ctags. > What's wrong with waiting N seconds before update the list? We can lower the > update-time if you'd like. > Or, better, you can force a save of the file when you've dragged your signal: > that would force symbol-db to automatically update the symbols of the file and > the view list. > > What do you think? I agree with Sébastien Granjoux on this.
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #32) > > > Or, better, you can force a save of the file when you've dragged your signal: > > > that would force symbol-db to automatically update the symbols of the file and > > > the view list. > > > > I think it's better avoid saving the file if it's not necessary because it's > > easier to cancel just close the editor without saving. > > > > Else, I think it is can be better to update the symbol on request instead of > > waiting a time out. I plan to commit these patches so tell me if you think > > something is not working. > > ok go ahead with patches. I'm just worrying a little bit about the high > coupling which is happening between symbol-db and the editor(s). > Massimo, I'd like to ask you to review the points of the patches that touch symbol-db (and if possible test it). If you see no problem with the changes, I'd say patches are finished.
I have committed your three patches but I don't see a big difference. Without it, I can already add members and signal handlers by double clicking on them in the glade properties pane if the file is the one currently open in the editor. Is there a new feature added with your first patch? Or do you plan new feature afterward? By the way, could you update the anjuta manual to explain how to use these new features. Then, the other patches improves the symbol refresh of symbol-db but it is still not working all the time. By example, if I delete the newly added signal handler, it's not removed from symbol-db if I don't save the file. I have read a bit the code and I have seen that if a refresh is requested while one is already running, the new one is canceled. I think, it should rather be remembered to be done when the current one is completed. Eventually the current can be aborted but it's could be a bit difficult to implement. Then, I have seen that symbol-db is already connected to the char-added signal so it can already refresh the symbol list when something is changed in the editor without waiting a save of the file. Why a new code-changed signal is needed?
(In reply to comment #38) > I have committed your three patches but I don't see a big difference. > > Without it, I can already add members and signal handlers by double clicking on > them in the glade properties pane if the file is the one currently open in the > editor. Is there a new feature added with your first patch? Or do you plan new > feature afterward? There feature allow to add members and signal handlers by double clicking on them in the glade properties pane even if the currently open editor the glade editor. > > By the way, could you update the anjuta manual to explain how to use these new > features. Will do. > > Then, the other patches improves the symbol refresh of symbol-db but it is > still not working all the time. By example, if I delete the newly added signal > handler, it's not removed from symbol-db if I don't save the file. The symbol-db plugin is still buggy. More work will be needed. Some of these bugs were already present before the changes have been committed. > > I have read a bit the code and I have seen that if a refresh is requested while > one is already running, the new one is canceled. I think, it should rather be > remembered to be done when the current one is completed. Eventually the current > can be aborted but it's could be a bit difficult to implement. > > Then, I have seen that symbol-db is already connected to the char-added signal > so it can already refresh the symbol list when something is changed in the > editor without waiting a save of the file. Why a new code-changed signal is > needed? The "char-added" signal is not triggered on undo/redo.
Created attachment 246015 [details] [review] Plug memory leaks in get_text_between There are two small memory leaks in get_text_between. The attached patch fixes them.
Created attachment 246016 [details] [review] Documentation fix for association between source and .ui files Documentation fix for association between source and .ui files.
I think the bug can be closed now.
Review of attachment 246015 [details] [review]: Thank you, I have committed your patch.
Review of attachment 246016 [details] [review]: Thank you, I have committed your patch.