Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


I did that at 824fe17511.

JMarc

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


I did that at 824fe17511.

JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 03:08 PM, Jean-Marc Lasgouttes wrote:

Le 18/07/12 17:43, Richard Heck a écrit :

Looks like you never backported this particular fix. If you decide to
do it, I'll backport my patch too.


Which thing didn't I backport?


This one, I think.


I guess that should go in, so I've done it.

Richard

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 03:08 PM, Jean-Marc Lasgouttes wrote:

Le 18/07/12 17:43, Richard Heck a écrit :

Looks like you never backported this particular fix. If you decide to
do it, I'll backport my patch too.


Which thing didn't I backport?


This one, I think.


I guess that should go in, so I've done it.

Richard

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch 
(no risk, IMO).


JMarc

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 19:21, Richard Heck a écrit :

OTOH, it looks like doing a reload in after save-as is a bit harsh. Is
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we
could do. It was definitely a big hammer, that commit.

The primary goal was to deal with the fact that children, parents, etc,
may no longer be where there are supposed to be. But the Buffer contains
all kinds of pointers (e.g., in the TOC, in the reference cache, etc)
that may point to children and the like. So I'm guessing there are other
issues.


Does it mean that LyX just crashes if I remove a file that is supposed 
to be here? How rude...


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 06:08 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

OTOH, it looks like doing a reload in after save-as is a bit harsh. Is
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we
could do. It was definitely a big hammer, that commit.

The primary goal was to deal with the fact that children, parents, etc,
may no longer be where there are supposed to be. But the Buffer contains
all kinds of pointers (e.g., in the TOC, in the reference cache, etc)
that may point to children and the like. So I'm guessing there are other
issues.

Does it mean that LyX just crashes if I remove a file that is supposed 
to be here? How rude...


I can't remember exactly what the crash was. But suppose you save the 
file to a new location, then you close what used to be a child buffer. 
The original file thinks that Buffer is still a child, and it didn't get 
notified of anything because it's not the parent any longer. Etc.


Richard

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch 
(no risk, IMO).



OK.

rh

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


Looks like you never backported this particular fix. If you decide to do 
it, I'll backport my patch too.


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 10:48 AM, Jean-Marc Lasgouttes wrote:

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


Looks like you never backported this particular fix. If you decide to 
do it, I'll backport my patch too.



Which thing didn't I backport?

rh

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/12 17:43, Richard Heck a écrit :

Looks like you never backported this particular fix. If you decide to
do it, I'll backport my patch too.


Which thing didn't I backport?


This one, I think.

JMarc

commit 9283cbdbcb3b33da28d2ecca6cd5665f39c7b388
Author: Richard Heck rgh...@comcast.net
Date:   Fri Jan 13 03:31:01 2012 +

Fix crash reported on list when renaming a child buffer and
then trying to compile.

There are really two problems here. One is that the renamed
buffer needs to be reloaded. All kinds of files, etc, may not
exist any more, if we've been saved to a new directory; our
children e.g. may not be in the right place. And, in this case,
we may no longer be a child of our old parent. Reloading will
fix all of that.

On reload, though, we need to clear our parent, since we may
not have one any more. It will get reset if need be.

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch 
(no risk, IMO).


JMarc

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 19:21, Richard Heck a écrit :

OTOH, it looks like doing a reload in after save-as is a bit harsh. Is
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we
could do. It was definitely a big hammer, that commit.

The primary goal was to deal with the fact that children, parents, etc,
may no longer be where there are supposed to be. But the Buffer contains
all kinds of pointers (e.g., in the TOC, in the reference cache, etc)
that may point to children and the like. So I'm guessing there are other
issues.


Does it mean that LyX just crashes if I remove a file that is supposed 
to be here? How rude...


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 06:08 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

OTOH, it looks like doing a reload in after save-as is a bit harsh. Is
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we
could do. It was definitely a big hammer, that commit.

The primary goal was to deal with the fact that children, parents, etc,
may no longer be where there are supposed to be. But the Buffer contains
all kinds of pointers (e.g., in the TOC, in the reference cache, etc)
that may point to children and the like. So I'm guessing there are other
issues.

Does it mean that LyX just crashes if I remove a file that is supposed 
to be here? How rude...


I can't remember exactly what the crash was. But suppose you save the 
file to a new location, then you close what used to be a child buffer. 
The original file thinks that Buffer is still a child, and it didn't get 
notified of anything because it's not the parent any longer. Etc.


