GNOME Bugzilla – Bug 578511
After Svn update the files' icons don't get updated
Last modified: 2012-03-05 18:14:49 UTC
If I do from gui subversion -> update, the icons' status of the files don't change. I have to fold/unfold that directory to see something changing. A real-time update whould be really appreciated by users.
*** Bug 578510 has been marked as a duplicate of this bug. ***
I think the best would be to add some signal to the version control plugins like: @files: list of GFile* with changed status ::status_changed (GList* files) The version control plugins should emit this once the status changes.
(In reply to comment #2) > I think the best would be to add some signal to the version control plugins > like: > > @files: list of GFile* with changed status > ::status_changed (GList* files) > > The version control plugins should emit this once the status changes. > yes, indeed it does make sense.
That solution would probably be the only practical way we could do it for subversion, but for git we can accomplish this just by monitoring one folder
As another solution, we could probably add a hook script to the subversion repository that notifies us of any changes. Doing that would let us know about command line originated changes as well...
(In reply to comment #5) > As another solution, we could probably add a hook script to the subversion > repository that notifies us of any changes. Doing that would let us know about > command line originated changes as well... do you mean to place some scripts on server-side? I don't think that's a good idea, it's better to have a local control of the files.
(In reply to comment #6) > (In reply to comment #5) > > As another solution, we could probably add a hook script to the subversion > > repository that notifies us of any changes. Doing that would let us know about > > command line originated changes as well... > > do you mean to place some scripts on server-side? I don't think that's a good > idea, it's better to have a local control of the files. Of course not, I'm referring only to the local copy here...
I think we should really fix that for operations done through the git plugin for 3.0. If someone accesses the version control from outside he can expect that thing will not update, but if he does it inside anjuta, it really should work. I would propose a "status-changed" signal that the vcs plugin emits and the file-manager uses to update all file status. It will be emitted for operations that don't touch files but change their VCS status, most notably "commit.
Created attachment 185469 [details] [review] patch fixing this issue This patch: file-manager/plugin.c | 11 +++++++++++ git/Makefile.am | 4 +++- git/git-pull-pane.c | 4 ++++ git/plugin.c | 3 +++ 4 files changed, 21 insertions(+), 1 deletion(-) I introduce a new signal in the new files I will attach, that is emited in the git-pull-pane.c, after the command is finished, and than connected and handled in the file manager plugin.c file. It refreshes the tree without the need to fold and unfold it. The files are included in the Makefile.am file, and I've also included the git-status-changed.h file in the plugin.c file in the git plug in.
Created attachment 185470 [details] [review] new signal This new file should be placed in the anjuta/plugins/git directory.
Created attachment 185471 [details] [review] header file for the new signal This new file should be placed in the anjuta/plugins/git directory.
Review of attachment 185469 [details] [review]: Overall I think the idea you did this is pretty good. The signal should be declared in the interface file (see below). ::: plugins/file-manager/plugin.c @@ +136,3 @@ + "status-changed", + (GCallback)refresh_signal_cb, + NULL); I think for this to work, you will need to pass file_manager instead of NULL. Note also that the first argument for the callback is always the object that emitted the signals, in this case the ivcs interface object. ::: plugins/git/plugin.c @@ +889,3 @@ klass->dispose = git_dispose; + + git_status_changed_init(klass); You shouldn't do that here. You can define the signal in the IAnjutaVcs interface (in libanjuta/interfaces/libanjuta.idl) without having to do any coding.
Created attachment 185789 [details] [review] follow up on the previously posted patch Johannes, I took your remarks into consideration and changed the patch. Now the signal is defined in the AnjutaVcs interface (in libanjuta/interfaces/libanjuta.idl), and I've also changed the plugins/file-manager/plugin.c file and repaired my previous mistakes. I also removed the two new files I introduced in the previous version of my solution, because they are now obsolete.
Review of attachment 185789 [details] [review]: ::: plugins/file-manager/plugin.c @@ +101,3 @@ +{ + AnjutaFileManager* file_manager = (AnjutaFileManager*) data; + file_view_refresh(file_manager->fv); Instead of refreshing the whole file-manager it would be nice to only update the VCS status of the files. This might not be that easy and maybe it is not super useful but it would be the cleaner solution. file-model.c has the code the queries/updates the VCS status, maybe you can have a look there. ::: plugins/git/plugin.h @@ +102,3 @@ }; +void git_status_changed_emit(AnjutaPlugin *plugin); I would make sense to rename this to git_plugin_statis_changed_emit (GitPlugin *plugin);
Created attachment 186437 [details] [review] updated patch fixing 578511 bug Johannes, I updated the patch with your considering your suggestions. About making the solution cleaner by using the functions from file-model.c, I've working to find a solution, but I haven't figured out how from git command to get list of changed files; and if there is any documentation about the functions from file-model.c, because it would be very helpful.
Created attachment 186438 [details] [review] updated patch fixing 578511 bug Johannes, I updated the patch with your considering your suggestions. About making the solution cleaner by using the functions from file-model.c, I've working to find a solution, but I haven't figured out how from git command to get list of changed files; and if there is any documentation about the functions from file-model.c, because it would be very helpful.
Review of attachment 186438 [details] [review]: OK, so this patch looks very good overall. Good coding style! Thanks! Two things you could add to make this even better: * It would be nice if the same as in the git plugin could be implemented for the Subversion plugin. It implements the same interface anyway so this should be rather trivial * For the file-manager you could try to code a file_model_update_vcs_status() method (in file-model.h/c) that does this pseudo-code: { gtk_tree_model_foreach (file_model_get_vcs_status ()) or maybe file_model_update_file() } This operation might take a moment, still we first want to code a synchronous version. In case it isn't fast enough we might want to think about an async version but that might be non-trivial. Could you please make further patches as seperate commits! Thanks!
Created attachment 188822 [details] [review] new verzion of the git 578511 bug fix I made the suggested changes. If it is finished now, I can start making a similar patch for the subversion plugin. Also I have to note that although you suggested: " ::: plugins/git/plugin.h @@ +102,3 @@ }; +void git_status_changed_emit(AnjutaPlugin *plugin); I would make sense to rename this to git_plugin_statis_changed_emit (GitPlugin *plugin); " It has to stay with AnjutaPlugin *plugin because it only functions like that.
Review of attachment 188822 [details] [review]: Looks good overall! But I think more git commands need to be considered when updating the status than pull. At least "commit" comes into my mind, more might be necessary. ::: plugins/file-manager/plugin.c @@ +100,3 @@ +refresh_signal_cb(gpointer data) +{ + AnjutaFileManager* file_manager = (AnjutaFileManager*) data; Note to myself: There should be proper ANJUTA_FILE_MANAGER() cast macros...
Created attachment 188903 [details] [review] extended patch that now beside pull features commit and push I made the necessary changes and the patch now features commit and push beside pull. I will start working now on subversion plugin.
Review of attachment 188903 [details] [review]: First, I am really disappointed that you didn't test the patch at all as it doesn't work. You can ask any questions and as many as you want but if you create a patch (and don't state that it doesn't work) I expect that it is at least filling it's purpose. I have made quite a couple of changes, you can see the end result here: http://git.gnome.org/browse/anjuta/commit/?id=4995ecc31f504d01361e1f2b4ee2f18427d1d0f5 You also got quite a couple of compiler warnings that showed the something was wrong. Please take care to fix all warnings, the compiler is your friend. Please at least create an appropriate and working patch for the subversion plugin, that shouldn't be much work. ::: plugins/file-manager/file-model.c @@ +404,3 @@ +void file_model_update_vcs_status (FileModel* model) +{ + gtk_tree_model_foreach (model, file_model_update_file, NULL); This cannot work as file_model_update_file has a completly different signature from GtkTreeModelForeachFunc. See the wrapper method I created in the patch. ::: plugins/file-manager/plugin.c @@ +100,3 @@ +refresh_signal_cb(gpointer data) +{ + AnjutaFileManager* file_manager = (AnjutaFileManager*) data; The signature of the status changed signal is status_changed (IAnjutaVcs, gpointer data) so the first argument of the method that you connect is not the plugin but the IAnjutaVcs argument. Thus, this results in an invalid cast and a crash. ::: plugins/git/git-commit-pane.c @@ +184,3 @@ + + g_signal_connect (G_OBJECT (commit_command), "command-finished", + G_CALLBACK (git_plugin_status_changed_emit), The signature of command-finished is command_finished (AnjutaCommand, int, data) so again this results in a crash because git_plugin_status_changed_emit expects the plugin to be the first argument. ::: plugins/git/git-pull-pane.c @@ +93,3 @@ + + g_signal_connect (G_OBJECT (pull_command), "command-finished", + G_CALLBACK (git_plugin_status_changed_emit), dito ::: plugins/git/git-push-pane.c @@ +135,3 @@ + + g_signal_connect (G_OBJECT (push_command), "command-finished", + G_CALLBACK (git_plugin_status_changed_emit), dito! ::: plugins/git/plugin.c @@ +891,3 @@ + +void git_plugin_status_changed_emit(AnjutaPlugin* plugin){ + g_signal_emit_by_name(plugin, "status_changed"); See how the correct signature is in the linked commit. ::: plugins/git/plugin.h @@ +102,3 @@ }; +void git_plugin_status_changed_emit(AnjutaPlugin* plugin); dito.
One last thing, could you please make sure to update your git configuration to have your full name and e-mail as explained in https://live.gnome.org/Git/Help/AuthorEmail Thanks!
I have no compiler warning messages when I run it at my computer. Also, my initial patch that I thought of myself worked great. I tested it many times before sending it as my initial contribution. It updated the tree without the need to unfold it, it refreshed it. The changes you suggested and I commited on 2011-05-29 gave no compiler warning errors so I assumed that it worked also. I was very confused about this as you can see from my previous messages and I didn't really understood what I was needed to do. I made some changes, trying as much as I can to get familiar with the code and implement the way documentation told me to. After I got the response "Looks good overall!" from you I thought it was ok. Who else I have to point me that something I think works doesn't? I guess I misunderstood this mentorship thing.
Please just double check my code in the future commits. There are so many things I don't know, and this is my first experience on a project like this. Thank you for the changes on the commit, sorry about the inconvenience.
update: I was building the project only in the terminal by now, and I must have missed the warnings...I ran it in Anjuta and it highlighted a few (4). I went trough your code thoroughly and I finally understood what I needed to do with updating the vcs status. I had totally misunderstood it earlier, that is why my "tried to fit" code was so useless.
> The changes you suggested and I commited on 2011-05-29 gave no compiler warning > errors so I assumed that it worked also. I was very confused about this as you > can see from my previous messages and I didn't really understood what I was > needed to do. I made some changes, trying as much as I can to get familiar with > the code and implement the way documentation told me to. After I got the > response "Looks good overall!" from you I thought it was ok. > > Who else I have to point me that something I think works doesn't? > I guess I misunderstood this mentorship thing. I think you misunderstood my critics. I don't expect that you produce perfect code from the beginning on, actually nobody probably produces perfect code. But what I expect to be able to do effective mentoring is that you ask questions when you are confused and that you ask as long as something isn't clear to you. There are absolutely no stupid questions, software development on a new codebase isn't easy but you need to ask those questions because I cannot know in advance which problems you will run into. I am actually sorry that I didn't test every iteration of the patch but I expected that you did run basic tests on the functionality for every iteration. With code review of a patch it is not always easy to spot errors. The one that I tested had, apart from compiler warnings that I think you know now how to deal with, a couple of run-time criticals on the command-line. Something that almost always means something is wrong. Normally I don't finish patches while mentoring because it doesn't have the best learning effect. But I needed quite some testing and fixing to find out what was going on and what was going wrong that didn't want to throw away my end result. I am not resentful in any way so just keep in mind what I told you about testing, asking and compiler warnings and I am pretty sure that you will be able to make a lot of good contributions.
Created attachment 191944 [details] [review] Changes to subversion plugin (commit and update) that emit a signal that updates the vcs status of the files and with it the icons of the files, without the need to fold and unfold the tree.
Review of attachment 191944 [details] [review]: Otherwise, looks fine. ::: plugins/subversion/plugin.h @@ +28,3 @@ #include <libanjuta/interfaces/ianjuta-editor.h> #include <svn_client.h> +#include <gio/gio.h> These includes should be rather in the plugin.c file and I don't think they are all needed.