Created attachment 76049 [details] [review]
patch detecting if the file on the disk changed, when the gedit window is focused in

Comment on attachment 76049 [details] [review]
patch detecting if the file on the disk changed, when the gedit window is focused in

Thanks for you patch.
I think the feature you implemented is very interesting and it has been in our todo list for a long time.

I have not reviewed the patch in all details, but I already have a few comments for you.

> 
>+	gboolean     user_notified_that_file_changed_on_disk;
>+

What is "user_notified_that_file_changed_on_disk" used for?


>+gboolean gedit_document_changed_on_disk (GeditDocument *doc)
>+{
>+	GnomeVFSFileInfo *info;
>+	GnomeVFSResult result;
>+	gint file_mtime;
>+	g_return_val_if_fail (GEDIT_IS_DOCUMENT (doc), FALSE);
>+
>+	info = gnome_vfs_file_info_new ();
>+	result = gnome_vfs_get_file_info_uri (doc->priv->vfs_uri, info,
>+			GNOME_VFS_FILE_INFO_DEFAULT);
>+	if (result != GNOME_VFS_OK)
>+	{
>+		gnome_vfs_file_info_unref (info);
>+		return FALSE;
>+	}
>+	file_mtime = info->mtime;
>+	gnome_vfs_file_info_unref (info);
>+	return (file_mtime > doc->priv->mtime);
>+}

I don't think we should check for all files but only for local ones.
Checking remote files in a sync way as you do could block the GUI for a long time.

>+static gint
>+gedit_view_focus_in (GtkWidget      *widget,
>+		 GdkEventFocus *event)
>+{
>+	GtkTextView *text_view;
>+	GeditDocument *doc;
>+	GeditTab *tab;
>+	
>+	text_view = GTK_TEXT_VIEW (widget);
>+	
>+	doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (text_view));
>+	tab = gedit_tab_get_from_document (doc);
>+	gedit_tab_focused_in (tab);

You should never call a gedit_tab function from a view.
Please, connect to the focus_in signal of the view in the tab.

(In reply to comment #2)
> (From update of attachment 76049 [details] [review] [edit])
> Thanks for you patch.
> I think the feature you implemented is very interesting and it has been in our
> todo list for a long time.
> 
> I have not reviewed the patch in all details, but I already have a few comments
> for you.

happy you're interested in the feature :-)

> > 
> >+	gboolean     user_notified_that_file_changed_on_disk;
> >+
> 
> What is "user_notified_that_file_changed_on_disk" used for?

I knew I should add a comment :-)
it's telling us whether we told the user that the file he's working on is different than the one on disk.
it's like that: we detect the file has changed on disk. we tell the user. he has two options: refresh or keep. if he chooses "keep", the next time we focus in we'll detect again and ask him again! so we keep in mind that we told him that the file changed, and he's fine with it, no need to ask again.
when the user reverts, loads or save a file, we set this boolean back to "FALSE". Since the user re-synced the file to the disk, he assumes that the file is the one on disk, so we must notify him again if we detect that it's not the case.

> >+gboolean gedit_document_changed_on_disk (GeditDocument *doc)
> >+{
> >+	GnomeVFSFileInfo *info;
> >+	GnomeVFSResult result;
> >+	gint file_mtime;
> >+	g_return_val_if_fail (GEDIT_IS_DOCUMENT (doc), FALSE);
> >+
> >+	info = gnome_vfs_file_info_new ();
> >+	result = gnome_vfs_get_file_info_uri (doc->priv->vfs_uri, info,
> >+			GNOME_VFS_FILE_INFO_DEFAULT);
> >+	if (result != GNOME_VFS_OK)
> >+	{
> >+		gnome_vfs_file_info_unref (info);
> >+		return FALSE;
> >+	}
> >+	file_mtime = info->mtime;
> >+	gnome_vfs_file_info_unref (info);
> >+	return (file_mtime > doc->priv->mtime);
> >+}
> 
> I don't think we should check for all files but only for local ones.
> Checking remote files in a sync way as you do could block the GUI for a long
> time.