Richard

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch 
(no risk, IMO).



OK.

rh

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


Looks like you never backported this particular fix. If you decide to do 
it, I'll backport my patch too.


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/18/2012 10:48 AM, Jean-Marc Lasgouttes wrote:

Le 18/07/2012 16:35, Richard Heck a écrit :

On 07/18/2012 05:46 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 19:21, Richard Heck a écrit :

This is why I propose to add a parameter to reload telling whether
undo should be discarded.


I just committed that change. This is an obvious candidate for branch
(no risk, IMO).


OK.


Looks like you never backported this particular fix. If you decide to 
do it, I'll backport my patch too.



Which thing didn't I backport?

rh

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 18/07/12 17:43, Richard Heck a écrit :

Looks like you never backported this particular fix. If you decide to
do it, I'll backport my patch too.


Which thing didn't I backport?


This one, I think.

JMarc

commit 9283cbdbcb3b33da28d2ecca6cd5665f39c7b388
Author: Richard Heck 
Date:   Fri Jan 13 03:31:01 2012 +

Fix crash reported on list when renaming a child buffer and
then trying to compile.

There are really two problems here. One is that the renamed
buffer needs to be reloaded. All kinds of files, etc, may not
exist any more, if we've been saved to a new directory; our
children e.g. may not be in the right place. And, in this case,
we may no longer be a child of our old parent. Reloading will
fix all of that.

On reload, though, we need to clear our parent, since we may
not have one any more. It will get reset if need be.

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 00:24, Pavel Sanda a écrit :

Richard Heck wrote:

This leads to the harmless message above, but more importantly, it means
that the undo stack is lost every time a file is saved-as. This looks like
a regression to me.

Richard, would it seem right to add a parameter to Buffer::reload asking
to preserve the undo stack in this case?


You mean to preserve undo stack for each reload of file or just for save-as
scenario? We also reload automatically after commit/checkout VCS operation which
can change read-only status.


This is why I propose to add a parameter to reload telling whether undo 
should be discarded.


OTOH, it looks like doing a reload in after save-as is a bit harsh. Is 
the goal only to sanitize IncludeInset?


I have been pondering for some time whether the updateBuffer mechanism 
could be extended with an enum telling what to update, in order to be 
able to run it for specific tasks. For example 
updateBuffer(EXTERNAL_FILES) could check the validity of all external 
files we hold. I had other applications in mind, but I can't remember 
them ATM.


JMarc

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 12/07/2012 18:14, Richard Heck a écrit :

Richard, would it seem right to add a parameter to Buffer::reload
asking to preserve the undo stack in this case?


You would probably know better than I do. Are there pointers on the undo
stack that will be invalidated once we reload?


The undo stack holds a pointer to the buffer (which is OK) and the undo 
elements hold StableIterators, which do not contain any meaningful 
pointers. So this should be safe enough IMO.


I will experiment.

JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/13/2012 03:47 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 00:24, Pavel Sanda a écrit :

Richard Heck wrote:
This leads to the harmless message above, but more importantly, it 
means
that the undo stack is lost every time a file is saved-as. This 
looks like

a regression to me.

Richard, would it seem right to add a parameter to Buffer::reload 
asking

to preserve the undo stack in this case?


You mean to preserve undo stack for each reload of file or just for 
save-as
scenario? We also reload automatically after commit/checkout VCS 
operation which

can change read-only status.


This is why I propose to add a parameter to reload telling whether 
undo should be discarded.


OTOH, it looks like doing a reload in after save-as is a bit harsh. Is 
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we 
could do. It was definitely a big hammer, that commit.


The primary goal was to deal with the fact that children, parents, etc, 
may no longer be where there are supposed to be. But the Buffer contains 
all kinds of pointers (e.g., in the TOC, in the reference cache, etc) 
that may point to children and the like. So I'm guessing there are other 
issues.


I have been pondering for some time whether the updateBuffer mechanism 
could be extended with an enum telling what to update, in order to be 
able to run it for specific tasks. For example 
updateBuffer(EXTERNAL_FILES) could check the validity of all external 
files we hold. I had other applications in mind, but I can't remember 
them ATM.


That's quite a good idea, I think. I'm sure we'd discover uses for this. 
I can well remember putting comments in the code that said things like, 
What we actually want here is such-and-such, and the way to get it is 
to update the Buffer


Richard

Re: Undo.cpp reports an error on trunk

[Pavel Sanda]

