Created attachment 229843 [details] [review]
symbol-db: Remove buffer update timeout when plugin gets deactivated.

Created attachment 229844 [details] [review]
document-manager:  Fix disconnection of GtkNotebook "switch-page" signal.

We previously tried to disconnect the signal on the AnjutaDocman instance but AnjutaDocman
is no longer a subclass of GtkNotebook.

Created attachment 229845 [details] [review]
document-manager: Remove autosave timeout when the plugin is deactivated.

Created attachment 229846 [details] [review]
build-basic-autotools: Disconnect all signals on editors when the plugin is deactivated.

Created attachment 229847 [details] [review]
anjuta-plugin-handle: Use g_hash_table_remove_all instead of g_hash_table_foreach_remove.

Created attachment 229848 [details] [review]
anjuta: Add new functions to peek for an object implementing an interface.

anjuta_shell_peek_object (AnjutaShell *shell, const gchar *iface_name) and
anjuta_shell_peek_interface (shell, iface_type).

These can be used to get an object that implements an interface without loading a plugin if it's
not already active.

Created attachment 229849 [details] [review]
run-program: Don't load the plugin implementing IAnjutaTerminal when disconnecting signals.

Use anjuta_shell_peek_interface () to get the IAnjutaTerminal object when disconnecting signals
so that we don't load the plugin if it's unloaded.

Created attachment 229850 [details] [review]
file-manager: Keep a weak reference to the IAnjutaVcs object in FileModel.

So that we don't call functions on it if it goes away.

Created attachment 229851 [details] [review]
file-manager: Unref the FileModel reference when the FileView gets finalized.

Created attachment 229852 [details] [review]
plugin-manager: Fix dependents checking on wrong object in should_unload().

The function should check if the plugin is one of the dependents of the plugin that is to be
unloaded and not if the plugin is dependent on itself.

Created attachment 229853 [details] [review]
plugin-manager: Clean up memory management.

1. Move cleanup from dispose to finalize. Calling functions on a disposed instance is not handled
     anyway.

2. Add a value destroy function to the activated_plugins and plugins_cache hash tables. Have
    the hash tables always owning one reference to the plugin.

3. Use g_hash_table_remove_all() and g_hash_table_destroy() instead of
    g_hash_table_foreach_remove().

Created attachment 229854 [details] [review]
plugin-manager: Make anjuta_plugin_manager_unload_all_plugins() not show any dialogs.

anjuta_plugin_manager_unload_all_plugins() is used to unload plugins before closing an
Anjuta instance/window so it does not make sense to show any dialogs.

Created attachment 229855 [details] [review]
plugin-manager: Don't destroy available_plugins order in populate_plugin_model().

The available_plugins list is sorted in dependency order so we should not destroy the ordering
by sorting it by name in populate_plugin_model().

Created attachment 229856 [details] [review]
profile-manager: Add new close() function.

/**
 * anjuta_profile_manager_close:
 * @profile_manager: A #AnjutaProfileManager object.
 *
 * Close the #AnjutaProfileManager making all further calls no-ops. This
 * is to be used before unloading all plugins when destroying an Anjuta instance.
 * This way plugins can call into #AnjutaProfileManager during deactivation
 * without triggering further action.
 */

Created attachment 229857 [details] [review]
anjuta: unload all plugins before closing a window.

Since all Anjuta windows are in the same process it's now important that we properly unload
all plugins when destroying the window. If we don't the plugins may have active
timeouts/asynchronous operations etc. that when triggered reference plugins/widgets that
are no longer alive.

Created attachment 229858 [details] [review]
anjuta: Fix destruction of the AnjutaUI instance.

AnjutaUI is a not a GtkWidget so call g_object_unref() instead of gtk_widget_destroy() on
it. Also disconnect the signal handlers from the GdlLayoutManager when the AnjutaWindow
is disposed so we don't get any stray signals.

Review of attachment 229843 [details] [review]:

Thanks for this patch series, it's quite impressive.