OK, I'll change that. didn't think about that problem. focus-in happens very often, so probably doing it even async would be overkill, so probably even long-term only local files seem to be the right path.

> >+static gint
> >+gedit_view_focus_in (GtkWidget      *widget,
> >+		 GdkEventFocus *event)
> >+{
> >+	GtkTextView *text_view;
> >+	GeditDocument *doc;
> >+	GeditTab *tab;
> >+	
> >+	text_view = GTK_TEXT_VIEW (widget);
> >+	
> >+	doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (text_view));
> >+	tab = gedit_tab_get_from_document (doc);
> >+	gedit_tab_focused_in (tab);
> 
> You should never call a gedit_tab function from a view.
> Please, connect to the focus_in signal of the view in the tab.

this is what I did at first but it didn't work... I don't understand why. 

in gedit-tab.c, in gedit_tab_init I added such a line:

	g_signal_connect_after(tab->priv->view,
			       "focus-in",
			       G_CALLBACK (view_focused_in),
			       tab);

just before the "realize" line. view_focused_in was called, but the "tab" parameter it was getting was crappy. tab->priv was NULL, and I got a different pointer for "tab" everytime view_focused_in was called... When I moved the method into gedit_view, it started working, so I assumed it's due to some event filters or who knows what, since I don't have the big view of the design.

actually, I remember that just changing the "realize" line in the code to "focus-in", and already view_realized was getting a crappy "tab" pointer. I'll try it again but I'm pretty sure it'll happen again.

Is this also the wrong location to do this? Do you expect this to work, or did you expect that it would happen what happened to me?

Created attachment 76105 [details] [review]
updated patch

Here is an updated patch which only considers local files, adds a comment on the variable on which you asked, but i didn't change the event handling in the view. As I mentionned, I tried adding those lines in gedit_tab_init:

	g_signal_connect_after(tab->priv->view,
			       "focus-in-event",
			       G_CALLBACK (gedit_tab_focused_in),
			       tab);

after doing that and unplugging the event triggererer in the view, I get a crash at gedit startup:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1227319104 (LWP 2179)]
gedit_tab_focused_in (tab=0x83d10c0) at gedit-tab.c:486
486             if ( (tab->priv->state == GEDIT_TAB_STATE_FILE_CHANGED)

 because the tab->priv is NULL. I don't understand why.

Created attachment 76108 [details] [review]
updated patch

i'm really sorry.. there was a leak in the previous patch (leaking uri in gedit_tab_focused_in). fixed in this updated patch.

> this is what I did at first but it didn't work... I don't understand why. 
>
> in gedit-tab.c, in gedit_tab_init I added such a line:
> 
>         g_signal_connect_after(tab->priv->view,
>                               "focus-in",
>                               G_CALLBACK (view_focused_in),
>                               tab);
>

I just quickly tried this and it works just fine here, provided that:
a) the signal name is "focus-in-event" (I assume this was just a typo)
b) the callback functions has the proper signature, for instance I used

static gboolean
view_focused (GtkWidget     *widget,
              GdkEventFocus *event,
              GeditTab      *tab)
{
	GtkTextView *text_view;
	GeditDocument *doc;

	text_view = GTK_TEXT_VIEW (widget);
	doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (text_view));

	g_assert (GEDIT_IS_TAB (tab));

	return FALSE;
}

Maybe you were using a callback with just two args and so the pointer you were trying to use as tab was actually the event pointer...


More comments on the patch later.

Comment on attachment 76108 [details] [review]
updated patch

Here are some more comments (some are just nitpicks, bear with me :)