Jean-Marc Lasgouttes wrote:
 You mean to preserve undo stack for each reload of file or just for save-as
 scenario? We also reload automatically after commit/checkout VCS operation
 which can change read-only status.

 This is why I propose to add a parameter to reload telling whether undo
 should be discarded.

That looks fine.

 OTOH, it looks like doing a reload in after save-as is a bit harsh. Is the
 goal only to sanitize IncludeInset?

Paths to images also change, though don't know whether reload is needed.

Pavel

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 13/07/2012 00:24, Pavel Sanda a écrit :

Richard Heck wrote:

This leads to the harmless message above, but more importantly, it means
that the undo stack is lost every time a file is saved-as. This looks like
a regression to me.

Richard, would it seem right to add a parameter to Buffer::reload asking
to preserve the undo stack in this case?


You mean to preserve undo stack for each reload of file or just for save-as
scenario? We also reload automatically after commit/checkout VCS operation which
can change read-only status.


This is why I propose to add a parameter to reload telling whether undo 
should be discarded.


OTOH, it looks like doing a reload in after save-as is a bit harsh. Is 
the goal only to sanitize IncludeInset?


I have been pondering for some time whether the updateBuffer mechanism 
could be extended with an enum telling what to update, in order to be 
able to run it for specific tasks. For example 
updateBuffer(EXTERNAL_FILES) could check the validity of all external 
files we hold. I had other applications in mind, but I can't remember 
them ATM.


JMarc

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 12/07/2012 18:14, Richard Heck a écrit :

Richard, would it seem right to add a parameter to Buffer::reload
asking to preserve the undo stack in this case?


You would probably know better than I do. Are there pointers on the undo
stack that will be invalidated once we reload?


The undo stack holds a pointer to the buffer (which is OK) and the undo 
elements hold StableIterators, which do not contain any meaningful 
pointers. So this should be safe enough IMO.


I will experiment.

JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/13/2012 03:47 AM, Jean-Marc Lasgouttes wrote:

Le 13/07/2012 00:24, Pavel Sanda a écrit :

Richard Heck wrote:
This leads to the harmless message above, but more importantly, it 
means
that the undo stack is lost every time a file is saved-as. This 
looks like

a regression to me.

Richard, would it seem right to add a parameter to Buffer::reload 
asking

to preserve the undo stack in this case?


You mean to preserve undo stack for each reload of file or just for 
save-as
scenario? We also reload automatically after commit/checkout VCS 
operation which

can change read-only status.


This is why I propose to add a parameter to reload telling whether 
undo should be discarded.


OTOH, it looks like doing a reload in after save-as is a bit harsh. Is 
the goal only to sanitize IncludeInset?


It certainly wouldn't surprise me if there were something simpler we 
could do. It was definitely a big hammer, that commit.


The primary goal was to deal with the fact that children, parents, etc, 
may no longer be where there are supposed to be. But the Buffer contains 
all kinds of pointers (e.g., in the TOC, in the reference cache, etc) 
that may point to children and the like. So I'm guessing there are other 
issues.


I have been pondering for some time whether the updateBuffer mechanism 
could be extended with an enum telling what to update, in order to be 
able to run it for specific tasks. For example 
updateBuffer(EXTERNAL_FILES) could check the validity of all external 
files we hold. I had other applications in mind, but I can't remember 
them ATM.


That's quite a good idea, I think. I'm sure we'd discover uses for this. 
I can well remember putting comments in the code that said things like, 
"What we actually want here is such-and-such, and the way to get it is 
to update the Buffer"


Richard

Re: Undo.cpp reports an error on trunk

[Pavel Sanda]

Jean-Marc Lasgouttes wrote:
>> You mean to preserve undo stack for each reload of file or just for save-as
>> scenario? We also reload automatically after commit/checkout VCS operation
>> which can change read-only status.
>
> This is why I propose to add a parameter to reload telling whether undo
> should be discarded.

That looks fine.

> OTOH, it looks like doing a reload in after save-as is a bit harsh. Is the
> goal only to sanitize IncludeInset?

Paths to images also change, though don't know whether reload is needed.

Pavel

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 11/07/2012 23:05, Scott Kostyshak a écrit :

git bisect leads me here:
bbbc2b654175edbccb1ce54ae6dbb15f8a042360

which is related to this ticket:
http://www.lyx.org/trac/ticket/8159


I am not surprised that this change leads to this kind of problems 
(which are harmless AFAICS).