Then, on this patch. Isn't it better to add this code in the the dispose function of the plugin object?
Because all plugins are not deactivate by default when a main window is closed. In fact, it's done only if the command line flags proper-shutdown is used.

Review of attachment 229845 [details] [review]:

I have the same remark than with the first patch. I think it's better to add this in the dispose function or at least add it in the dispose function too.

(In reply to comment #17)
> Review of attachment 229843 [details] [review]:
> 
> Thanks for this patch series, it's quite impressive.
> 
> Then, on this patch. Isn't it better to add this code in the the dispose
> function of the plugin object?
> Because all plugins are not deactivate by default when a main window is closed.
> In fact, it's done only if the command line flags proper-shutdown is used.

I think it's kind of bad that deactivate() is not guaranteed to be called when a plugin is unloaded since it means that code has to be duplicated between deactivate and dispose/finalize.

I think we should always atleast try to run deactivate but not care if it fails. The patch https://bugzilla.gnome.org/attachment.cgi?id=229857 fixes so that we always run deactivate on the currently activated plugins when closing a window. 

To be honest I don't really get the point of the proper-shutdown option since if we were just to exit as fast as possible it would be much easier to just call exit() and not do the semi-shutdown we do now.

Review of attachment 229846 [details] [review]:

It could be fine to do this in the deactivate function of the plugin but only if destroying the main window doesn't activate those signals. Have you check it's fine?

In theory it would be better to take less care in the dispose function to be able to destroy the object faster, we could even accept some memory leak but it shouldn't cause a crash or even a critical warning. So in practise, I'm not sure it make a big different in speed and it's more complex to check to different code paths so I'm not sure it worths it. But it's already done like this, we can keep it for the moment.

Comment on attachment 229844 [details] [review]
document-manager:  Fix disconnection of GtkNotebook "switch-page" signal.

Thanks for your patch.

Comment on attachment 229847 [details] [review]
anjuta-plugin-handle: Use g_hash_table_remove_all instead of g_hash_table_foreach_remove.

Thanks.

(In reply to comment #19)
> I think it's kind of bad that deactivate() is not guaranteed to be called when
> a plugin is unloaded since it means that code has to be duplicated between
> deactivate and dispose/finalize.

I'm agree that it's more complex to not be sure that deactivate will be called. There are perhaps some reasons to do it like though but I don't know them. 


> I think we should always atleast try to run deactivate but not care if it
> fails. The patch https://bugzilla.gnome.org/attachment.cgi?id=229857 fixes so
> that we always run deactivate on the currently activated plugins when closing a
> window. 

Ok, I will look at this patch.


> To be honest I don't really get the point of the proper-shutdown option since
> if we were just to exit as fast as possible it would be much easier to just
> call exit() and not do the semi-shutdown we do now.

I don't know what is the point of having this option neither. I have kept it especially because the session are not saved when it is on. But I'm agree that we can remove it unless someone find a good reason to keep it. We need to check that it's working fine when a proper shutdown is done.

Review of attachment 229848 [details] [review]:

I think this function can be useful but I don't think it's related to the current issue.

I see you have used it in the terminal plugin, is there other use of this function? or do you plan to use it somewhere else?

Review of attachment 229849 [details] [review]:

Thanks for I think it is better to keep a reference on the IAnjutaTerminal plugin in the RunProgramPlugin object and use this object in run_free_all_children and run_plugin_child_free instead of getting a new reference on it. In this way, you are sure that you connect and disconnect signal on the same object even if I'm not sure that having a different is possible currently. Do you agree with this?

Review of attachment 229850 [details] [review]:

Thanks for your patch. I have just committed it.

(In reply to comment #25)
> Review of attachment 229849 [details] [review]:
> 
> Thanks for I think it is better to keep a reference on the IAnjutaTerminal
> plugin in the RunProgramPlugin object and use this object in
> run_free_all_children and run_plugin_child_free instead of getting a new
> reference on it. In this way, you are sure that you connect and disconnect
> signal on the same object even if I'm not sure that having a different is
> possible currently. Do you agree with this?

Yeah I think that makes sense. Perhaps a weak reference is even better? No need to keep the plugin alive to just disconnect signals?

Anyway, perhaps AnjutaPluginManager should also be changed so that plugins are never loaded/activated after anjuta_plugin_manager_unload_all_plugins() has been called?

Comment on attachment 229852 [details] [review]
plugin-manager: Fix dependents checking on wrong object in should_unload().

Thanks for this patch. I have committed it. It was a very old error.

Comment on attachment 229851 [details] [review]
file-manager: Unref the FileModel reference when the FileView gets finalized.

Thanks, it is committed.

Created attachment 230025 [details] [review]
run-program: Keep a weak reference to the IAnjutaTerminal we run the program in.

So that we can disconnect our signal handlers from it without reloading it if it has already
been unloaded.

Created attachment 230029 [details] [review]
anjuta: Fix destruction of AnjutaPreferences instance.

AnjutaPreferences is not a GtkWidget so it should not be destroyed with
gtk_widget_destroy().

Comment on attachment 230025 [details] [review]
run-program: Keep a weak reference to the IAnjutaTerminal we run the program in.

Thanks for your patch. I have just added a set of terminal to NULL in the instance init function.

Comment on attachment 229855 [details] [review]
plugin-manager: Don't destroy available_plugins order in populate_plugin_model().

Thanks, indeed it looks better like this. I have committed it.

Comment on attachment 229858 [details] [review]
anjuta: Fix destruction of the AnjutaUI instance.

Thanks I have missed this when I have updated AnjutaWindow recently. I have committed your patch.

Review of attachment 230029 [details] [review]:

Thanks you patch is good. The current code is obviously wrong and I think it's better to use only the dispose function. But AnjutaPreferences object is a bad singleton which doesn't add a reference when you call anjuta_preferences_new a second time, so it was better with the wrong code that doesn't release the object.

That's easy to fix but then I don't know if we can keep it as a singleton. I would prefer to keep it as a singleton, it's more work, I think that's fine if the design is beter, but I'm not sure it's possible. Each plugin object should be local to each Anjuta Window, then I don't know if we can share the same plugin manager object. What do you think?

(In reply to comment #32)
> (From update of attachment 230025 [details] [review])
> Thanks for your patch. I have just added a set of terminal to NULL in the
> instance init function.

That's not strictly needed since GObject zero initialises the struct anyway.

(In reply to comment #36)
> That's not strictly needed since GObject zero initialises the struct anyway.

Yes, you are right, I have forgotten this.

Review of attachment 229853 [details] [review]:

I don't agree with your first point, calling a function on a dispose instance is defined and it must do nothing. I think it's better to keep all cleanup in the dispose function if possible. I think it's replace the closed state that you have added in a later patch or do you see an issue?

Else the remaining of your patch is just fine.

Created attachment 230043 [details] [review]
build-basic-autotools: Remove idle for updating indicators when plugin is deactivated.

Also fix compile warning regarding zero parameter functions.

Review of attachment 229856 [details] [review]:

This really looks like the behavior of a disposed object. Isn't it possible to use dispose instead?

Review of attachment 229854 [details] [review]:

Thank for you patch.
I don't like the additional show_errors_in_ui argument. It's working fine but it looks a bit clumsy.

I have looked at the plugin_set_update function and most of the code is not needed when you want to unload all plugins. I think it would be better to call directly anjuta_plugin_deactivate in anjuta_plugin_manager_unload_all_plugins. What do you think?

(In reply to comment #41)
> Review of attachment 229854 [details] [review]:
> 
> Thank for you patch.
> I don't like the additional show_errors_in_ui argument. It's working fine but
> it looks a bit clumsy.
> 
> I have looked at the plugin_set_update function and most of the code is not
> needed when you want to unload all plugins. I think it would be better to call
> directly anjuta_plugin_deactivate in anjuta_plugin_manager_unload_all_plugins.
> What do you think?

Yeah, I agree that that's cleaner and better. I'll attach a new patch that does that.

(In reply to comment #40)
> Review of attachment 229856 [details] [review]:
> 
> This really looks like the behavior of a disposed object. Isn't it possible to
> use dispose instead?

I guess dispose could be (ab)used for that but I really think the dispose functions purpose is to break reference cycles and should not be used for other things.

(In reply to comment #43)
> I guess dispose could be (ab)used for that but I really think the dispose
> functions purpose is to break reference cycles and should not be used for other
> things.

I have confused profile manager and the plugin manager. My remarks was rather on the plugin manager and linked to your attachement 229853. On the profile manager, it would be better if we don't need this function but I don't know how to do it in another way and I'm agree that dispose is not right.

I will think about it. Anyway thanks again for all this work.

(In reply to comment #35)
> Review of attachment 230029 [details] [review]:
> 
> Thanks you patch is good. The current code is obviously wrong and I think it's
> better to use only the dispose function. But AnjutaPreferences object is a bad
> singleton which doesn't add a reference when you call anjuta_preferences_new a
> second time, so it was better with the wrong code that doesn't release the
> object.
> 
> That's easy to fix but then I don't know if we can keep it as a singleton. I
> would prefer to keep it as a singleton, it's more work, I think that's fine if
> the design is beter, but I'm not sure it's possible. Each plugin object should
> be local to each Anjuta Window, then I don't know if we can share the same
> plugin manager object. What do you think?

I think it's probably easiest to have one AnjutaPreferences object per window.
Sharing it between the windows will probably be quite hard.

Created attachment 230049 [details] [review]
plugin-manager: Directly use anjuta_plugin_deactivate in unload_all_plugins().

Instead of using plugin_set_update() which may show dialogs and does a lot more than
we need when we deactivate all the plugins.

(In reply to comment #45)
> I think it's probably easiest to have one AnjutaPreferences object per window.
> Sharing it between the windows will probably be quite hard.

Sure. Ok could you update your patch to remove the AnjutaPreference singleton. I suppose it will work fine. Then, we can make a singleton later if we have time or see additional advantages.

Created attachment 230126 [details] [review]
sourceview: Don't store the builder for the preferences in a global variable.

There's no need to keep the builder alive since anjuta_preferences_add_from_builder()
will keep the widgets alive and we can just store all the widgets we want to interact with
in the plugin object.

This fixes a crasher when more than one preferences window is used at the same time for
different windows.

Created attachment 230127 [details] [review]
anjuta: Fix destruction of AnjutaPreferences instance.

AnjutaPreferences is not a GtkWidget so it should not be destroyed with
gtk_widget_destroy(). Also make it not a singleton anymore since this causes
some hard to solve problems.

Comment on attachment 230127 [details] [review]
anjuta: Fix destruction of AnjutaPreferences instance.

Thanks for your update, I have committed your patch.

Comment on attachment 230126 [details] [review]
sourceview: Don't store the builder for the preferences in a global variable.

Thanks, I have committed your patch.

Created attachment 230135 [details] [review]
plugin-manager: Clean up memory management.

1. Add a value destroy function to the activated_plugins and
   plugins_cache hash tables. Have the hash tables always owning one reference
   to the plugin.

2. Use g_hash_table_remove_all() and g_hash_table_destroy() instead of
   g_hash_table_foreach_remove().

Comment on attachment 230135 [details] [review]
plugin-manager: Clean up memory management.

Thank for your updated patch.

Comment on attachment 230049 [details] [review]
plugin-manager: Directly use anjuta_plugin_deactivate in unload_all_plugins().

Thanks.

Review of attachment 229856 [details] [review]:

I have tried to find another way to handle the deactivation of all plugins which doesn't need this close function but I haven't found a better way to do it. So I think now it's a good idea unless you have another solution.

Then, I'm not sure it's needed to make all further calls no-ops. Isn't it possible to use the already existing freeze function if needed?

I think the profile manager could take care of unloading all plugins in the close function too.

The call anjuta_shell_notify_exit could probably be removed. I think it looks like a work around allowing to save the project session without using profile-descoped signal.

This is still not completely clear for me. I think some improvements in the documentation of libanjuta would be useful to explain how this is working.

(In reply to comment #55)
> Review of attachment 229856 [details] [review]:
> 
> I have tried to find another way to handle the deactivation of all plugins
> which doesn't need this close function but I haven't found a better way to do
> it. So I think now it's a good idea unless you have another solution.
> 
> Then, I'm not sure it's needed to make all further calls no-ops. Isn't it
> possible to use the already existing freeze function if needed?
> 
> I think the profile manager could take care of unloading all plugins in the
> close function too.
> 
> The call anjuta_shell_notify_exit could probably be removed. I think it looks
> like a work around allowing to save the project session without using
> profile-descoped signal.
> 
> This is still not completely clear for me. I think some improvements in the
> documentation of libanjuta would be useful to explain how this is working.

The reason I changed all functions to no-ops is that when the project manager is deactivated it calls anjuta_profile_manager_pop() which causes the previous profile to be loaded, thus activating the plugins of that profile. anjuta_profile_manager_pop() does not check the freeze_count at all but I guess that it could be fixed to check it.

You're right that anjuta_shell_notify_exit() can be removed since only the git and project-manager plugins use the "exiting" signal. The git plugin doesn't need it anymore since plugins always gets deactivated and the project-manager plugin doesn't need it since "profile-descoped" will always get emitted during destruction.

Created attachment 230249 [details] [review]
anjuta: Remove "exiting" signal from AnjutaShell.

It's not needed anymore since plugins will always be deactivated during destruction of a
AnjutaWindow and the AnjutaProfileManager "profile-descoped" signal will also be
emitted when a AnjutaWindow is destroyed.

Review of attachment 229856 [details] [review]:

I have done some checks here. I think adding this anjuta_profile_manager_close function is a good idea.

But I think the is_closed flag is not useful. In anjuta_profile_manager_close function you already clear profiles and profiles_queue, so the anjuta_profile_manager_pop function will have no effect anyway. I think it's would be good to just remove the g_warning as it will be a normal situation when closing the profile manager. I think the anjuta_profile_manager_push will not be called when a plugin is deactivated.

Then, in the anjuta_profile_manager_close, I think it would be better to call anjuta_plugin_manager_unload_all_plugins. The AnjutaProfileManager object manage the loading and unloading of the plugins, I think it makes sense here.

Could you do these changes? or do you have other ideas?

Review of attachment 229857 [details] [review]:

After adding anjuta_profile_manager_close, I think this patch is right but we can do more simplifications:

* remove anjuta_shell_notify_exit
You have already done it in another patch, so that's ok. In fact, I have in mind to still keep the notification but just remove its current usage in the git and project manager plugin as it's used really as a work around because deactivated was not called. Anyway, that's fine to completely remove it if you think it's better. I think it was not use correctly.

* Move anjuta_plugin_manager_unload_all_plugins in profile manager

* The explicite session save in on_anjuta_delete_event is not needed

/* If current active profile is "user", save current session as
 * default session
 */
current_profile = anjuta_profile_manager_get_current (profile_manager);
if (strcmp (anjuta_profile_get_name (current_profile), "user") == 0)
{
	gchar *session_dir;
	session_dir = USER_SESSION_PATH_NEW;
	anjuta_shell_session_save (ANJUTA_SHELL (win), session_dir, NULL);
	g_free (session_dir);
}

anjuta_profile_manager_close will emit the profile-descoped signal, the current session with be save by the already existing on_profile_descoped function if needed.


I think the saving of the remembered plugins and the check for unsaved data can stay here.

What do you think about this? If you think everything is right can you do the change?

(In reply to comment #59)

I think your suggestions make sense I will update the patches accordingly.

> * remove anjuta_shell_notify_exit
> You have already done it in another patch, so that's ok. In fact, I have in
> mind to still keep the notification but just remove its current usage in the
> git and project manager plugin as it's used really as a work around because
> deactivated was not called. Anyway, that's fine to completely remove it if you
> think it's better. I think it was not use correctly.
> 

I think it's best to remove it completely if we don't have a usecase for it. No need to keep API that's unused anyway.

(In reply to comment #58)
> Review of attachment 229856 [details] [review]:
> 
> I have done some checks here. I think adding this anjuta_profile_manager_close
> function is a good idea.
> 
> But I think the is_closed flag is not useful. In anjuta_profile_manager_close
> function you already clear profiles and profiles_queue, so the
> anjuta_profile_manager_pop function will have no effect anyway. I think it's
> would be good to just remove the g_warning as it will be a normal situation
> when closing the profile manager. I think the anjuta_profile_manager_push will
> not be called when a plugin is deactivated.

Ok,  I will remove the is_closed parameter.

> Then, in the anjuta_profile_manager_close, I think it would be better to call
> anjuta_plugin_manager_unload_all_plugins. The AnjutaProfileManager object
> manage the loading and unloading of the plugins, I think it makes sense here.

One problem with calling anjuta_plugin_manager_unload_all_plugins() in anjuta_profile_manager_close() is that we want to hide the AnjutaWindow after "profile-descoped" is emitted but before we start to unload all the plugins. The reason we can't hide the window before "profile-descoped" is emitted is that anjuta-window.c:on_session_save() depends on that the window is still visible.

(In reply to comment #61)
> One problem with calling anjuta_plugin_manager_unload_all_plugins() in
> anjuta_profile_manager_close() is that we want to hide the AnjutaWindow after
> "profile-descoped" is emitted but before we start to unload all the plugins.
> The reason we can't hide the window before "profile-descoped" is emitted is
> that anjuta-window.c:on_session_save() depends on that the window is still
> visible.

Ok, then we can keep the call of anjuta_plugin_mananger_unload_all_plugins outside anjuta_profile_manager_close().

Created attachment 230381 [details] [review]
profile-manager: Add new close() function.

/**
 * anjuta_profile_manager_close:
 * @profile_manager: A #AnjutaProfileManager object.
 *
 * Close the #AnjutaProfileManager causing "profile-descoped" to be emitted and
 * all queued and previous profiles to be released. This function is to be used
 * when destroying an Anjuta instance.
 */

Created attachment 230382 [details] [review]
anjuta: unload all plugins before closing a window.

Since all Anjuta windows are in the same process it's now important that we properly unload
all plugins when destroying the window. If we don't the plugins may have active
timeouts/asynchronous operations etc. that when triggered reference plugins/widgets that
are no longer alive.

Created attachment 230383 [details] [review]
anjuta: Remove "exiting" signal from AnjutaShell.

It's not needed anymore since plugins will always be deactivated during destruction of a
AnjutaWindow and the AnjutaProfileManager "profile-descoped" signal will also be
emitted when a AnjutaWindow is destroyed.

Comment on attachment 230381 [details] [review]
profile-manager: Add new close() function.

Thank.

I have committed all your patches. But I'm afraid there are still some plugins to fix, so I will keep this bug open unless you prefer to create another one.

Hi!

> Sure. Ok could you update your patch to remove the AnjutaPreference singleton.
> I suppose it will work fine. Then, we can make a singleton later if we have
> time or see additional advantages.

In practice AnjutaPreferences probably doesn't need to be a singleton anymore as it is used for the preference dialog only now and everything else is directly handled by GSettings (which is a singleton anyway).

Created attachment 233107 [details] [review]
libanjuta: Actually free stuff in AnjutaPkgConfigChooser::finalize.

https://bugzilla.gnome.org/show_bug.cgi?id=680954

Created attachment 233108 [details] [review]
git: Fix destruction of GitLogPane.

1. Disconnect signal handler from ref_command.

2. Free the commands GitLogPane created by itself if they're still alive when GitLogPane is
    destroyed.

3. Remove spin timer source.

https://bugzilla.gnome.org/show_bug.cgi?id=680954

Created attachment 233109 [details] [review]
libanjuta: Actually free stuff in AnjutaPkgConfigChooser::finalize.

Created attachment 233110 [details] [review]
git: Fix destruction of GitLogPane.

1. Disconnect signal handler from ref_command.

2. Free the commands GitLogPane created by itself if they're still alive when GitLogPane is
    destroyed.

3. Remove spin timer source.

Created attachment 233111 [details] [review]
libanjuta: Actually free stuff in AnjutaPkgConfigChooser::finalize.

Created attachment 233112 [details] [review]
git: Fix destruction of GitLogPane.

1. Disconnect signal handler from ref_command.

2. Free the commands GitLogPane created by itself if they're still alive when GitLogPane is
    destroyed.

3. Remove spin timer source.

Review of attachment 233111 [details] [review]:

Thanks, this patch looks fine, could you commit it?

Review of attachment 233112 [details] [review]:

Thanks for your patch.

Are you sure that the functions refresh_log, on_log_view_row_selected and on_ref_command_finished cannot be called before their corresponding command is finished? In this case you will overwrite the previous command, I have no idea if it can happen though.

If it is a possibility, I think you could add only a list to a GitLogPane object and add all commands to this list when you create a new one. Then when a command is completed, you remove it from the list and destroy it. You can destroy all commands left in the list when the log pane is destroyed.

Then perhaps James will have more comments.

Review of attachment 233111 [details] [review]:

Thanks for the review, I've commited it now.

(In reply to comment #76)
> Review of attachment 233112 [details] [review]:
> 
> Thanks for your patch.
> 
> Are you sure that the functions refresh_log, on_log_view_row_selected and
> on_ref_command_finished cannot be called before their corresponding command is
> finished? In this case you will overwrite the previous command, I have no idea
> if it can happen though.
> 
> If it is a possibility, I think you could add only a list to a GitLogPane
> object and add all commands to this list when you create a new one. Then when a
> command is completed, you remove it from the list and destroy it. You can
> destroy all commands left in the list when the log pane is destroyed.
> 
> Then perhaps James will have more comments.

You're right I think those functions can be called before their corresponding commands are finished. Perhaps it's possible to just unref the previous command in that case? Maybe we should disconnect the signals too to be on the safe side?

GitCommand seem to unref the AnjutaLauncher it uses to execute the program when its finalized. The AnjutaLauncher in turn removes the GSources connected to the executed program when it's finalized. So I think unreffing the previous command should be enough to not receive any further signals from the command.

Anyway I don't think it's a good idea to have multiple commands running at the same time. It seems to me that they would step on each others toes plus that you really don't need the result of the previous command if you already scheduled a new one.

(In reply to comment #78)
> You're right I think those functions can be called before their corresponding
> commands are finished. Perhaps it's possible to just unref the previous command
> in that case? Maybe we should disconnect the signals too to be on the safe
> side?

I think just unref them is enough. I think you don't need to disconnect the signal because it will be destroy with the command.


> Anyway I don't think it's a good idea to have multiple commands running at the
> same time. It seems to me that they would step on each others toes plus that
> you really don't need the result of the previous command if you already
> scheduled a new one.

I think keeping the old commands is a bit safer because it keeps the same behavior. But I don't think it's really needed and I'm agree that it is cleaner to unref the aborted command.

Created attachment 233351 [details] [review]
git: Fix destruction of GitLogPane.

1. Disconnect signal handler from ref_command.

2. Free the commands GitLogPane created by itself if they're still alive when GitLogPane is
    destroyed.

3. Remove spin timer source.

Review of attachment 233351 [details] [review]:

Thanks, I think it's good like this now. You can commit it.

Comment on attachment 233351 [details] [review]
git: Fix destruction of GitLogPane.

Attachment 233351 [details] pushed as e74acc6 - git: Fix destruction of GitLogPane.

bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all old bug reports in Bugzilla which have not seen updates for many years.

If you can still reproduce this issue in a currently supported version of GNOME (currently that would be 3.38), then please feel free to report it at https://gitlab.gnome.org/GNOME/anjuta/-/issues/

Thank you for reporting this issue and we are sorry it could not be fixed.