>Index: gedit/gedit-document.c
>===================================================================
>RCS file: /cvs/gnome/gedit/gedit/gedit-document.c,v
>retrieving revision 1.135
>diff -u -r1.135 gedit-document.c
>--- gedit/gedit-document.c	24 Aug 2006 13:17:13 -0000	1.135
>+++ gedit/gedit-document.c	6 Nov 2006 20:04:43 -0000
>@@ -109,6 +109,11 @@
> 
> 	GTimeVal     time_of_last_save_or_load;
> 
>+	/* did we already notify the user that the file on disk
>+	 * is different than what is in gedit? if we notified
>+	 * the user, we won't notify again since user already knows. */
>+	gboolean     user_notified_that_file_changed_on_disk;
>+
> 	guint        search_flags;
> 	gchar       *search_text;
> 	gint	     num_of_lines_search_text;


I think this flag should be kept inside gedit-tab: though sometimes a bit not clean as they should be[1], gedit-document, gedit-view and gedit-tab are a MVC (model, view, controller) pattern and all the "intelligence" should be put in the tab state machine.

I think you should add a _gedit_document_check_external_modification function to gedit document (note that it's a private function, given the prepended underscore) and call it from the focus in callback, while keeping the flag if the check should be done or not in gedit-tab, resetting the flag in the callbacks for the "saved" and "loaded" signals.

[1] for instance we force just one view for each doc

>+gboolean
>+gedit_document_is_local_file (GeditDocument *doc)
>+{
>+	return gnome_vfs_uri_is_local (doc->priv->vfs_uri);
>+}
>+

drop this function, use gedit_utils_uri_has_file_scheme() (incidentally,
gnome_vfs_uri_is_local doesn't do what its name seems to imply :)

>+
>+gboolean gedit_document_changed_on_disk (GeditDocument *doc)
>+{

Coding style: return types should be on the prev lines so that function names are easily greppable with ^func_name

>+	GnomeVFSFileInfo *info;
>+	GnomeVFSResult result;
>+	gint file_mtime;

Coding style: always a blank line after the var declarations.

>+	g_return_val_if_fail (GEDIT_IS_DOCUMENT (doc), FALSE);
>+
>+	info = gnome_vfs_file_info_new ();
>+	result = gnome_vfs_get_file_info_uri (doc->priv->vfs_uri, info,
>+			GNOME_VFS_FILE_INFO_DEFAULT);

Coding style: args should be aligned

>+	if (result != GNOME_VFS_OK)
>+	{
>+		gnome_vfs_file_info_unref (info);
>+		return FALSE;
>+	}
>+	file_mtime = info->mtime;
>+	gnome_vfs_file_info_unref (info);
>+	return (file_mtime > doc->priv->mtime);

>===================================================================
>RCS file: /cvs/gnome/gedit/gedit/gedit-tab.c,v
>retrieving revision 1.17
>diff -u -r1.17 gedit-tab.c
>--- gedit/gedit-tab.c	25 Aug 2006 13:59:41 -0000	1.17
>+++ gedit/gedit-tab.c	6 Nov 2006 20:04:48 -0000
>@@ -450,6 +450,144 @@
> 				   (gpointer *)&tab->priv->message_area);
> }
> 
>+static void 
>+file_modified_on_disk_message_area_response (GeditMessageArea *message_area,
>+						gint              response_id,
>+						GeditTab         *tab)
>+{
>+	GeditView *view;
>+	GeditDocument *doc;
>+
>+	doc = gedit_tab_get_document (tab);
>+	g_return_if_fail (GEDIT_IS_DOCUMENT (doc));
>+
>+	set_message_area (tab, NULL);
>+	view = gedit_tab_get_view (tab);
>+	gedit_tab_set_state (tab, GEDIT_TAB_STATE_NORMAL);
>+
>+	if (response_id == GTK_RESPONSE_OK)
>+	{
>+		_gedit_tab_revert (tab);
>+	}
>+	else
>+	{
>+		gedit_document_set_user_notified_that_file_changed_on_disk (doc, TRUE);
>+	}
>+
>+	gtk_widget_grab_focus (GTK_WIDGET (view));
>+}
>+
>+void
>+gedit_tab_focused_in (GeditTab    *tab)
>+{
>+	GeditDocument *doc;
>+	gchar *uri;
>+
>+	if ( (tab->priv->state == GEDIT_TAB_STATE_FILE_CHANGED)
>+	 	|| (tab->priv->state == GEDIT_TAB_STATE_LOADING)
>+		|| (tab->priv->state == GEDIT_TAB_STATE_REVERTING) )
>+	{
>+		return;
>+	}

Coding style: we prefer the '||' on the prev line, no spaces between the ((


>+	doc = gedit_tab_get_document (tab);
>+	g_return_if_fail (GEDIT_IS_DOCUMENT (doc));
>+
>+	uri = gedit_document_get_uri (doc);
>+	if (uri == NULL)
>+	{	// file has no disk saving: nothing to do
>+		return;

Codyng style: do not use C++ comments, place comments on a different line from {


Apart from the above, my biggest concern is that we are adding a state to the tab state machine: this is the correct thing to do, but we should be carefull to take that into account in all the other state transitions, we should also write down from which states we can get into this new state and to which states we can go to. Substantially we should really write down some docs on the state machine before it gets out of hand :) (this is really our fault, not related to your patch)

you were right about me having my focus-in callback in gedit-tab with only two parameters. i copied the signature from the "realized" callback :-/ I miss a good, up-to-date GTK class/signals reference (apart from the source ;-) ).

(In reply to comment #7)
> I think you should add a _gedit_document_check_external_modification function
> to gedit document (note that it's a private function, given the prepended
> underscore) and call it from the focus in callback, while keeping the flag if
> the check should be done or not in gedit-tab, resetting the flag in the
> callbacks for the "saved" and "loaded" signals.

something is not clear to me here... the focus-in callback is now in gedit-tab... the function to check for external modifications is private to gedit-document... so how could the focus callback in gedit-tab could call it, since it's private to another class? Am I missing something obvious here?

> [1] for instance we force just one view for each doc

It's off topic... but I noticed that it's missing from the GUI. Is it by choice or because the code is not ready? If the code is not ready I would be interested to help, if there is some document/bug where to start (you can tell that to me in private mail if you wish to answer).

Just to mention that I'm completely available to finish this patch, I just don't understand your comment about the private function, as mentioned in my previous comment. just explain that to me and i'll submit an updated patch.

yeah, sorry. Your reply above was lost in the bugzilla noise and I didn't notice it.

> something is not clear to me here... the focus-in callback is now in
> gedit-tab... the function to check for external modifications is private to
> gedit-document... so how could the focus callback in gedit-tab could call it,
> since it's private to another class? Am I missing something obvious here?

Yes, I was a bit cryptic :-)

we (and many other projects, including gtk) have the convention that functions named _foo (underscore prepended to the name) are considered "private", that is while they can be called from another file inside the project, they should not be considered part of the API public to third party code (for instance, to plugins)

In libraries like gtk, this is not just a convention but is enforced by the linker, which is instructed to not make those functions available to the user of the library.

>> [1] for instance we force just one view for each doc
>
>It's off topic... but I noticed that it's missing from the GUI.

What do you mean here? What is missing from the gui?

ops, in the explanation above, I forgot the important part: even if conventionally private, the functions named _foo are not marked static to the file, otherwise as you say you would not be able to call them from another file.

>>> [1] for instance we force just one view for each doc
>>
>>It's off topic... but I noticed that it's missing from the GUI.
>
>What do you mean here? What is missing from the gui?

I meant that in gedit there is no GUI allowing several views for one document. I was wondering if this is a code limitation or if it was decided the feature is not needed. And if it's a code limitation, if there is a document with details about that somewhere, what needs to be fixed.

i'll upload an updated patch very soon.

Created attachment 77175 [details] [review]
updated patch

Here is the patch updated based on your comments.

> I meant that in gedit there is no GUI allowing several views for one document.
> I was wondering if this is a code limitation or if it was decided the feature
> is not needed. And if it's a code limitation, if there is a document with
> details about that somewhere, what needs to be fixed.


Well, the limitation goes fairly deep into GtkTextView/GtkTextBuffer itself, so we decided to avoid the N views <-> 1 buffer feature for now. removing all the issues is a big task, and maybe it's better to simply have many buffers and try to keep them in sync... dunno, it's something that needs to be properly thought out and desgned and I am not able myself to answer that easily.


The patch looks way better: I am away next week, but I'll try to review it as soon as possible since we definately want to get this committed. Thanks again for all your efforts!

Comment on attachment 77175 [details] [review]
updated patch

Hi, 
  here a few comments about your patch.

Please, use the -p option for diff while generating a patch.

>+gboolean
>+_gedit_document_check_external_modification(GeditDocument *doc)

Add a space before (.

>+
>+	info = gnome_vfs_file_info_new ();
>+	result = gnome_vfs_get_file_info_uri (doc->priv->vfs_uri, info,
>+						GNOME_VFS_FILE_INFO_DEFAULT);

You should problably add GNOME_VFS_FILE_INFO_FOLLOW_LINKS too.


> 
>+static void 
>+file_modified_on_disk_message_area_response (GeditMessageArea *message_area,
>+						gint              response_id,
>+						GeditTab         *tab)
>+{
>+	GeditView *view;
>+	GeditDocument *doc;
>+
>+	doc = gedit_tab_get_document (tab);
>+	g_return_if_fail (GEDIT_IS_DOCUMENT (doc));

doc variable is not used, please remove it.


>+static gboolean
>+view_focused_in (GtkWidget     *widget,
>+              GdkEventFocus *event,
>+              GeditTab    *tab)
>+{
>+	GeditDocument *doc;
>+	gchar *uri;
>+	
>+	g_assert (GEDIT_IS_TAB (tab));

We don't use g_assert to check parameters in gedit.
Please, use g_return_val_if_fail.

>+        
>+	if ((tab->priv->state == GEDIT_TAB_STATE_FILE_CHANGED) ||
>+		(tab->priv->state == GEDIT_TAB_STATE_LOADING) ||
>+		(tab->priv->state == GEDIT_TAB_STATE_REVERTING))
>+	{
>+		return FALSE;
>+	}

Are you sure these ones are the only invalid states.
It seems to me also SAVING, PRINTING, etc. are invalid.
See my thoughts at the end of this comment.

>+	
>+	doc = gedit_tab_get_document (tab);
>+	g_return_val_if_fail (GEDIT_IS_DOCUMENT (doc), FALSE);
>+
>+	uri = gedit_document_get_uri (doc);
>+	if (uri == NULL)
>+	{
>+		/* file has no disk saving: nothing to do */
>+		return FALSE;
>+	}
>+	gboolean local_document = gedit_utils_uri_has_file_scheme (uri);

We don't use C99 extensions in gedit.
Please, use C89 (all the variables declared at the beginning of the function).

>+	const gchar *primary_text = "The file changed on disk";
>+	const gchar *secondary_text = "Which version do you want to edit?";
>+
>+	GtkWidget *message_area;
>+	GtkWidget *hbox_content;
>+	GtkWidget *image;
>+	GtkWidget *vbox;
>+	gchar *primary_markup;
>+	gchar *secondary_markup;
>+	GtkWidget *primary_label;
>+	GtkWidget *secondary_label;

No C99 please.
I think you can split the view_focused_in in two functions.
I think the function related to message area could be put in a separate file.


I'm a bit worried about committing this patch.
This is not due to your work, that is indeed very good, but to the fact I'm reluctant to add new states to the tab state machine without having first documented it.
Every time we add a new state we risk to add new unwanted possible state transitions.
And I think we are losing the control of this state machine.

(In reply to comment #15)
> I think you can split the view_focused_in in two functions.
> I think the function related to message area could be put in a separate file.

I think it makes sense too. When coding this patch, I was surprised there's no method in gedit-message-area for a "generic" message area message.

how about in gedit-message-area something like
gedit_message_area_add_primary_and_secondary_text (gchar *primary_text, gchar *secondary_text);

and then the rest are mostly small function calls to gedit-message-area (gedit_message_area_add_button, gedit_message_area_set_contents) and that code (the rest) stays in gedit-tab?
 
> I'm a bit worried about committing this patch.
> This is not due to your work, that is indeed very good, but to the fact I'm
> reluctant to add new states to the tab state machine without having first
> documented it.
> Every time we add a new state we risk to add new unwanted possible state
> transitions.
> And I think we are losing the control of this state machine.

I understand. It's probably naive, but maybe a state machine is not needed? Maybe we just need several booleans, file_changed_message_area_displayed, print_preview_message_area_displayed, loading_message_area_displayed, and if it turns out that several are displayed at the same time, why not? Just make sure we're not displaying several times the SAME one at the same time. Maybe just the "print preview" one would have to be really special. If this was already discussed and it's a bad idea, don't spend too much time explaining me.

At least the state machine shouldn't be related to the notification areas. Such a decoupling will be needed if we want to allow plugins to show their own notification areas.

Created attachment 77751 [details] [review]
updated patch

took into account your comments, although for the state machine it's still not great. I did add that the notification that the file changed should also not appear during saving, which is obvious. For the rest, I didn't think it through much... I made a function in gedit-message-area to add primary & secondary text into the message area. it's useful I think but many times the caller wants to customize things (add things in one HBox or VBox) so it cannot be used everywhere. I still found another location where it's used the same way (I copied the code for the display of the message area from there) and changed it to call the new function instead of copying the code.

so, this patch still has at least the unresolved problem of the complexity of the state machine & not really inspected if all states work well with this new state.

*** Bug 75861 has been marked as a duplicate of this bug. ***

See also comments at bug #383293

Cheers
JP

just as a reminder, patch for bug #379778 also introduces a _gedit_document_check_externally_modified utility function.

NB: that latest patch add the new public function gedit_message_area_add_message_and_icon()

I suppose that function is callable by plugins (no "_") so its signature should be thought out before commiting it. maybe it should have an "_" at the beginning.

Any progress on this? It would be a super kewl new feature for 2.17/2.18

Unfortunately not much progress... and it's my fault for not having had enough time to give Emmanuel proper feedback :(


I took another look at the patch and it needs some more work:

The state transitions imho should be like this:
 - we can enter the NOTIFY_FILE_CHANGED_ON_DISK (which btw I like more as a name=)  state *only* from the NORMAL state so this means that if we are not in the normal state, view_focused should just return.
 - from the new state we have two cases: if the used presses "Cancel", we need to go back to the normal state which is where we come from (this is an important thing and seems to be missing from the patch). If the user presses "Reload" we automatically go in the REVERTING state. We need to enforce this transitions in a couple of ways: when exiting the NOTIFY_FILE_CHANGED_ON_DISK we should assert that we are either in NORMAL or in REVERTING and we also make sure that our new state is in the list of the ones allowed when entering the REVERTING state.

Adding a new state also needs some more modifications outside of gedit-tab.c: grepping for GEDIT_TAB_STATE shows that the tab state is used in various places and in particulat in gedit-window and in gedit-commands where we should make sure the user interface properly enforces what we said above about state transitions, for instance by making the save and print button insensitive when we are in the new state. 

Ok, I thought about this some more.

First of all I was wrong saying that the patch didn't reset the state to NORMAL when pressing Cancel. It does.

More importantely I think it's totally legit to press Save or Close while we are in the NOTIFY_EXTERNALLY_CHANGED state, since saving is necessary to overwrite the external change and closing simply drops the current changes. In other words I think that the save and close actions should remain sensitive during the notification and that we should accept NOTIFY_EXTERNALLY_CHANGED -> SAVING transitions.

In fact by looking at the code I think it should work just fine already. Even if there is a saving error, the current message area is cleared so we do not have problems of having two message areas.
However this behavior requires some further changes: pressing save while that notification is displayed should pass to document_save the flag that tells it to ignore external modifications or we would be prompted again.

What I said about gedit-window sensitivity handling was not 100% correct either: since there we check if the state is normal to set the actions sensitive or not, the patch as it is now already works. On the other hand if we implement what I propose above to allow Save while the notification is displayed we, need to make sure that that action is sensitive.


A couple more comments on the patch:

+	const gchar *primary_text = _("The file changed on disk");
+	const gchar *secondary_text = _("Which version do you want to edit?");

I don't like the proposed message very much, what about something like:

_("The document you are editing changed on disk"),
_("Do you want to drop your changes and reload the file?")

Actually the secondary message should be different depending on the fact that we have modified the buffer or not.


About the question if gedit_message_area_add_message_and_icon() should be added to the public message area api, I would avoid it for now: I suggest making it a static function ikn gedit-io-error-message-area.c and also put in that file a 
gedit_externally_modified_message_area_new() which creates the message area for our notification.

Created attachment 79644 [details] [review]
updated patch

This patch should fix your concerns as per your latest comments.
Sorry i didn't use "-p" parameter for the diff, it doesn't work with "svn diff".

Thanks for your patience on reviewing all those patches..

Created attachment 79793 [details] [review]
committed patch

Here is the patch I committed to svn. I tested it quite a bit and it works ok and since we really want to get this feature in 2.18 I committed, but we should really keep an eye on it since it's a delicate change.

With respect to Emmanuel's last patch it just contains some small modifications (slightly changes function names to suit my taste, introduce gedit_doc_is_local to avoid strdupping the uri each time as discussed with paolo, made the revert action sensitive).

Thanks a lot to Emmanuel for his awesome work and to put up with my slow feedback!

Emmanuel: may I ask you one last effort, since I have no time to do it right now, but I'd like to have a couple more enhancement to the patch:

 - pressing File->Revert should not warn about losing changes if we are displaying the notification

 - It would be cool if tabs with the notification displayed could have a warning ico in the tab as it happens with error messages (to be honest this is not really important since we can show the warning when the view is focused, but still...)

Created attachment 79881 [details] [review]
don't ask for confirmation when notification is displayed and reverting, fix two buglets

OK, maybe we can take those two functionalities one by one. when you take the first one i'll do the second one (if I manage).

This patch makes that revert does not ask for confirmation when the notification is displayed. it also fixes two minor defects from my patch/the SVN version: one warning of void function returning a value (return message_area), and I realized that you get a warning on the console when doing a revert when the notification is displayed. the "set_message_area (tab, null) which I added fixes it.

Otherwise I gave two parameters for revert_with_statusbar_flash (I know you won't like the name ;-) ), because when we call the method without asking for confirmation, we know we have the right tab. but when we call after confirmation, there's this comment about relying on modal window to have the right tab. so since the comment applies only to one case, i decided against getting the tab from the window inside 
revert_with_statusbar_flash. of course it's more a matter of taste.

Gah, the two buglets will teach to not do "ah, I can also do that" changes just before committing...

Thanks! Committed. And yes, I changed the function name :P

Created attachment 80126 [details] [review]
Here's the patch to change the tab icon when file changed on disk

The patch for the last feature you wanted. I didn't think it would be that easy :-)

I used the "Refresh" GTK icon. There could also be the "harddisk" stock icon, otherwise I can't see any other that would fit. I guess it would be one of those two or a custom icon.

the icon I meant was GTK_STOCK_DIALOG_WARNING. I fixed that and committed.


Finally closing this bug. Thanks again for your efforts and your patience Emmanuel!