Apache OpenOffice (AOO) Bugzilla – Issue 29152
webDAV locking needs to be implemented
Last modified: 2016-08-12 05:45:15 UTC
Even though the webdav functionality as provided is very useful, having the ability to use webdav locking is really necessary - especially when sharing the editing permissions with other distributed authors. As an example webdav authoring with plone (plone.org) or the oocommunity site http://oosurvey.gratismania.ro:8080/oocommunity where we would find the locking feature very useful. CPH
Hi Kai, can you please have a look into this? Thanks, Matthias
Some general comments. This is not just about extending the WebDAV-UCP. Actually this task is a subtask for a bigger, new and more general Office feature: support for collaborative work on documents. This must not be restricted to WebDAV, although it fits perfectly here. Implementing this new Office feature is certainly much more work than extending the UCP.
Any movement on this ? Maybe with some more description the community could help.
Andreas, please take care.
accepted
In business context the collaborative ability is a natural extension to traditional standalone desktop office work (word processing and so on). Today the situation is as simple as following : to share documents, in an heteregoneous environment Linux/Mac/Windows with minimal locking there are 2 solutions : - using smb : it works well with Msoffice AND Openoffice, but it is not what I call a standard, despite the great work of samba team. For remote roaming users (out of the office) accessing a smb server trough various clients is a secure manner is not so simple as smb "needs" many ip ports to be opened. - using dav : supported by apache server, we can set up a common business set of "working group folders", each restricted to authorized user for viewing and modifying documents. This is standard, works gracefully with firewalls, easy to secure by tunneling http in SSH tunnel protected by use of ssh2 keys. Tunneling smb in ssh is possible but only with windows XP and not 9x/NT because XP supports virtual network cards.And I do not want too promote particularly XP in the company. Unfortunately today the support of dav locking is best in...Ms Office. Shortly there are 3 ways to use webdav in windows (2 in 9x/Me/NT and one more in 2k/XP) :
...FOLLOW ON of my previous comments, since I typed "enter" too soon... ..1/ by entering a "folder" url in internet explorer and open it as "Web Folder" : well funny but not so useful ..2/ by creating a "permanent" network shortcut to a dav folder : then, with ms office versions from mso2000 to 2003 you can DOUBLE CLICK on any mso doc and it opens gracefully alerting about locking if another person has already opened the doc simply by dblclicking on it (no need to put a lock explicitely with a property sheet as is Novell NetDrive for example or the too simple "Dossiers Web libres". This works very well. and probably the most reliable solutions in Windows world. ..3/ refinement on 2k/XP : using -transparently- the Webclient service one can "MOUNT", I should say "MAP", a dav folder url to a local virtual drive (as we commonly do with SMB shares): interesting thing, giving the opportunity to DBL CLICK on ANY type of documents in remote locations (eg oo docs) BUT WITH ALMOST NO LOCKING SUPPORT: even for msoffice docs the webclient locking support between different versions of msoffice DOES NOT WORK (eg : no locking support between off2k and off2003, whereas with solutions 2/ it was ok...) I do not want of course to talk about the "sharepoint service" which is only a non standard MS "moneyware" MS wants to impose as a replacement for the "good" dav mechanism. Well finally it is a strategic business issue that : 1/ oo supports oo interlocking access to a common dav folder. 2/ AND, if possible, a shell extension in windows world and maybe in other platforms, allowing the DBL CLICK on a dav locations mounted locally as "network shortcut" (a MSwin particularity) and/or as a drive. 3/ for "bad" interoperabilty reasons, be able to manage locking ON "MSoffice" docs CONCURRENTLY with other machines accessing the same docs not with oo but with MSOffice: ie transparent concurrency on msoffice docs WITH either other oo clients OR MS OFFICE clients. Hope this will help.
Anything new in this issue?
ABI->KSO: As discussed ...
.
Any news?
KSO->TKR: Please take care of this issue.
I've implemented the locking into the WebDAV UCP in ooo-build, you can see the patch here: http://svn.gnome.org/viewvc/ooo-build/trunk/patches/src680/webdav-locking.diff So far it needs few more patches from ooo-build (gnome-vfs related) to work properly; I'll attach a bit cleaned up version soon for review.
We can change the type to PATCH even now I think ;-)
kendy: Thanks alot. As this patch is pretty large, may I kindly ask you to explain at a conceptual level what this patch does. How is it exposed at application level? Some UI to check "lock it" upon opening a document or is it implicit? ... After a very quick lock, some first notes from me: - don't use comphelper::getProcessFactory(). This is bad style. Keep, pass and use always the process service factory that was used to create the UNO object instance. In theory, there can be more than one service factories in one OOo process - how is the XStream stuff related to webdav locking? Or is this another (IMO neat) feature? If so, please supply a separate patch to keep things more easy and straigth
kso: Thank you for looking at this! Yes, I plan to separate the patch a bit so that it's easier to understand. But at least few answers now ;-) Generally, most of what the patch does is that it adds support for XActiveDataStreamer sinks, because that is the only way [I found ;-)] to be able to have the document open for all the time the user edits it - which is needed to be able to unlock the file after closing. The rest of the changes more or less depends on it - eg. the extension of the NeonInputStream to XStream (XActiveDataStreamer needs to get a full XStream). Another thing that needs comments is the "SupportsActiveStreaming" property. I've implemented it in another patch in ooo-build (http://svn.gnome.org/viewvc/ooo-build/trunk/patches/src680/sfx2-gnome-vfs-lock.diff, http://svn.gnome.org/viewvc/ooo-build/trunk/patches/src680/sfx2-gnome-vfs-lock-fix-xls.diff - I hope I did not forget other cases), because the code in sfx2 treats local files differently from others (even though everything should be provided by UCB), and for the XActiveDataStreamer to work in all the cases, this had to be extended. To answer your questions: Yes, the locking is implicit (similarly to the locking of files), if the document is locked on the server, it is opened read-only locally. The check for being locked needs a bit of an improvement, the response of the DAV server has to be checked if it is really 'document locked' error. comphelper::getProcessServiceFactory() is a crude hack, and I'm aware of that, but unfortunately it is impossible to avoid without even more growing the size of the patch :-( The reason is that ucbhelper::IsDocument() creates no interaction environment, but DAVAuthListener_Impl::authenticate() needs that to be able to provide the authentication dialog or information. When I added the creation of the interaction environment directly to ucbhelper::IsDocument(), I started getting error dialog boxes on OOo startup (harmless, but irritating). The other occurrence is in the stream destructor - again no idea how better to pass the service factory there :-( Already in the constructor? - ideas appreciated. XStream is needed for the XActiveDataStreamer as described above, the write when/if the NeonInputStream is dirty is still a TODO (but it seems not to be a problem for now). Please correct me if my assumptions above are wrong :-) Thank you!
cc tbe
set target 3.x
So - I'd like to create a CWS for this ;-) It will also contain a gnome-vfs locking work we did in the gvfs UCP. Please, could you help me getting it polished? I'm afraid there are going to be some controversial parts; in many cases, the there are hardcoded checks for 'file' protocols which I had to change to for 'SupportsActiveStreaming' property, etc. Things like the getProcessServiceFactory() could be hopefully cleaned up in a follow-up CWS (if really necessary). What do you think?
kendy: Sounds good. Go ahead and let me know if you need any help. If we don't change the UI. We could set the target to OOo 3.0
Committed to CWS webdavandgvfslocking1, now I'll try if it's warning-free etc. [it's a verbatim commit of the patches from ooo-build]. Setting target to 3.0, the CWS contains no GUI changes. Please have a look - I'll be happy to answer any questions :-)
kendy: Some comments after quickly reading the diff for CWS webdavandgvfslocking1. - Seems, that you put your fix for #i84137# "Remove gnome-vfs from startup procedure" into this CWS (without adding the task to the CWS, btw.) As you know I cannot accept this fix, because it violates UCB design constraint not to use solar libs. OTOH, I cannot see how this changes are related to gnome-vfs file locking. Please remove the changes for #i84137# from this CWS. Let's deal with this problem another time. - remove comphelper::getProcessServiceFactory() calls. Use the service factory/component context that was used to create the UCP. - remove the hack to create your own Interaction Handler in case none is available (DAVResourceAccess::createCommandEnvironment()). The code calling the UCB must be changed to provide the right Interaction Handler, if one is needed. - remove the hard cast from XStream to NeonInputstrem you're doing in webdavcontent.cxx in order to be able to call NeonInputStream::SetLock(). This is a hack, isn't it. - I don't really like that locking is only done in case an XActiveDataStreamer is supplied with the "open" command. What about XOutputStream, XActiveDataSink "open"-variants? - Is the implementation of property "IsReadOnly" and "SupportactiveStreaming" is not acceptable as it is now => Remove the "if ( rProperties.getLength() == 1 )" stuff. What is this good for? - "IsReadOnly" implementation seems to be a hack, too. Why is the value only avail in case m_bForceReadOnly is true? The property should always be available. The correct value should be obtained by inspecting the webdav-locking state of the resource. More comments to follow after I found the time to do a detailed analysis of your changes ;-)
kso: I knew this was not going to be easy ;-) So: > #i84137# "Remove gnome-vfs from startup procedure" Sorry, this was a mistake during commit (forgot that our GnomeVFS section contains a bit more than just the locking patches), reverted in the CWS. > remove comphelper::getProcessServiceFactory() calls I thought that the agreement in the above was to clean up this in a follow-up CWS. If you really want to have it cleaned up now, the patch will get even more complex and harder to integrate :-( But of course, I can do it. > remove the hack to create your own Interaction Handle This opens another ugly can of worms :-( Of course I tried it first, but then even more parts of OOo had to be touched - eg. you get tons of dialogs like 'File blahbleh.xcu is not available' during startup if there was always an interaction handler available, etc. > remove the hard cast from XStream to NeonInputstrem Removed on the expense of adding 2 more parameters to GET() - hopefully better now. > XOutputStream, XActiveDataSink "open"-variants I did not find a way do that, unfortunately :-( The stream in these case does not stay open, it is just read, and thrown away. You get no information when the document is closed, so you have no way to unlock the file again. Of course, the stream is locked while performing save (using the other UCB access methods), but that was there already before my changes, and it co-operates (uses the same lock store). > the implementation of property "IsReadOnly" and "SupportactiveStreaming" is not acceptable as it is now Oh - such stuff was in the WebDAV UCP itself ages before when the gnome-vfs locking was introduced (see eg. checking for 'Title' property in OOG680_m*), and I just mimicked that at that time (and I did not update it after it vanished). Fixed in the CWS now. > "IsReadOnly" implementation seems to be a hack Well - I need to return 'true' there when we are locked - so that the UI can react accordingly, and has nothing to do with the fact if the resource is really read-only on the remote end, or not. If we had better way to present the user that it is locked, I would use that one ;-) The fact that 'IsReadOnly' property is not implemented (got from the remote end) is not really my problem here ;-) - but of course can be addressed in a follow-up CWS if necessary. > More comments to follow after I found the time to do a detailed analysis of > your changes ;-) Oh - please don't make it too bad, or I'll run away screaming ;-) BTW, why are there the abstract classes (DAV*.hxx)? Do you have a different implementation (not using neon) in StarOffice? Or is it just remnant of some other implementation, and would deserve cleanup as well? Thank you a lot for your feedback!
kendy: >> #i84137# "Remove gnome-vfs from startup procedure" > >Sorry, this was a mistake during commit (forgot that our GnomeVFS section >contains a bit more than just the locking patches), reverted in the CWS. Okay. Thanks. >> remove comphelper::getProcessServiceFactory() calls > >I thought that the agreement in the above was to clean up this in a follow-up >CWS. If you really want to have it cleaned up now, the patch will get even >more complex and harder to integrate :-( But of course, I can do it. I suggest to change it in this CWS. I'm afraid, it would stay that bad forever, otherwise. ;-) >> remove the hack to create your own Interaction Handler > >This opens another ugly can of worms :-( Of course I tried it first, but then >even more parts of OOo had to be touched - eg. you get tons of dialogs >like 'File blahbleh.xcu is not available' during startup if there was always >an interaction handler available, etc. We have to find a solution for this problem in this CWS. You have to find the places where to supply an IH and where not. Always supplying an IH is obviously not working. >> remove the hard cast from XStream to NeonInputstrem > >Removed on the expense of adding 2 more parameters to GET() - hopefully better >now. Thanks. Sure. >> XOutputStream, XActiveDataSink "open"-variants > >I did not find a way do that, unfortunately :-( The stream in these case does >not stay open, it is just read, and thrown away. You get no information when >the document is closed, so you have no way to unlock the file again. I thought about this some time ago. IMO, we need a "close" command for the UCB that is executed by the application framework whenever a document gets closed. >Of course, the stream is locked while performing save (using the other UCB >access methods), but that was there already before my changes, and it >co-operates (uses the same lock store). >> the implementation of property "IsReadOnly" and "SupportactiveStreaming" is >>not acceptable as it is now > >Fixed in the CWS now. Cool. >> "IsReadOnly" implementation seems to be a hack > >Well - I need to return 'true' there when we are locked - so that the UI can >react accordingly, and has nothing to do with the fact if the resource is >really read-only on the remote end, or not. If we had better way to present >the user that it is locked, I would use that one ;-) The property IsReadOnly has a predefined semantics. If you need something similar, but nt exactly the same semantics, you have to introduce a different property name. How about "IsLocked"? BTW, you should talk to tbe (on CC here) about a better way to present the user that a document is locked. IIRC, he's working on something like this right now. Would be great if we could join efforts here. >The fact that 'IsReadOnly' property is not implemented (got from the remote >end) is not really my problem here ;-) - but of course can be addressed in a >follow-up CWS if necessary. IIRC, it's actually your problem, because you introduced support for this property, but sort of "incomplete". But I'd go for the "IsLocked" property approach... >> More comments to follow after I found the time to do a detailed analysis of >> your changes ;-) > >Oh - please don't make it too bad, or I'll run away screaming ;-) BTW, why Please don't get me wrong. Your work is highl appreciated. as you said, your patch is kind of incomplete and needs some polishing. >are there the abstract classes (DAV*.hxx)? Do you have a different >implementation (not using neon) in StarOffice? Or is it just remnant of some >other implementation, and would deserve cleanup as well? No, we don't have a different implementation for StarOffice. The abstract classes are actually of no use. Feel free to remove them, if you like.
> I suggest to change it in this CWS. I'm afraid, it would stay that bad > forever, otherwise. ;-) Even if I promise? ;-) Well - I'll have a look. > We have to find a solution for this problem in this CWS. You have to find > the places where to supply an IH and where not. Always supplying an IH is > obviously not working. Well, I don't know how to solve it then :-( What I had was the following IIRC: #--- comphelper/source/misc/mediadescriptor.cxx #+++ comphelper/source/misc/mediadescriptor.cxx #@@ -754,9 +754,12 @@ sal_Bool MediaDescriptor::impl_openStreamWithURL(const ::rtl::OUString& sURL) # throw(::com::sun::star::uno::RuntimeException) # { # // prepare the environment #+ css::uno::Reference< css::lang::XMultiServiceFactory > xFactory( ::comphelper::getProcessServiceFactory(), uno::UNO_QUERY ); # css::uno::Reference< css::task::XInteractionHandler > xOrgInteraction = getUnpackedValueOrDefault( # MediaDescriptor::PROP_INTERACTIONHANDLER(), #- css::uno::Reference< css::task::XInteractionHandler >()); #+ css::uno::Reference< css::task::XInteractionHandler > ( #+ xFactory->createInstance( rtl::OUString( RTL_CONSTASCII_USTRINGPARAM("com.sun.star.uui.InteractionHandler") ) ), css::uno::UNO_QUERY ) #+ ); # # StillReadWriteInteraction* pInteraction = new StillReadWriteInteraction(xOrgInteraction); # css::uno::Reference< css::task::XInteractionHandler > xInteraction(static_cast< css::task::XInteractionHandler* >(pInteraction), css::uno::UNO_QUERY); d Ideas appreciated - the authentication dialog is needed there - think of a user that does not want OOo to remember the password - then he needs to be presented with the dialog every time (and that's not possible without the IH from what I saw, but maybe I did not see everything?). > I thought about this some time ago. IMO, we need a "close" command for the > UCB that is executed by the application framework whenever a document gets > closed. OK - but let's agree that this is not going to be part of this CWS - is that OK for you? 'file' UCP does not have this either. > The property IsReadOnly has a predefined semantics. If you need something > similar, but nt exactly the same semantics, you have to introduce a > different property name. How about "IsLocked"? Well - this way I would need to touch even the 'file' UCP to provide the 'IsLocked' property as well instead of returning 'IsReadOnly', and other parts of OOo to make this right; searching for all the occurrences, extend it to handle it correctly, ... Sorry, I refuse to do it in this CWS ;-) - I hope you understand. > BTW, you should talk to tbe (on CC here) about a better way to present the > user that a document is locked. tbe: Or maybe you already have a solution for the above already ('IsLocked' or something)? > IIRC, it's actually your problem, because you introduced support for this > property, but sort of "incomplete". But I'd go for the "IsLocked" property > approach... Well, I'll have a look if neon can return it somehow. > No, we don't have a different implementation for StarOffice. The abstract > classes are actually of no use. Feel free to remove them, if you like. OK, I'll do it in the follow-up CWS.
kendy: what's the problem with the InteractionHandler here? Why do you think that you need one? Sorry in case that question looks dumb, but I couldn't find any reference in the previous comments why this might be necessary. Is it because you want to show a dialog even if no Interaction handler is provided? This is bad because it will break all applications that want to run without user interaction. In this case it would be much better to proceed silently with an unlocked and read-only document. We do the same for documents in the file system we can't lock. IIRC the way we are doing this is to catch the exception that tells about the lock failure) and restart the "open" command, now without different parameters.
cc mav
kendy: Without knowing any details, I think that a 'IsLocked' property is the right approach. In CWS calcshare2, which will be integrated hopefully soon, Mikhail Voitenko (MAV) improved file locking for documents on local file systems. Part of this feature is, that an additional lock file is written. MAV also made some changes to the user interaction. If a document is locked, an interaction handler opens a dialog, which allows the user to open the document in readonly mode or create a copy of the document. I'm sure, MAV can give some more details here. I will set him on cc.
The new locking mechanics in calcshare2 cws is just a workaround for heterogeneous network file systems problem. Unfortunately, in some cases the system file locking does not work there as expected. The necessary for the workaround lock file is used to store the information about the user, this information can be used in the new com.sun.star.document.LockedDocumentRequest request later. The current usage of "IsReadOnly" property is no hack from my point of view, if we agree that ReadOnly mode of documents is not hack, that is actually questionable for me. Although I can not say anything abouth implemenation. In few words the approach in MediaDescriptor is as follows: - the file URLs can be opened for editing and locked, so if it is not possible to do it the file is locked or readonly or does not exist and can not be created... - all other URLs do not allow file locking, so the file is always opened ReadOnly; if the "IsReadOnly" property not set it will be allowed to edit the document So the new WebDAV locking could do something like following to let it work at least as the file locking before without introducing of the new property for all the contents ( and as I understood this is the wish for the first step ) : - the file URL is handled as before - the WebDAV content should provide information whether it is possible to get readwrite stream from the content in general ( in other words the fact that the server supports the feature, sorry I do not know whether it always so ), if yes the impossibility to open the stream means exactly the same as in case of file-URL - all other cases ( including WebDAV that supports no locking ) are handled as before based on IsReadOnly property and input stream By the way, generally the change in calcshare2 should not affect the WebDAV locking mechanics, although after the cws calcshare2 is integrated the SfxMedium part should be adjusted to let new WebDAV locking be used. It is a quite small change, but it is already too late to integrate it in calcshare2 since QA round is already in progress.
Sorry in the block "The current usage of "IsReadOnly" property is no hack from my point of view, if we agree that ReadOnly mode of documents is not hack, that is actually questionable for me. Although I can not say anything abouth implemenation." please read "The current usage of "IsReadOnly" property is no hack from my point of view, if we agree that ReadOnly mode of documents is not hack, that is actually questionable for me. Although I can not say anything abouth implemenation of the "IsReadOnly" property in UCB."
mba: The code in question is this: int DAVAuthListener_Impl::authenticate( const ::rtl::OUString & inRealm, const ::rtl::OUString & inHostName, ::rtl::OUString & inoutUserName, ::rtl::OUString & outPassWord ) { uno::Reference< task::XInteractionHandler > xIH; if ( m_xEnv.is() ) xIH = m_xEnv->getInteractionHandler(); else xIH = DAVResourceAccess::createCommandEnvironment()->getInteractionHandler(); The 'else' part is the hack we are talking about... I'd really love to get rid of that, but without that part, the authentication just fails exactly in the cases when there is no interaction handler ( xIH.is() returns false). In the old code, there is a hack that caches old password; but this works only if the _first_ call is done with existing interaction handler, and the consequent ones use this cached password. But this is not the case after the changes, with XActiveDataStreamer, the calls _without_ the interaction handler are the first ones. And this is regardless of the locking - it is just the way it is accessed, IIRC it is propagated from some exist() method of something - can find the details on request. But maybe I'm really doing something wrong myself ;-) - ideas still appreciated.
mav: For me, it would be perfectly OK to create a follow-up CWS for WebDAV locking after 'calcshare2' is integrated to change it so that it does it the same way as 'file' UCB (presenting the dialog saying 'this is locked' if the interaction handler is available, etc.) Please - would that be OK for you? (Until calcshare2 is not in, show it as read-only if locked; when calcshare2 is integrated, do the changes in the WebDAV UCB?)
kendy: I hope that we have the common understanding that creating an IH inside the UCB is simply plain wrong. It is a must for every client of the UCB to supply the right IH. This can be the VCL-based IH usually used in OOo, but it can also be a completeöy different implementaion. For instance the UCB is heavily used by the command line tool unopkg. unopkg must not instanciate the VCL-based IH, but this is what you're currently doing.
As MBA has already mentioned, only the InteractionHandler from MediaDescriptor should be used during document opening. If there is no InteractionHandler, no interaction is allowed, in this case the authentication has no other choice than to fail and throw an exception. The scenario when MediaDescriptor has an InteractionHandler is more complex. If an InteractionHandler is provided to UCB, it will be used not only for query and authentication interactions, but also for error handling. Although most of UCB errors ( "no file", "can not open readwrite" and etc. ) are no errors in context of document loading, so they must not be shown. The only solution, that I see in this case, is to use a wrapper over InteractionHandler from MediaDescriptor, that handles only a subset of interactions, and to provide this wrapper to UCB in all the cases when UCB is used to load a document. But that would require checking of the UCB implementation, since the implementation has to check whether at least one continuation has been selected, otherwise the related exception must be thrown by UCB. I am not sure that UCB does it already. By the way, does it really makes sense not to remember the password at least for runtime. The problem is that in this case there might be a huge amount of authentication dialogs, that makes the scenario quite unuseful. Is it really a case that has to be handled?
kso: Well, I guess it really is wrong, but I really don't know how to solve the problem described above without that - I learn just by what I see in the other parts of OOo, and in the debugger. And I saw creating the InteractionHandler in ucbhelper (http://lxr.go-oo.org/source/util/unotools/source/ucbhelper/ucbhelper.cxx#584), so I thought what was good enough for ucbhelper, must be good enough for ucb's ucp as well, or not? I'd really want to get rid of it - please tell me the way, and I'll do it [or feel free to change it in the CWS yourself - that would be even more appreciated!] BTW, I don't rely on the VCL implementation, I rely on whatever uno::Reference< task::XInteractionHandler > ( xFactory->createInstance( rtl::OUString( RTL_CONSTASCII_USTRINGPARAM("com.sun.star.uui.InteractionHandler") ) ), uno::UNO_QUERY ); returns me, which to me looks OK from the UNO point of view [which still might be wrong from the UCB point of view, of course ;-)]
kendy: using a customized IH, like mav suggested, sounds really very good to me. mav: The special IH we're using while loading documents, isn't this exactly what kendy is looking for? Where is this IH implemented? Can he not just use it as well? There would be no need to implement another specialized IH, then.
mav: Thank you for your answer, this is what I needed. > The only solution, that I see in this case, is to use a wrapper over > InteractionHandler from MediaDescriptor, that handles only a subset of > interactions, and to provide this wrapper to UCB in all the cases when UCB > is used to load a document. > But that would require checking of the UCB implementation, since the > implementation has to check whether at least one continuation has been > selected, otherwise the related exception must be thrown by UCB. I am not > sure that UCB does it already. Please, do you think you could help me with it? Like a proto-patch, or even directly in the CWS? Or at least some code pointers [like it's something similar to what we do in blah/bleh/ugh.cxx:234-450, method GG::HH()]? Thank you a lot in advance!
> Please - would that be OK for you? (Until calcshare2 is not in, show it as > read-only if locked; when calcshare2 is integrated, do the changes in the > WebDAV UCB?) I see no problem if implementation is adjusted after calcshare2 is ready. But I would like to repeat, after integration of calcshare2 the WebDAV file locking of documents will not work at all. Although it can be easily fixed with a small change.
> mav: The special IH we're using while loading documents, isn't this exactly what > kendy is looking for? Where is this IH implemented? Can he not just use it as > well? There would be no need to implement another specialized IH, then. There is a number of similar wrappers implemented in office including the mentioned one, but I do not know any that would do exactly what expected in this case - allow only authentication requests. From other side it should be easy to implement one, docfile.cxx has similar implementation that is not used any more, so it could be adjusted to do exactly this.
kendy: I can implement the wrapper and use it in sfx2 and comphelper tomorrow. Please add comphelper project to cws in this case.
mav: Thank you a lot! Added the 'comphelper' module to the CWS. Also I now noticed CommandEnvironment_Impl directly in webdavcontent.cxx, maybe it would be better to re-use this one? It seems to be exactly what is needed - implementing handling the passwords only. kso, mav: Do you think it would be enough to move this one to the comphelper?
kendy: I assume that Kai's reservation against using uui is that this will create a runtime dependency against VCL as this component links against it. And UCB is a component that should be usable in "VCL-free" environments. But anyway, I think we are moving towards a good solution.
mba: OTOH - UNO handles it correctly, and in VCL-free environments, the query just returns a null IH, and all bad that happens is that even with the hack the webdav UCP does not authenticate (the code is ready to handle xIH.is() == false). But yeah, I think it will be better without this. And yes, I'm very pleased with the direction of all this [not just the interaction handler part], and really thank all the involved people for helping and extremely fast responses!
kendy, mav: I still cannot understand why we cannot use the already existing specialized IH here that was designed exactly to handle all interactions that are of interest when loading documents. Have we actually tried this? If yes, what went "wrong" with it? Are we sure that we're only interested in authentication requests when loading docs "the new way"? Or do we just not know that we're actually interested in all the other interactions that are handled by the doc-loader IH?
kso: You mean the CommandEnvironment_Impl in webdavcontent.cxx? I did not use it, because I overlooked it (as I said in the comment 45). That's why I asked in that comment what to do with this - if the correct use is just to use this one in the 'else' part of the piece code from comment 35, perfect for me!
kendy: No, I'm talking about the IH implementation mav mentioned before. The one, that resides in sfx2. I still think that it is worth a try to just use it instaed of implementing something new in comphelper. I don't think that using CommandEnvironment_Impl is a good idea. This class has been introduced as a workaround for a bug only (parts of the code are copied verbatim from UUI Interaction Handler implementation, for instance) and I'd really like to get rid of this rather using it at other places. From my point of view the wrapper-approach mav suggested is the best option we have to solve the problem. mav: Where can we find the implementation of the before mentioned document-loader IH?
kso: There are three interaction handlers that are used during the loading implemented in framework/source/interaction. Not all of them are wrappers, and they are definitely not related to the scenario. So I thought that you were meaning the interaction handler implemented in sfx/source/doc/docfile.cxx ( that is not used any more ) and the implementation in comphelper/source/misc/mediadescriptor.cxx ( that replaces the previous one ). Both of them prevent showing of "access denied", "locking violation" and "unsupported data sink" error. All other errors would be handled. I just was not sure that this is enough. It is definitely enough when the InteractionHandler wrapper is used in MediaDescriptor implementation only, since in this case the error will be show only once while retrieving the stream. The problem is that in case this handler is used in SfxMedium and probably some other places in sfx2 the same error might be shown more than once because the current implementation in sfx2 does not expect interation in most places where UCB is called. From other side the probability of such an error is small enough to use the InteractionHandler wrapper without change for now and adjust it later.
While thinking a little bit more about this I have recognized that it is indeed not necessary to adjust the InteractionHandler wrapper in the mediadescriptor.cxx to avoid duplicate errors. There is already another wrapper in framework that does exactly this, and if I am not wrong it is used already during the loading. Sorry, I needed some time to recognize this. :) So as kso has mentioned we can just export the wrapper from mediadescriptor.cxx and use it in sfx2. Or even use the old implementation from the docfile.cxx as first step since it does pretty the same.
I have just commited the changes to the cws. I have compiled them but did not test in office since I do not have access to the cws build. The following files has been changed: comphelper/inc/comphelper/stillreadwriteinteraction.hxx comphelper/source/misc/mediadescriptor.hxx sfx/source/doc/docfile.cxx In addition, please test opening of documents from a WebDAV server that requires authentication but does not support locking.
mav: Thank you a lot! Works great now without the 'the hack to create your own Interaction Handler' from comment 26 (committed the removal already) both for the normal server, as well as with the one not supporting locking. So far it seems that the getProcessServiceFactory() removal is the last thing (right?), working on that now.
kendy, mav: Thanks guys for sorting the Interaction Handler stuff out. kendy: Yes, next thing to to is to get rid of the getProcessServiceFactory() stuff. Additionally, I will check whether your new code for the "open" command still fits to the fix for a "open" command multihtreading issue I just did in a recent CWS. If changes are needed, I will do them directly on your CWS. So, what are the things we agreed to target in followup CWS(s)? (1) Fix webdav file locking after integration of CWS calcshare2 (2) In case of file is locked, make DAV-UCP and GVFS-UCP behave like File-UCP after CWS calcshare2 is integrated (use new "file is locked" interaction, ...). (3) Fix implementation of property "IsReadOnly" to respect server-side read-only case (=> DAV property "lockdiscovery", describes the active locks on a resource) (4) Support locking not only if XActiveDataStreamer is used, but also for XOutputStream and XActiveDataSink (implement "close" command for all UCPs supporting locking?) Anything else?
@kendy: I am wondering whether any of the changes to module tools, especially the controversial changes in tools/inc/tools/urlobj.hxx:1.2.118.2 and tools/source/fsys/urlobj.cxx:1.61.26.2 (see <http://www.openoffice.org/servlets/ReadMsg?list=dev&msgNo=22151>) are necessary for webdav (resp. gvfs) locking.
sb: The introduction of INET_PROT_GENERIC_HIERARCHICAL is from 79843, so it's not completely locking-related; added (cwsaddtask) the issue to the CWS. [Sorry, the same thing as what happened with #i84137# (comment 27) :-(]. As to the username & password, it is convenient in the WebDAV case to be able to provide the possibility, and the code in the WebDAV UCP had code for that (though a bit broken). I agree that it is not good user behavior, but OTOH the users want it (https://bugzilla.novell.com/show_bug.cgi?id=363363), and other applications (in KDE and Gnome) support this as well, so... But if you insist it should not be there, I can make it ooo-build only [though I'd rather up-stream it].
kso: getProcessServiceFactory() calls are gone, and it still seems to work ;-) As to the points you wrote, I'd like to: (5) Remove the abstract classes.
kendy: I checked for the potential multithreading issue I mentioned earlier. Looks good. No changes were needed. > kso: getProcessServiceFactory() calls are gone, and it still seems to work ;-) Cool. > (5) Remove the abstract classes. Thanks. I forgot that we already agreed to change this. If I only had an idea how to remove the ugly hacks from NeonInputStream.cxx I just stumbled over... I will come back to you on this.
@kendy: Re INET_PROT_GENERIC_HIERARCHICAL: The code did look like deja vu, but I had completely lost track of issue 79843. Thanks for clarifying. Re username & password: Nah, having it only downstream in ooo-build is probably not what anybody wants. I do not *insist*, so if you *do* insist---go ahead. ;)
sb: I kind of do ;-) - so I've let it there. kso: As we discussed by mail, let's try to make the CWS ready is it is now, regardless the hack in NeonInputStream.cxx. I've resynced it to DEV300-m16, builds beautifully, but unfortunately, the interaction handler solution still seems not to be perfect :-( mav: Please, do you think you could have a look? During startup (./soffice -calc), I get error dialogs like 'Error loading BASIC of document file <the-installation>/share/uno_packages/cache/uno_packages/3OLqkL_/pdfimport.oxt/script.xlb: General Error. General input/output error.' [repeats several times, for each extension] At the moment, I'm double checking that I don't get it without the CWS, and I'll try to have a look myself as well - but... ;-)
mav: OK, the error dialog box looks unrelated, I get it even without the CWS ;-) - so I'll do a bit more testing tomorrow and pass the CWS to QA.
The error message is a result of an installed extension that was not properly deinstalled (but perhaps removed in the file system). You can fix it by either removing the whole user profile or just the uno_packages folder - or by poking inside of some files that might take more time.
Unfortunately, now I found a real problem - the save does not work, again thanks to non-existing interaction handler :-( mav: Please, could you re-test in your environment that I'm not seeing ghosts here? ;-) Could you please help me solving this? I got into a dead-end a bit after trying to patch left and right - see the attached proto-patch please, but it is still not working :-( - apparently either more places has to be touched, or I'm patching them a wrong way... Thank you a lot in advance!
Created attachment 54343 [details] The patch.
I see the problem. The InteractionHandler is still not transported to UCB always. The last patch part regarding docfile.cxx looks good, although I am not sure that SfxMedium::GetInteractionHandler() will be accepted by all the compilers as const, since it changes the member of the pImp pointer. The last patch part regarding fileaccess.cxx and ucbhelper.cxx does not help since the xInteractionHandler reference wrapped with ::comphelper::StillReadWriteInteraction is empty, and thus can not show any dialog. But even if is would be initialized with the default InteractionHandler, it would be even worse hack than it was before. Before the interaction handler was explicitly created only for webdav protocol, and now it would be created for all the protocols. So from my point of view it is no acceptable solution. The ucbhelper.cxx could be fixed easily by providing the InteractionHandler to the help functions as an argument. The problem with fileaccess.cxx is more difficult, the service implemented there does not allow to provide the InteractionHandler at all so it should be reimplemented. As well as the implementations that use the service. I am not sure that it is possible to reach this for OOo3.0. To reach the OOo3.0 deadline I would suggest to use SfxMedium as a buffer as it was actually done before ( implicit ) for webdav protocol, since the protocol did not allow to write directly. It is a pretty small change, in SfxMedium::GetOutputStorage() the call GetTempFileNoCopy() should not be called only for local files for now ( IsLocalFile() check instead of SupportsActiveStreaming ). If this workaround will be used, please write me a new bug to fix SfxMedium for the direct saving for the webdav account in future.
mav: Thanks for the help! Now fixed in the CWS, I'll attach here the patch I used [both the SfxMedium::SupportsActiveStreaming() and SfxMedium::GetOutputStorage() hunks are needed to make it working]. I also used const_cast<>() instead of the making GetInteractionHandler() const, hopefully this will be acceptable by all the compilers :-) Changing the issue to FIXED - should be OK now. Thanks everyone for the help!
Created attachment 54379 [details] The patch.
Reassigning to Tobias for verification.
up-stream version has too old gnome-vfs, and so the gnome-vfs part cannot be compiled. As discussed with Tobias, it was removed from the CWS, and now is tracked as issue 91151.
mav: I've been testing the stuff a bit more now (due to resync to m21), and found more problems :-( The scenario that is failing now: Open the WebDAV document, save it (works OK), and save it for the second time. Now, pInStreamItem->GetValue() >>= pImp->xInputStream; (line 2248, in SfxMedium::GetMedium_Impl()) fails, and results in error dialog box saying "The object cannot be accessed due to insufficient user rights.". Any ideas, please? Backtrace: #1 0x00002b96cf426e1b in SfxMedium::GetStorage (this=0x151e310) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/docfile.cxx:1160 #2 0x00002b96cf4623e4 in SfxObjectShell::DisconnectStorage_Impl (this=0xe269f0, rSrcMedium=@0x151e310, rTargetMedium=@0x1544f60) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:1953 #3 0x00002b96cf46a9f2 in SfxObjectShell::SaveTo_Impl (this=0xe269f0, rMedium=@0x1544f60, pSet=0x14fbcb0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:1419 #4 0x00002b96cf46f659 in SfxObjectShell::DoSave_Impl (this=0xe269f0, pArgs=0x14fbcb0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:2683 #5 0x00002b96cf46fbdb in SfxObjectShell::Save_Impl (this=0xe269f0, pSet=0x14fbcb0) at /local/ooo-build/ooo-bui2322 in /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/docfile. cxx #6 0x00002b96cf4cf70e in SfxBaseModel::storeSelf (this=0xe27018, aSeqArgs=@0x7fffdcf32680) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/sfxbasemodel.cxx:1530 #7 0x00002b96cf4e18f3 in SfxStoringHelper::GUIStoreModel (this=0x7fffdcf32ea0, xModel=@0xe26e28, aSlotName=@0x7fffdcf336d0, aArgsSequence=@0x7fffdcf32f90, bPreselectPassword=0 '\0', aUserSelectedName=@0x7fffdcf336c0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/guisaveas.cxx:1230 #8 0x00002b96cf47bfc1 in SfxObjectShell::ExecFile_Impl (this=0xe269f0, rReq=@0x151bb70) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objserv.cxx:674 #9 0x00002b96cf47e46b in SfxStubSfxObjectShellExecFile_Impl (pShell=0xe269f0, rReq=@0x151bb70) at ../../unxlngx6.pro/inc/sfxslots.hxx:161
mav: A thing that seems to be related: The SfxMedium tries to create backup on the server, like document0.odt (when the original document is document.odt) in SfxMedium::DoInternalBackup_Impl() (sfx2/source/doc/docfile.cxx:2090) which leads to '500 Internal Server Error' with some WebDAV servers (and generally does not sound to me as a good idea, to create a new version of the file on the server). The backtrace: #9 0x00002b760385eed8 in SfxMedium::DoInternalBackup_Impl (this=0x1555540, aOriginalContent=@0x7fffa8af2c00) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/docfile.cxx:2135 #10 0x00002b760386111a in SfxMedium::StorageBackup_Impl (this=0x1555540) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/docfile.cxx:839 #11 0x00002b76038611ce in SfxMedium::GetBackup_Impl (this=0x1555540) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/docfile.cxx:849 #12 0x00002b76038a142c in SfxObjectShell::DisconnectStorage_Impl (this=0xe38080, rSrcMedium=@0xe92320, rTargetMedium=@0x1555540) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:1961 #13 0x00002b76038a99f2 in SfxObjectShell::SaveTo_Impl (this=0xe38080, rMedium=@0x1555540, pSet=0x15521e0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:1419 #14 0x00002b76038ae659 in SfxObjectShell::DoSave_Impl (this=0xe38080, pArgs=0x15521e0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:2683 #15 0x00002b76038aebdb in SfxObjectShell::Save_Impl (this=0xe38080, pSet=0x15521e0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/objstor.cxx:2766 #16 0x00002b760390e70e in SfxBaseModel::storeSelf (this=0xe386a8, aSeqArgs=@0x7fffa8af4080) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/sfxbasemodel.cxx:1530 #17 0x00002b76039208f3 in SfxStoringHelper::GUIStoreModel (this=0x7fffa8af48a0, xModel=@0xe384b8, aSlotName=@0x7fffa8af50d0, aArgsSequence=@0x7fffa8af4990, bPreselectPassword=0 '\0', aUserSelectedName=@0x7fffa8af50c0) at /local/ooo-build/ooo-build/build/dev300-m19/sfx2/source/doc/guisaveas.cxx:1230 Thank you for your help in advance!
The CWS is resynced to m21, reopening so that the abovementioned problems are fixed.
mav: Reassigning to you for now - could you please help me with this? I'm still a bit confused by the SfxMedium stuff :-(
There were actually two problems. The first one, the main reason is that the backup creation has got no InteractionHandler, so it has failed. The normal backup creation copies the file to the backup folder, and only if it is not possible it tries to make backup in the destination folder ( for the case of protected folders ). But that should happen only for file-systems, so the second problem was that the target folder was tried for backup creation in case of webdav protocol as well. The fix for both problems follows soon.
mav->kendy: Sending the bug to you for now. Please send it back if the provided fix does not help.
Created attachment 54863 [details] mav->kendy: A schematic patch. Sorry, did not compile it, preparing of the cws environment would take too long.
mav: Unfortunately, did not help :-( I'm now trying to catch all the places that call authenticate() without an existing interaction handler, and there's still quite a bunch of them - I wonder if the decision I must not create an IH in the webdav ucp was really right :-( Anyway - when I have all, I'll update you with info if it helped, and with a patch for review.
mav->kendy: Does the patch solve the problem the stack points to?
mav: It did not help, so still clueless :-( If you can get this CWS environment at some stage & try, that would be most appreciated. Thank you a lot for your help!
mav: Tracked again the 1st case, and it is the same even with the changes - pInStreamItem->GetValue() >>= pImp->xInputStream; (line 2248, in SfxMedium::GetMedium_Impl()) still fails.
mav->kendy: I am in progress with building the cws locally.
set target to OOO 3.1 because code freeze for 3.0 is today.
any news?
After the task has got the new target, I have postponed the investigations. But the issue has highest priority in my 3.1 list currently. So I plan to start next week with investigations.
I have just commited the change for the cws. The scenario with the second saving looks to work well now.
mav: Great, thank you a lot! Let me resync the CWS to the most current milestone (probably m29), test it a bit, and set Ready for QA :-)
mav: So I got to this again; I've resynced to m29, and again I'm getting "Error saving the document XY: Error creating object. Could not create backup copy." when I try to save:-(( Unfortunately I did not try before the resync to see if it was there even before, or if it's something new. Could you please have a look again?
Reopening according to the above comment.
I am building the cws currently and will investigate the problem as soon as possible.
mav->kandy: The reported by you problem ( open a document, save it, save it second time => error ) that I have fixed before the resync is not reproducible by me after the resync. The only problematic scenario I found currently is when a new document is stored to webdav. This problem should be fixed in mav37 cws ( should be integrated in the next milestone of DEV300 branch ). If you know any other problematic scenarios please explain them in details. I think it makes sense to resync the cws one more time after integration of mav37, since the changes from mav37 affect the storing process.
mav: Thank you! I'll resync & retry when it's integrated - hopefully all will be OK then :-)
mav->kendy: I have integrated the changes from mav37 into this cws and it looks like they are not enough, although necessary. The additional problem is that webdav ucp returns true as property value of "IsDocument" property if the document does not exist. As I understand the ucp must return false in this case ( or throw an exception, the idl specification seems not to cover this case explicitly ). Please correct me if I am wrong with my understanding.
just a small correction to my last comment: I have integrated the changes locally, I did not commit anything.
Created attachment 56084 [details] Could you please try with this patch?
mav: For non-dav resources, it implicitly returned IsDocument == true; the patch refrains from that when we got 404 (not found) - hopefully it does not break something else ;-)
mav->kendy: Yes, the patch solves the problem.
Do we have a deadlock here? Is somebody waiting for somebody else? And what about issue 91151 - does it still have any relevance?
mba: No deadlock ;-) I resynced the CWS to m37, but unfortunately failed to make it perfect for the feature freeze :-( - I'll resync it to m40 now, and hopefully it'll get to 3.2... mba: As to the issue 91151, no idea - gnome-vfs got obsoleted by gio in the meantime, so I guess the gnome-vfs with the locking features will never get to the up-steam builds.
Ah, great! Please let me know if something gets stuck. wrt. issue 91151: we are currently discussing in issue 84137 whether gvfs is still needed; we will definitely keep it for Solaris (but I would understand if that's not enough for your to justify the upstreaming of the gvfs locking ;-)), I'm not sure how many combinations of older Linux versions and OOo3.x we have to support where gio can't be used.
move target to 3.2
Seems like issue 103442 is affected by these patches. Please review.
I'm using OpenOffice from Ubuntu Jaunty, which incorporates these patches. I'm attempting to edit files in Atlassian Confluence via DAV, but Confluence requires a file to be LOCKed before the file can be updated with a PUT request. This code, however, appears to unlock the file just before writing, then lock it again just after writing. Is there a reason for this behavior? Can it be adjusted to leave the file LOCKed while performing the PUT? These are the requests seen from OO when editing a file via DAV on an Apache 2.2 server: <Ask OO to open the file> "PROPFIND /test/test.doc HTTP/1.1" 207 543 "-" "-" "LOCK /test/test.doc HTTP/1.1" 200 402 "-" "-" "GET /test/test.doc HTTP/1.1" 200 11264 "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 482 "-" "-" <Make some changes and ask OO to save the file> "UNLOCK /test/test.doc HTTP/1.1" 204 - "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 543 "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 500 "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 482 "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 1177 "-" "-" "GET /test/test.doc HTTP/1.1" 200 11264 "-" "-" "PUT /test/test.doc HTTP/1.1" 204 - "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 482 "-" "-" "PROPFIND /test/test.doc HTTP/1.1" 207 543 "-" "-" "LOCK /test/test.doc HTTP/1.1" 200 402 "-" "-" "GET /test/test.doc HTTP/1.1" 200 10240 "-" "-" <Close the file> "UNLOCK /test/test.doc HTTP/1.1" 204 - "-" "-" Note that the file is UNLOCKed before doing the PUT, then LOCKed again afterward.
As these patches you mention have not been included the upstream OpenOffice.org, this is the wrong place to report problems in them. File a bug in the Ubuntu bug tracker. (From which then the maintainer will forward it to bugzilla.novell.com where the ooo-build bugs are.) P.S. It's counter-productive to use weird terms like "Atlassian Confluence" expecting readers to know what that is if it matters to the case (which it probably doesn't?). You could at least explain shortly, or just use some generic term "a website" or whatever.
what's the status with the patch and integration ?
A disaster.
*** Issue 105464 has been marked as a duplicate of this issue. ***
BTW, we have noticed that in ooo-build, where we use a current reincarnation of kendy's patch, when one pastes into Writer the HTML contents of a web page, OOo wants to read images in that HTML page through the webdav UCP (and even tries to lock them then, i.e. lots of in this case IMHO pointless hideous complexity gets involved). Is the use of the webdav UCP in a case like this as expected and does it happen also in upstream OOo? Should the images be read in as read-only? Or is the expectation that if one is editing a document in Writer that has originated as a web page, then the images included in that documented are editable and saveable back to the location from which they are opened even? Please note that I am just asking for some friendly hints from upstream experts here, I am not expecting you to solve problems caused by our code patches for us.
There is no special webdav UCP, we have the same UCP for webdav and http. As the former is just an extension to the latter we thought that this should be fine (and I still believe that this is true). I didn't study the patch, but if this patch has the consequence that the UCP itself decides on whether locking should be applied or not, this might explain the observed behavior. Opening the files "read only" would be fine if that solved the problem, though I think that we should be able to open files in read/write mode without locking also (at least that is possible in the file system). Whether locking is applied or not should be a decision of the code that uses the UCP to open files. BTW: can you explain what your comment "a disaster" means? Is there anything I can do to make the situation "less disastrous"? ;-)
tml: Could you please attach the "current reincarnation of kendy's patch" to this issue. I would be happy to take a look at it.
> There is no special webdav UCP, we have the same UCP for webdav and http. Yes, but it is in a source directory called "webdav" and many of its source files have a "webdav" prefix, so I tend to call it "the webdav UCP" ;) > if this patch has the consequence that the UCP itself decides > on whether locking should be applied or not, this might explain > the observed behavior That probably is what is going on. I *think* that when I worked on making the patch "work" in the 3.0 timeframe, I wasn't even fully aware that it indeed is used also for plain http , which was of course a bit of failure from my side. > can you explain what your comment "a disaster" means? It means that the more time I spend staring in my editor or the debugger at the code touched and added by the patch, the less I understand it, and the more convinced I am that it "works" by accident;) > Could you please attach the "current reincarnation of kendy's > patch" to this issue. Sure, it's no secret, in a public git repo and all, so I'll attach it below.
Created attachment 65504 [details] The current "webdav-locking.diff" in ooo-build, applies to ooo320-m2 for instance
UCB clients, when executing the command "open", can supply information whether locking of a resource is requested. Refer to the api ref http://api.openoffice.org/docs/common/ref/com/sun/star/ucb/OpenMode.html or ucb/source/ucp/file/bc.cxx (-> look for SHARE_DENY) to get the idea. WebDAV-UCP currently does not support these open command options. IMO, implementing this missing functionality and replacing all the implicit locking stuff with correct open command usage in framework/sfx2 is the right starting point. Everything else is just hacks. ;-) OpenMode DOCUMENT_SHARE_DENY_WRITE should be mapped to a DAV write lock, I guess.
tml wrote: >It means that the more time I spend staring in my editor or the debugger at the >code touched and added by the patch, the less I understand it, and the more >convinced I am that it "works" by accident;) I fully agree here. -) After spending some time with studying the patch (and the changes done afterwardsin CWS webdavandgvfslocking1) I got the idea, but I still have concerns re both the concept and the implementation: a) The WebDAV-UCP simply cannot safely decide on its own whether it is okay to lock the DAV resource before doing the GET. It might be okay in the context of loading a document, but not in case of loading linked graphics (like tml described above) or when doing an extension/product update ping. Well, with the patch a client might control this using the type of data sink handed over to the "open" command (it must be a data streamer in order to trigger locking, IIRC), but I consider this not optimal, despite of the current framework bugs not using the different data sinks "right". b) The patch is rather big, even after taking out the code peaces that are not related to WebDAV locking. Larger code changes in sensitive areas, spread over many modules ... c) ooo-build uses this patch for a longer time now and still has issues with this feature (see above) d) The CWS is based on an ancient OOo milestone. Resyncing will be like hell, I'm afraid. e) If I got things right, the implementation of determining whether a resource is locked and therefore shall be openened readonly is currently based of a member variable of the corresponding UCB content object. This breaks as soon as the corresponding object gets released while the document is open, which does currently not happens -- by luck. f) The current implementation contains many room for improvements (like discussed and confirmed earlier in this issue), including hacks for that we don't have a clue how to do things right. Thus, I sat together with mav (he is known to be our document load/save framework expert) and discussed the locking-topic in depth. The outcome is that we would prefer an implicit locking approach. Every UCB content implementation (not just WebDAV) could support two new commands "lock" and "unlock". "lock" creates an exclusive write lock with an indefinite lifetime. Client must call "unlock" to release the lock when it is done with the document (document gets closed). This approach has several advantages over the current (the implicit locking) approach: a) The OOo document load/save framework is the right place to decide whether a lock is needed for the document resource. All needed context is available there. b) Meanwhile (since the patch was created) the framework was greatly enhanced in a way that it is now easy to add the lock/unlock logic. c) Far less (non-UCB) code changes in less places are necessary to implement generic document locking feature for OOo. d) The new concept does not only work for WebDAV. Even the file-UCP could be extended to support the new lock/unlock commands instead of the "open" command with special arguments approach it currently supports. e) a clean implementation, without hacks should be possible (can tell this for sure for WebDAV-UCP ;-) Having all this said, I want to give you a heads up that I created issue 106830 to extend the WebDAV-UCP to support the new lock / unlock commands. Implementation makes good progress. I took as many code peaces (as well as conceptional ideas) from the original locking patch. This saved me some time. I very appreciate the work already done by the original patch contributors. Thanks a lot. Plan is to implement the necessary OOo document load/save framework changes asap. This part will be done by mav. If all works out well, we will end up with OOo 3.3 supporting WebDAV locking in a clean and maintainable way. :-)
> we would prefer an implicit locking approach. Every UCB content implementation I meant of course "explicit locking approach". ;-)
> Resyncing will be like hell,I'm afraid. Indeed. I spent an evening trying but didn't get very far...
kso: I like your approach :-) In addition to the abovementioned, far too many places in OOo used to special-case the case when files were written, I believe this will help to clean up that too. Thank you! [Just a note about the indefinite lifetime, in the WebDAV case it should be implemented as a lock that tries to renew itself before the expiration - for the cases when the connection is lost, etc. Otherwise it might get locked on the server forever without any possibility to unlock it from the client side.]
>kso: I like your approach :-) Thanks. :-) >[Just a note about the indefinite lifetime, in the WebDAV case it should be >implemented as a lock that tries to renew itself before the expiration - for >the cases when the connection is lost, etc. Otherwise it might get locked on >the server forever without any possibility to unlock it from the client side.] Right. My implementation of the "lock" comamnd always requests 180-sec timeout locks and I use a thread that triggers a lock refresh 30 secs before the lock expires. Works out pretty good, except for IIS and WSS, which behave "weird" wrt. DAV locking. Still working on that.
The corresponding CWS 'webdavandgvfslocking1 is on target OOo 3.4. Therefore I change the target of this issue accordingly.
I think this issue should now be resolved as a duplicate of i#106830, which seems to use at least partially code/inspiration from the patch here anyway. *** This issue has been marked as a duplicate of 106830 ***
No, not a duplicate of, but depending on issue 106830. Reopen.
Added a new one, see issue 126305, where it takes into account the serf library added to AOO
A WebDAV compliant server is not required to support locking in any form. If the server does support locking it may choose to support any combination of exclusive and shared locks for any access types. DAV:lockdiscovery property The DAV:lockdiscovery property returns a listing of who has a lock, what type of lock he has, the timeout type, the time remaining on the timeout, the associated lock token and the root of the lock. <!ELEMENT lockdiscovery (activelock)* > <!ELEMENT activelock (lockscope, locktype, depth, owner?, timeout?, locktoken?, lockroot) > Depth: the value of the Depth header <!ELEMENT depth (#PCDATA) > owner: provides information about the principal taking out a lock (see Section 2.5.5). <!ELEMENT owner ANY> timeout: the time remaining until timeout of a lock. <!ELEMENT timeout (#PCDATA) > locktoken: the lock token associated with a lock; the href element contains the lock token. <!ELEMENT locktoken (href) > lockroot: the URL that was specified as Request-URI in the LOCK creation request; the href element contains the URL (see Section 2.5.3). <!ELEMENT lockroot (href) > As discussed above, the principal authenticated for the UNLOCK request MUST be allowed to remove the identified lock. Thanks !! http://qwikfix.co.uk/bt-customer-services-contact-number/