What happens is that, when a new file is saved, it has to be renamed 
and, since commit 9283cbdb from Richard, this implies a Buffer::reload, 
which in turn clears the undo stack.


This leads to the harmless message above, but more importantly, it means 
that the undo stack is lost every time a file is saved-as. This looks 
like a regression to me.


Richard, would it seem right to add a parameter to Buffer::reload asking 
to preserve the undo stack in this case?


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/12/2012 07:31 AM, Jean-Marc Lasgouttes wrote:

Le 11/07/2012 23:05, Scott Kostyshak a écrit :

git bisect leads me here:
bbbc2b654175edbccb1ce54ae6dbb15f8a042360

which is related to this ticket:
http://www.lyx.org/trac/ticket/8159


I am not surprised that this change leads to this kind of problems 
(which are harmless AFAICS).


What happens is that, when a new file is saved, it has to be renamed 
and, since commit 9283cbdb from Richard, this implies a 
Buffer::reload, which in turn clears the undo stack.


This leads to the harmless message above, but more importantly, it 
means that the undo stack is lost every time a file is saved-as. This 
looks like a regression to me.


Richard, would it seem right to add a parameter to Buffer::reload 
asking to preserve the undo stack in this case?


You would probably know better than I do. Are there pointers on the undo 
stack that will be invalidated once we reload?


I'd have done something less drastic than reload the file there if I 
could have thought of anything. But there are so many parameters that 
depend upon the file's location that it didn't seem anything less would do.


Richard

Re: Undo.cpp reports an error on trunk

[Pavel Sanda]

Richard Heck wrote:
 This leads to the harmless message above, but more importantly, it means 
 that the undo stack is lost every time a file is saved-as. This looks like 
 a regression to me.

 Richard, would it seem right to add a parameter to Buffer::reload asking 
 to preserve the undo stack in this case?

You mean to preserve undo stack for each reload of file or just for save-as
scenario? We also reload automatically after commit/checkout VCS operation which
can change read-only status.
Pavel

Re: Undo.cpp reports an error on trunk

[Jean-Marc Lasgouttes]

Le 11/07/2012 23:05, Scott Kostyshak a écrit :

git bisect leads me here:
bbbc2b654175edbccb1ce54ae6dbb15f8a042360

which is related to this ticket:
http://www.lyx.org/trac/ticket/8159


I am not surprised that this change leads to this kind of problems 
(which are harmless AFAICS).


What happens is that, when a new file is saved, it has to be renamed 
and, since commit 9283cbdb from Richard, this implies a Buffer::reload, 
which in turn clears the undo stack.


This leads to the harmless message above, but more importantly, it means 
that the undo stack is lost every time a file is saved-as. This looks 
like a regression to me.


Richard, would it seem right to add a parameter to Buffer::reload asking 
to preserve the undo stack in this case?


JMarc

Re: Undo.cpp reports an error on trunk

[Richard Heck]

On 07/12/2012 07:31 AM, Jean-Marc Lasgouttes wrote:

Le 11/07/2012 23:05, Scott Kostyshak a écrit :

git bisect leads me here:
bbbc2b654175edbccb1ce54ae6dbb15f8a042360

which is related to this ticket:
http://www.lyx.org/trac/ticket/8159


I am not surprised that this change leads to this kind of problems 
(which are harmless AFAICS).


What happens is that, when a new file is saved, it has to be renamed 
and, since commit 9283cbdb from Richard, this implies a 
Buffer::reload, which in turn clears the undo stack.


This leads to the harmless message above, but more importantly, it 
means that the undo stack is lost every time a file is saved-as. This 
looks like a regression to me.


Richard, would it seem right to add a parameter to Buffer::reload 
asking to preserve the undo stack in this case?


You would probably know better than I do. Are there pointers on the undo 
stack that will be invalidated once we reload?


I'd have done something less drastic than reload the file there if I 
could have thought of anything. But there are so many parameters that 
depend upon the file's location that it didn't seem anything less would do.


Richard

Re: Undo.cpp reports an error on trunk

[Pavel Sanda]

Richard Heck wrote:
>> This leads to the harmless message above, but more importantly, it means 
>> that the undo stack is lost every time a file is saved-as. This looks like 
>> a regression to me.
>>
>> Richard, would it seem right to add a parameter to Buffer::reload asking 
>> to preserve the undo stack in this case?

You mean to preserve undo stack for each reload of file or just for save-as
scenario? We also reload automatically after commit/checkout VCS operation which
can change read-only status.
Pavel