Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
On 20/08/11 17:32, Lionel Elie Mamane wrote: I'd rather we fix*that* separate bug cleanly; after all, other exceptions may by thrown during a save. That commit is only in master (not libreoffice-3-4) anyway, so we have breathing space. it appears the exception here isn't really handled, I think the exception is caught ( and ignored ) previously precisely because nothing really can be done ( or at least the calling code lacks the brains to handle the exception ). Writer seems to handle it correctly... Also, the exception*is* caught (file sfx2/source/doc/objserv.cxx function SfxObjectShell::ExecFile_Impl line 680 in master), as the nice pop-up error message shows. Probably calc just mishandles an error return value of that function. That code throws away the message of the exception, though:-( It will probably be about two weeks before I can look into it. ah, ok, but please revert then if you cannot make it *behave* itself Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
On Thu, Aug 18, 2011 at 12:52:55PM +0100, Noel Power wrote: On 17/08/11 19:02, Lionel Elie Mamane wrote: On Wed, Aug 17, 2011 at 09:48:04AM +0100, Noel Power wrote: On 16/08/11 17:32, Lionel Elie Mamane wrote: I notice that calc and writer have the good sense of displaying an error message when an exception is raised during save. That's better than Base, that just silently aborts the save with no error message. However, calc seems to get very confused and disables (greys out) the save, close file, quit application, etc features :-| then it might be an idea to back out that commit, I'd rather we fix *that* separate bug cleanly; after all, other exceptions may by thrown during a save. That commit is only in master (not libreoffice-3-4) anyway, so we have breathing space. it appears the exception here isn't really handled, I think the exception is caught ( and ignored ) previously precisely because nothing really can be done ( or at least the calling code lacks the brains to handle the exception ). Writer seems to handle it correctly... Also, the exception *is* caught (file sfx2/source/doc/objserv.cxx function SfxObjectShell::ExecFile_Impl line 680 in master), as the nice pop-up error message shows. Probably calc just mishandles an error return value of that function. That code throws away the message of the exception, though :-( It will probably be about two weeks before I can look into it. On Thu, Aug 18, 2011 at 02:06:54PM +0100, Noel Power wrote: On 17/08/11 19:02, Lionel Elie Mamane wrote: Which leads me to another bug: If I remove the librarie's only dialog and save, I restart LO, I reopen the file again, the dialog is back. Fixed (by me) in master. maybe a better test would be + if ( ! ( xNameAccess-hasElements() || bInplaceStorage ) ) instead of + if ( ! ( xNameAccess-hasElements() || isModified() ) ) I'm not familiar with that code, if you say so, then make that change. -- Lionel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
Hi Lionel On 17/08/11 19:02, Lionel Elie Mamane wrote: On Wed, Aug 17, 2011 at 09:48:04AM +0100, Noel Power wrote: On 16/08/11 17:32, Lionel Elie Mamane wrote: I did now. Reproduced there, but someone (you?) pushed my patch so *now* one cannot reproduce it anymore. I also took the liberty of adding you to the CC list for the bug. I did not go as far as assigning it to you ;-) yes, I reproduced it too, the key point was you need to use the Tools | Macros | RunMacro, I was using Tools | Macros | Organise Macros | Libreoffice Basic. And yes, I did push the fix to master and the 3.4 branch, did you see the message ( in this thread ) with subject [PUSHED] - cherrypick to 3.4.2 Base fdo#40079 file / save (as) inoperant note: 3.4.2 is wrong and is corrected later in the mail spew. Which leads me to another bug: If I remove the librarie's only dialog and save, I restart LO, I reopen the file again, the dialog is back. probably worth opening another bug for that then, anyway, lets look at one thing at a time ( otherwise I will get depressed ) Fixed (by me) in master. http://cgit.freedesktop.org/libreoffice/core/commit/?id=caee685234e87a4509c9aec4f04813714bfaf93d ? not sure about this, I would like to know why this seems to work outside of base :/ I will try and see My testing shows that SfxDialogLibraryContainer::storeLibrariesToStorage aborts at the same place for a calc document (an exception is raised), but this does not abort the save. Thus, logically, in the conditions where this bug shows up, Calc looses the embedded images Fixed (by me) in master for calc and writer, commit 7f3a944146879d2f0e6ee3f69cf721eb14f18689. I notice that calc and writer have the good sense of displaying an error message when an exception is raised during save. That's better than Base, that just silently aborts the save with no error message. However, calc seems to get very confused and disables (greys out) the save, close file, quit application, etc features :-| then it might be an idea to back out that commit, it appears the exception here isn't really handled, I think the exception is caught ( and ignored ) previously precisely because nothing really can be done ( or at least the calling code lacks the brains to handle the exception ). This isn't unusual in libreoffice code, sad but true and especially true when saving. imo a document that has lost possibly some basic related changes is preferable to an inoperable document and of course the particular scenario triggering this should no longer be an issue right ? Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
On 17/08/11 19:02, Lionel Elie Mamane wrote: Which leads me to another bug: If I remove the librarie's only dialog and save, I restart LO, I reopen the file again, the dialog is back. probably worth opening another bug for that then, anyway, lets look at one thing at a time ( otherwise I will get depressed ) Fixed (by me) in master. maybe a better test would be + if ( ! ( xNameAccess-hasElements() || bInplaceStorage ) ) instead of + if ( ! ( xNameAccess-hasElements() || isModified() ) ) afaics in all cases where we have inplace storage we need to regenerate the basic related streams ( although I don't know where else other than base this is the case ) I just worry there might be some strange case where isModified might not be correct ( I have some vague recollection of it not being totally reliable under some circumstances... ) regards, Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
Hi Lionel On 16/08/11 17:32, Lionel Elie Mamane wrote: Two questions, a) did you try this on master No, not yet. no problem ( I did and it appeared to work as expected ) but I am pretty 'base' disabled so I fear I may have missed something in trying to recreate it You have to be very cautious what you do to reproduce it, it is fragile: for example, if you open the Basic IDE, you have lost, as this, according to my testing, leads to the Dialog library having been loaded at the point I patch. I think I did it right then, but.. ok worth trying again I suppose, also I rebuilt a fresh 3.4 that at least can now open a database file so I will try there again with the same instructions lso should not be empty; it should contain at least one dialog. Which leads me to another bug: If I remove the librarie's only dialog and save, I restart LO, I reopen the file again, the dialog is back. If the library has two dialogs and I delete one, it is gone after a save and reload. This seems to be Base-specific. probably worth opening another bug for that then, anyway, lets look at one thing at a time ( otherwise I will get depressed ) b) is this behaviour specific to base documents? Well, this bug is, but that is because the other apps have another bug that hides this one :-( My testing shows that SfxDialogLibraryContainer::storeLibrariesToStorage aborts at the same place for a calc document (an exception is raised), but this does not abort the save. Thus, logically, in the conditions where this bug shows up, Calc looses the embedded images what are the steps to reproduce this? ( hopefully it is reproducible ) this sounds bad in Dialogs. This is tested. And I guess also looses (the changes to?) anything that is saved *after* the Dialogs, if anything is (this is not tested). thanks again Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
On 16/08/11 17:32, Lionel Elie Mamane wrote: I disabled the export out any embedded image object code by adding a return; before it, and it does not loose the embedded images, even after restarting LO and re-opening the document. ( after refamiliarising myself with this code ) this behaviour is not at all expected. It seems that base documents are doing something strange ( well maybe better to say different ) here, in fact if you add, save and then remove some image controls ( with embedded images ) then the Picture streams are never discarded. With the other applications ( calc, writer etc. ) when you save you save you save to new storage and the storage content is regenerated from the document model so effectively the existing content is thrown away and recreated. In the case of basic of course some optimizations might occur like copying existing 'Basic' substorages to rather than writing to the target storage based on the in-memory libraries model. With that in mind and after doing a little testing I think your patch for loading the library in SfxDialogLibraryContainer::storeLibrariesToStorage is the simplest thing to do and should avoid any such problems in any other applications like the one you mentioned ( that I cannot reproduce ) in calc. So I will push this ( and good job too tracking it down ) The issue with the leaking 'Picture/xx.png etc. streams still remains, I guess I would need to understand what base actually does when saving with the existing content to see what we can do there, probably it is just a case of excluding the Pictures directory rather than blanket copying stuff. thanks, Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
Hello Lionel On 15/08/11 13:10, Lionel Elie Mamane wrote: When the Standard Basic Macro Library for the document is loaded (which happens implicitly as soon as a macro is executed), but not the Standard Dialog Library, the File / Save action (or the equivalent toolbar button) does nothing, without error message. I would like to have a look at this, be a little patient as I never looked too deeply into this part of the code you are patching ( although the embeddedimages part seems like something I added :-) ) Two questions, a) did you try this on master ( I did and it appeared to work as expected ) but I am pretty 'base' disabled so I fear I may have missed something in trying to recreate it b) is this behaviour specific to base documents? ( again I tested against master ) and it appears not and if so then I wonder what does base do differently from 'normal' documents and why sigh I also tried this on 3.4 but it appears even loading base documents causes some core ( but perhaps this is due to a stale build/install ) - I will build from clean later Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
probably I should have read further before replying ;-) ( sorry excuse for a mind is elsewhere at the moment ) On 15/08/11 13:10, Lionel Elie Mamane wrote: So probably what happened is that the // Can we simply copy the storage? optimisation / fast path was added and suddenly SfxLibraryContainer::storeLibrariesToStorage does not anymore load the libraries, as the rest of the code in SfxDialogLibraryContainer::storeLibrariesToStorage expected, which would suggest that my patch is the right fix. this sounds very plausible The remaining question is: if xSourceLibrariesStor-copyElementTo was called, does the code under: // we need to export out any embedded image object(s) // associated with any Dialogs. First, we need to actually gather any such urls // for each dialog in this container still need to be run? In other words, has copyElementsTo copied these embedded image objects, or not? not at all sure but it should be easy enough to test, in your dialog create an image control, right click to get the property sheet for the control, under the general tab scroll down to the Graphics property, click on the '...' to select a graphic, make sure you unselect the 'link' check box to ensure the image is embedded. After saving, the image should be stored in it's own document stream. After testing with your 'modification' scenario ( e.g. run macro, make change, re-save ) when you open the dialog the image should be still there ( best of course to check that after restarting libreoffice and re-opening the document ) Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
On Tue, Aug 16, 2011 at 09:41:43AM +0100, Noel Power wrote: On 15/08/11 13:10, Lionel Elie Mamane wrote: When the Standard Basic Macro Library for the document is loaded (which happens implicitly as soon as a macro is executed), but not the Standard Dialog Library, the File / Save action (or the equivalent toolbar button) does nothing, without error message. I would like to have a look at this, (...) Two questions, a) did you try this on master No, not yet. ( I did and it appeared to work as expected ) but I am pretty 'base' disabled so I fear I may have missed something in trying to recreate it You have to be very cautious what you do to reproduce it, it is fragile: for example, if you open the Basic IDE, you have lost, as this, according to my testing, leads to the Dialog library having been loaded at the point I patch. Either bind a macro to the open document event or run the macro from Tools / Macros / Run Macro. The Standard Dialog library also should not be empty; it should contain at least one dialog. Which leads me to another bug: If I remove the librarie's only dialog and save, I restart LO, I reopen the file again, the dialog is back. If the library has two dialogs and I delete one, it is gone after a save and reload. This seems to be Base-specific. b) is this behaviour specific to base documents? Well, this bug is, but that is because the other apps have another bug that hides this one :-( My testing shows that SfxDialogLibraryContainer::storeLibrariesToStorage aborts at the same place for a calc document (an exception is raised), but this does not abort the save. Thus, logically, in the conditions where this bug shows up, Calc looses the embedded images in Dialogs. This is tested. And I guess also looses (the changes to?) anything that is saved *after* the Dialogs, if anything is (this is not tested). I wonder what does base do differently from 'normal' documents and why It (correctly IMHO) aborts the save when the save raises an exception. On Tue, Aug 16, 2011 at 09:51:39AM +0100, Noel Power wrote: On 15/08/11 13:10, Lionel Elie Mamane wrote: The remaining question is: if xSourceLibrariesStor-copyElementTo was called, does the code under: // we need to export out any embedded image object(s) // associated with any Dialogs. First, we need to actually gather any such urls // for each dialog in this container still need to be run? In other words, has copyElementsTo copied these embedded image objects, or not? not at all sure but it should be easy enough to test, (...) After saving, the image should be stored in it's own document stream. After testing with your 'modification' scenario ( e.g. run macro, make change, re-save ) when you open the dialog the image should be still there ( best of course to check that after restarting libreoffice and re-opening the document ) I disabled the export out any embedded image object code by adding a return; before it, and it does not loose the embedded images, even after restarting LO and re-opening the document. I additionally disabled the // Can we simply copy the storage? case, and it seems to have corrupted the document: I cannot anymore edit the dialog :-) It just shows the Basic code or another dialog instead. -- Lionel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PATCH] Base fdo#40079 file / save (as) inoperant
When the Standard Basic Macro Library for the document is loaded
(which happens implicitly as soon as a macro is executed), but not the
Standard Dialog Library, the File / Save action (or the equivalent
toolbar button) does nothing, without error message.
The attached patch (against libreoffice-3.4 branch, thus old
bootstrap repository) explicitly loads the library before trying to
extract embedded images from it, which is the part that gave rise to
the exception that aborted the save operation. Tested to fix the bug
as I experience it.
There was a comment there saying the libraries would already be loaded
from previous code, but that is not the case. I thought maybe the
real bug is in previous code, the code that was supposed having
loaded the libraries, or maybe that code was genuinely changed and
really does not need to load the libraries anymore.
Looking into that direction, the comment probably refers to the call
to SfxLibraryContainer::storeLibrariesToStorage, which calls
SfxLibraryContainer::storeLibraries_Impl with bComplete=sal_True
whose code looks like:
(...)
if( !nLibsToSave )
return;
(...)
if ( bStorage )
{
// Don't write if only empty standard lib exists
if ( ( nNameCount == 1 ) ( aNames[0].equalsAsciiL(
RTL_CONSTASCII_STRINGPARAM( Standard ) ) ) )
{
(...)
if ( !xNameAccess-hasElements() )
return;
}
(...)
}
(...)
for( ; pName != pNamesEnd; ++pName )
{
if( pImplLib-implIsModified() || bComplete )
{
// Can we simply copy the storage?
if( !mbOldInfoFormat !pImplLib-implIsModified()
!mbOasis2OOoFormat xSourceLibrariesStor.is() )
{
MY TESTING SHOWS THAT THIS BRANCH IS TAKEN
try
{
xSourceLibrariesStor-copyElementTo( rLib.aName,
xTargetLibrariesStor, rLib.aName );
}
catch ... { ... }
}
else
{
(...)
// Maybe lib is not loaded?!
fprintf(stderr, about to maybe loadLibrary,
bComplete='%d'\n, bComplete);
if( bComplete )
loadLibrary( rLib.aName );
(...)
So probably what happened is that the // Can we simply copy the
storage? optimisation / fast path was added and suddenly
SfxLibraryContainer::storeLibrariesToStorage does not anymore load the
libraries, as the rest of the code in
SfxDialogLibraryContainer::storeLibrariesToStorage expected, which
would suggest that my patch is the right fix.
The remaining question is: if xSourceLibrariesStor-copyElementTo was
called, does the code under:
// we need to export out any embedded image object(s)
// associated with any Dialogs. First, we need to actually gather any such
urls
// for each dialog in this container
still need to be run? In other words, has copyElementsTo copied these
embedded image objects, or not?
I also attach a small patch that slightly simplifies the code without
changing its behaviour in any way.
--
Lionel
From ca35f4f5e586e6c4eed0f64a466bc471333b7b6c Mon Sep 17 00:00:00 2001
From: Lionel Elie Mamane lionel.mam...@gestman.lu
Date: Sun, 14 Aug 2011 17:00:54 +0200
Subject: [PATCH] fdo#40079: load Dialog library before trying to get embedded images
---
basic/source/uno/dlgcont.cxx |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/basic/source/uno/dlgcont.cxx b/basic/source/uno/dlgcont.cxx
index c8da150..158d9fe 100644
--- a/basic/source/uno/dlgcont.cxx
+++ b/basic/source/uno/dlgcont.cxx
@@ -292,7 +292,7 @@ void SfxDialogLibraryContainer::storeLibrariesToStorage( const uno::Reference e
Sequence OUString sLibraries = getElementNames();
for ( sal_Int32 i=0; i sLibraries.getLength(); ++i )
{
-// libraries will already be loaded from above
+loadLibrary( sLibraries[ i ] );
Reference XNameContainer xLib;
getByName( sLibraries[ i ] ) = xLib;
if ( xLib.is() )
--
1.7.2.5
From db46b409b2a8c921846ed0b6808950824c2144a8 Mon Sep 17 00:00:00 2001
From: Lionel Elie Mamane lionel.mam...@gestman.lu
Date: Mon, 15 Aug 2011 14:02:25 +0200
Subject: [PATCH] Janitorial: deduplicate code; no behaviour change
---
basic/source/uno/namecont.cxx |4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/basic/source/uno/namecont.cxx b/basic/source/uno/namecont.cxx
index 4882c6f..2251d12 100644
--- a/basic/source/uno/namecont.cxx
+++ b/basic/source/uno/namecont.cxx
@@ -1806,10 +1806,8 @@ void SfxLibraryContainer::storeLibraries_Impl( const uno::Reference embed::XSto
// open the source storage which might be used to copy yet-unmodified libraries
try
{
-if ( mxStorage-hasByName( maLibrariesDir ) )
+if ( mxStorage-hasByName( maLibrariesDir ) || bInplaceStorage )
xSourceLibrariesStor = mxStorage-openStorageElement( maLibrariesDir,
