Apache OpenOffice (AOO) Bugzilla – Issue 64717
Layered accelerators
Last modified: 2007-02-01 14:53:33 UTC
I'll attach code that does 'layered accelerators': > Luckily of course each accelerator has a unique key / name - so we can > easily overlay these guys; have a true 'default.xml' file [ perhaps in > the accelerator/ directory ? ] that we then overlay much smaller files > for i18n on top of. The principle is easy, as described by Michael. Apart from reading the global/accelerator/<lang>/default.xml, it also reads global/accelerator/default.xml (similarly for modules/<module>/accelerator/), and if it contains not yet assigned accelerators, it assigns them. This adds a lot of freedom in creating the accelerator files. The really global accelerators can be defined global/accelerator/default.xml, and their language mutations can be re-defined in global/accelerator/<lang>/default.xml (usually very small). And similarly for the modules - they do not have to define accelerator for Save, e.g.; it is already defined in the global default.xml. The most important change is the one in framework/source/accelerators/acceleratorconfiguration.cxx (AKA the ~10 lines or so ;-) ), the rest is just supporting it - to have access to the language independent path (global/accelerator/default.xml), etc. Please comment on this approach, and if you are OK with it, I'll create a CWS for it, etc. Thank you in advance!
Created attachment 35991 [details] The patch.
Thanks for the patch, we will work on it as soon as possible. As the last CWS integration will be next thursday I don't expect that we can finish it for 2.0.3.
.
Sorry ... but it's not so easy to implement layered accelerators .-) The patch cant be accepted, because: a) It open files inside our "share" layer in read/write mode. That cant work especialy on unix. The method openTarget() was designed to open files inside the user layer for read/ write operations. b) Our current implementation copies "share/<locale>/default.xml" files to "user/<locale>/ current.xml" before it uses these files. But with this patch we will copy only localized shortcuts - not all. So customization of all shortcuts cant work. c) Some code is superflous. E.g. the member m_lStoragesNoLang isnt used realy. It was copied only. Currently we are thinking about switching from our current implementation to the normal xml configuration using xcs/xcu files. There we will get all these features. But for a OOo 2.0.4 it's not so easy. Because we have to migrate might be existing user configured shortcuts on demand. BTW: Doing so we will get another feature ... more then 3 layers, which isnt solved with this patch here also .-)
a) Well - looking at the openTarget() implementation - it is designed to work with read-only share: // try it readonly if it failed before. // inform user about errors (use original exceptions!) nOpenMode &= ~css::embed::ElementModes::WRITE; xStream = xFolder->openStreamElement(sFile, nOpenMode); We can default to css::embed::ElementModes::READ when bUseNoLangGlobal is sal_True - I see no problem with that ;-) b) Yes, it copies that - but I consider it a bug, not a feature... Why should the entire configuration be copied for the user? What if we ship with a new version of OOo, and we want to change some of the accelerators? This improves the situation from my point of view... Still, the user's accelerators have higher priority if I did not make a mistake ;-) c) Right, sorry for that. I'll clean that up, of course. And yes, I have no problem with a better solution for post-2.0.4 ;-) But this patch could improve the situation a lot right now, without any deep impact on the current code & behavior.
reassign
a) The method openTarget() (as the name suggest) would be designed to return TARGET files, ready for reading AND (!) writing on the user layer. So changing that would break the interface of the PresetHandler class. Solution: Move returning a storage related to your new m_xWorkingStorageNoLang member from openTarget() to openPreset(). Of course acceleratorconfiguration.cxx has to be changed accordingly. b) I wouldnt say "it's an error" ... it's more "the simplest way to implement handling of two layers" .-) Anyway: your implementation reach also a state, where the full configuration will be stored on the user layer. Please use the following steps to reproduce that: - start without a user layer - open a writer - type some letters into the document => as a result of that the localized default.xml from "en-US/default.xml" was copied to the user layer. - open inside the "Tools->Customize" dialog the "Keyboard" tab page - change something there and press OK => now the full content of the accelerator cache was flushed to the user layer .-) So your implementation does not solve this "problem" - which isnt realy a problem for me (currently). But using the normal XCS/XCU configuration for accelerators will solve it. c) Your problems show me, that it would be easier to change the implementation to the XCS/XCU configuration .-) Because ... d) ... your patch doesnt work for our "Tools->Customize->Keyboard" tab page. E.g. changing existing shortcuts works only in case the office will be started new. Further you should use a debug version for testing. There is an assertion from the xml reader class, that existing keys will be not overwritten by new keys. May be you should change this code in a way, that it overwrites even existing keys without showing a warning. But then you have to change the order of reading the different files. Because then m_xWorkingStorageNoLang has to be readed first, before m_xWorkingStorageUser is asked for content. Otherwhise you overwrite user defined keys. Regards Andreas
a) openTarget() still opens a target - which is taken as the first argument. The difference in my patch is _where_ the target is open - in user share, or in NoLang share according to the new parameter. I still do not see a reason why not to change that - it's documented, etc. The statement 'AND (!) writing on the user layer' is just not true; have a look at the code, and you'll see that the target is open read-only if the read/write open request fails. But anyway, I have no problem with renaming openTarget() to something more sensible so that it would allow me do the needed change. Or - of course - I can even copy'n'paste it to a new method, but I'd like to avoid that... b) If you really want to store everyting in current.xml, I guess I can quite easily extend the code so that it stores all the accelerators there after it parses all the needed files, I see no problem with that... c) Sure! But how much time will it take to re-implement? 3 months? More? And if you plan it for 3.0 - it is at least 1 year from now; probably more. d) I'll try that, I've probably overlooked something. e) Again - no problem in changing the order of reading the files provided that I change the code in framework/source/xml/acceleratorconfigurationreader.cxx (but the assertion there is commented out, just a warning is issued). I wanted my changes to be touching the minimal set of files; but surely I can patch acceleratorconfigurationreader.cxx as well. Sorry to ask that - but why did you make the issue 'INVALID'? I understand that you have concerns wrt. the code; but I am willing to fix all the issues. Provided that you want to throw all this away in favor of the XCS/XCU configuration later anyway, what's wrong with an intermediate solution? So - if I fix a), b), d) and e) - will you at least think about allowing this patch in? ;-) Thank you in advance!
Re-assignment ping-pong ;-)
a) You should not rename openTarget() ... you should use the existing method openPreset() and move your code from openTarget() to openPreset(). And it's not a question if it's possible that openTarget() can return readonly storages. It's designed that storages returned by openTarget() MUST(!) be opened in read/write mode and openPreset() returns readonly storages. Please move the code and everything is fine. b) Copying / Storing all informations to the user layer isnt realy a bug. Not in my implementation nor in your patch. No discussion about it .-) c) It's not realy a huge task. I think 1-2 weeks. But there is one problem: migration of real user defined shortcuts from OOo 2.0.x to OOo 2.0.4. For an OOo 3.0 version there is no such problem. Because there we support an explicit migration of such user settings. But that's not possible for a normal product patch. So we have to find a solution to migrate user defined keys on demand ... or we have to ignore it. That's why we dont started an implementation till now. Because this question isnt answered till today .-) d) No problem. Of course this patch has to be verified by our QA so we dont kill the feature. That can be a disaster e.g. for the writer module. The cursor traveling of the writer base on shortcuts ... e) Sorry for setting this task to INVALID. Of course we can find another way to communicate ... but adding some comments without sending the issue back will have the risk, that you dont read my comments and wait for completion of this issue .-) May be we can communicate further via email ... ? f) Yes - I allow to add this patch to OOo ... in case it works as aspected and do not break the feature. So if you make the mentioned changes and our QA shows us no problems - you can integrate it into OOo. Regards Andreas PS: further comments via email ?
No problem with mail, but IRC would be better... kendy on irc.freenode.net, channels #openoffice.org, or #go-oo; join, if it's OK for you :-)
ab wrote: > b) Copying / Storing all informations to the user layer isnt realy a bug. > Not in my implementation nor in your patch. No discussion about it .-) Andreas - Honestly! ;-) - the corollary of not cutting & pasting -code- all over the place is that -duplicating state- is *really* bad programmatic practice. I find it hard to understand that you don't see that as a design flaw. The archetypal problem with duplicating state is keeping it consistent: consider the (outlandish & perhaps unimaginable) case that OO.o 2.0.0's accelerator configuration scheme was not perfect. This would mean that people would want to configure it: that would then freeze a snapshot of that (imperfect) setup for all time [ until a 'version upgrade' threw all their changes away ;-]. ie. this design screws us (as maintainers) wrt. changing & improving keybindings over time & migrating them to better & more intuitive patterns wholesale. Inasmuch that it does that [ which unless I'm mistake it does ], it's a -serious- problem, certainly for my customers [ who tend to complain about many of the accelerators not being where they expect them ;-]. If we can move to a design that doesn't have this flaw, while at the same time removing all the rampant & unmaintainable state duplication in the source tree this is a good step. Currently we duplicate the complete state per-language [source-tree] and per-user-that-changed-anything - we want to move this to ~no duplicated state in the source tree, and no duplicated state per-user: surely that is a good thing?
While I agree that the current situation is unfortunate, I don't consider it to be a bug. OTOH the proposed patch still is buggy so that Andreas doesn't want to integrate it as is and I support this. I still maintain the point that the changed configuration (that indeed is a necessary step) should be done in a clear and "clean" manner and I already outlined a possible strategy in one of the mails we have exchanged about this topic (and Andreas also mentioned it in one of his comments). Of course we still can use a "light weight" approach if it doesn't break anything, but at least for the current patch that's not where we are now. If anybody can provide a patch that doesn't break any current feature, that can be integrated without forcing Andreas to change a lot of code or do a lot of manual rework and if it doesn't create new problems for a future move to xcu-based configurations I support its integration, but only *if*.
AS->mmeeks: The reason for rejecting this patch was "Tools->Customize->Keyboard" doesnt work. The rest are "cosmetic changes". As mba already said: please provide a patch, which does not disturb an existing feature of the office and the patch will be accepted. And as I've already said: another solution would be the migration to the "normal xcs/xcu configuration" ...
Michael/Kendy: any update on this? Any objections against the proposed solution with moving the accelerator configuration to "the" configuration?
> Michael/Kendy: any update on this? Any objections against the proposed > solution with moving the accelerator configuration to "the" configuration? To my mind this just moves the problem around, and will (no doubt hideously) degrade performance. The root problem we are trying to solve here is the hideous duplication of state that means we can't possibly patch / maintain or improve keybindings. Using the configmgr is rather orthogonal to the imperative re-work: to remove the galloping state duplication and allow us (and our UI team) to get on with the interesting task of creating an improved set of keybindings for the whole suite. OTOH - Jan - can you look at the bug as points out ? surely that's fixable ?
as: Sorry for the long delay - I was distracted by some other tasks... Here's the updated patch that: - changes usage of (patched) openTarget() to (patched) openPreset() - the non-working "Tools->Customize->Keyboard" was fixed (it was a trivial bug, sorry for that) Please have a look at the updated version that I'll attach, I hope you'll like it :-) Thank you in advance!
Created attachment 38186 [details] The updated patch.
One more small improvement, I changed the simple m_aReadCache = AcceleratorCache(); (which solved the Tools->Customize->Keyboard problem) to + // impl_ts_load() does not clear the cache + // SAFE -> ---------------------------------- + aWriteLock.lock(); + m_aReadCache = AcceleratorCache(); + aWriteLock.unlock(); + // <- SAFE ---------------------------------- The ultimate version is here: http://www.go-oo.org/patches/src680/framework-layered-accelerators.diff
I will investigate into the patch asap ... if OOo 2.0.4 is out. Currently I have to make some more fixes for this version .-)
So - the deadlines for 2.1 are approaching... Please, any chance to have a look & approve the changes? ;-) I can do the CWS, etc. if you do not have a good one. Thank you!
AS->kendy: OK - the patch was applied on cws[patch01as], will be tested and integrated next time. But: this patch changes the code only. Please create your own "follow-up-cws" based on these cws here (if it was integrated into the master) to change the accelerator configuration itself accordingly. means: refactoring of the XMl files and creating of a new installation package.
as: Thanks! Will do when it appears in a milestone.
AS->ES: Please make sure that this patch does not influence our current accelerator feature .-) THX (cws=[patch01as])
ES->TM: Please file child tasks for the testers of the correponding applications (Writer, Calc, Draw...).
TM->ES: Sorry, but it´s your job as issue owner to decide AND to write subtasks if you think it is necessarry to have them.
TM->ES: please verify your issue !
ES->TM: Just for the records: It's meaningless to talk about "issue owner" when the issue didn't go to the correct owner. All this stuff (configuration): YOUR job, not mine. Verfied as ok in Writer.
Ok in OOF_m5