Issue 64717 - Layered accelerators
Summary: Layered accelerators
Status: CLOSED FIXED
Alias: None
Product: General
Classification: Code
Component: code (show other issues)
Version: OOo 2.0.2
Hardware: All All
: P3 Trivial (vote)
Target Milestone: OOo 2.x
Assignee: eric.savary
QA Contact: issues@framework
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-24 19:43 UTC by kendy
Modified: 2007-02-01 14:53 UTC (History)
5 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
The patch. (10.01 KB, patch)
2006-04-24 19:43 UTC, kendy
no flags Details | Diff
The updated patch. (8.92 KB, patch)
2006-08-01 18:32 UTC, kendy
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description kendy 2006-04-24 19:43:19 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!
Comment 1 kendy 2006-04-24 19:43:57 UTC
Created attachment 35991 [details]
The patch.
Comment 2 Mathias_Bauer 2006-04-26 07:56:00 UTC
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.
Comment 3 andreas.schluens 2006-04-27 12:55:12 UTC
.
Comment 4 andreas.schluens 2006-04-27 14:06:45 UTC
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 .-)
Comment 5 kendy 2006-05-02 11:19:09 UTC
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. 
Comment 6 kendy 2006-05-02 15:14:41 UTC
reassign 
Comment 7 andreas.schluens 2006-05-11 12:07:11 UTC
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
Comment 8 andreas.schluens 2006-05-11 12:07:53 UTC
.
Comment 9 kendy 2006-05-11 13:22:28 UTC
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!  
Comment 10 kendy 2006-05-11 13:23:08 UTC
Re-assignment ping-pong ;-) 
Comment 11 andreas.schluens 2006-05-11 14:26:13 UTC
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 ?
Comment 12 kendy 2006-05-11 14:45:41 UTC
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 :-) 
Comment 13 mmeeks 2006-05-17 13:32:10 UTC
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?
Comment 14 Mathias_Bauer 2006-05-18 09:36:06 UTC
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*.
Comment 15 andreas.schluens 2006-05-18 11:00:45 UTC
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" ...
Comment 16 Mathias_Bauer 2006-07-10 09:03:27 UTC
Michael/Kendy: any update on this? Any objections against the proposed solution
with moving the accelerator configuration to "the" configuration?
Comment 17 mmeeks 2006-07-10 20:09:07 UTC
> 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 ?
Comment 18 kendy 2006-08-01 18:28:13 UTC
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!
Comment 19 kendy 2006-08-01 18:32:01 UTC
Created attachment 38186 [details]
The updated patch.
Comment 20 kendy 2006-08-03 13:18:49 UTC
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
Comment 21 andreas.schluens 2006-08-03 13:35:35 UTC
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 .-)
Comment 22 kendy 2006-10-03 12:56:16 UTC
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!
Comment 23 andreas.schluens 2006-11-17 12:56:32 UTC
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.
Comment 24 kendy 2006-11-20 10:40:52 UTC
as: Thanks!  Will do when it appears in a milestone.
Comment 25 andreas.schluens 2006-12-06 10:35:31 UTC
AS->ES: Please make sure that this patch does not influence our current
accelerator feature .-) THX
(cws=[patch01as])
Comment 26 eric.savary 2006-12-11 15:37:24 UTC
ES->TM: Please file child tasks for the testers of the correponding applications
(Writer, Calc, Draw...).
Comment 27 thorsten.martens 2006-12-12 08:21:22 UTC
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. 
Comment 28 thorsten.martens 2006-12-13 11:53:50 UTC
TM->ES: please verify your issue !
Comment 29 eric.savary 2006-12-18 15:23:56 UTC
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.
Comment 30 eric.savary 2007-02-01 14:53:33 UTC
Ok in OOF_m5