Key bugs: progress report
Edward K. Ream
I have spent several hours staring at the results of a unit test
before it dawned on me that the results were correct! It's that kind
of code...
Happily, the work has resulted in a proper unit test. So only the
unit test itself needs to change. This test is close to verifying that
everything works as expected with the new merging code.
The new test may have uncovered another key-related buglet, which can
probably be solved relatively easily. It would be easier to solve
with new key-related classes, but not yet.
Next, I'll ensure that the key bug Terry recently mentioned has been
stamped out...
I'll change the trunk only when all testing is complete. That will
probably happen in a day or three.
Yes, fixing this bug has been glacially slow. My tolerance for this
kind of extreme detail work is limited. If you are impatient, just
know that I am even more impatient :-)
Edward
Viktor Ransmayr
from
your current work on improving the key handling of Leo. - I'm sending
you this traceback in the hope that it helps ...
<log>
D:\Users\Viktor Ransmayr\Documents>leo ./BR-2012-01-28.leo
** isPython3: True
Leo 4.9.1 devel, build 4950, 2012-01-28 10:16:45
Python 3.2.2, qt version 4.8.0
Windows 6, 1, 7601, 2, Service Pack 1
reading settings in D:\Branches\leo-editor\leo\config\leoSettings.leo
reading settings in D:\Users\Viktor Ransmayr\.leo\myLeoSettings.leo
reading settings in D:\Users\Viktor Ransmayr\Documents
\BR-2012-01-28.leo
wrote recent file: D:\Users\Viktor Ransmayr\.leo\.leoRecentFiles.txt
<string>:179: (WARNING/2) Literal block expected; none found.
Traceback (most recent call last):
File "D:\Branches\leo-editor\leo\plugins\qtGui.py", line 8689, in
eventFilter
ret = k.masterKeyHandler(event)
File "D:\Branches\leo-editor\leo\core\leoKeys.py", line 2897, in
masterKeyHandler
done,val = k.doMode(event,state,stroke)
File "D:\Branches\leo-editor\leo\core\leoKeys.py", line 2973, in
doMode
val = k.callStateFunction(event) # Calls end-command.
File "D:\Branches\leo-editor\leo\core\leoKeys.py", line 2947, in
callStateFunction
val = k.state.handler(event)
File "D:\Branches\leo-editor\leo\core\leoKeys.py", line 2010, in
fullCommand
k.callAltXFunction(k.mb_event)
File "D:\Branches\leo-editor\leo\core\leoKeys.py", line 2056, in
callAltXFunction
func(event)
File "D:\Branches\leo-editor\leo\core\leoRst.py", line 472, in rst3
self.processTopTree(self.c.p)
File "D:\Branches\leo-editor\leo\core\leoRst.py", line 482, in
processTopTree
self.processTree(p,ext=None,toString=False,justOneFile=justOneFile)
File "D:\Branches\leo-editor\leo\core\leoRst.py", line 510, in
processTree
self.write_rst_tree(p,ext,fn,toString=toString,justOneFile=justOneFile)
File "D:\Branches\leo-editor\leo\core\leoRst.py", line 560, in
write_rst_tree
self.write_files(ext,fn,callDocutils,toString,writeIntermediateFile)
File "D:\Branches\leo-editor\leo\core\leoRst.py", line 1749, in
write_files
f.write(s)
File "C:\Python32\lib\encodings\cp1252.py", line 19, in encode
return codecs.charmap_encode(input,self.errors,encoding_table)[0]
UnicodeEncodeError: 'charmap' codec can't encode character '\u0142' in
position
19816: character maps to <undefined>
</log>
As you can see I have re-produced this with the latest revision of
leo-trunk. - If you want I can send a minimal outline that
consistently
produces this tracedback.
With kind regards,
Viktor
Edward K. Ream
<viktor....@gmail.com> wrote:
> I do not know if this helps you in this issue - or - distracts you from your current work on improving the key handling of Leo. - I'm sending you this traceback in the hope that it helps ...
> File "D:\Branches\leo-editor\leo\core\leoRst.py", line 1749, in
> write_files
> f.write(s)
> File "C:\Python32\lib\encodings\cp1252.py", line 19, in encode
> return codecs.charmap_encode(input,self.errors,encoding_table)[0]
> UnicodeEncodeError: 'charmap' codec can't encode character '\u0142' in
> position
> 19816: character maps to <undefined>
Thanks for this report. This is a "garden variety" unicode error,
having nothing to do with Leo's key handling.
Edward
Edward K. Ream
>> Another set of unit tests is needed to verify that expected overrides happen.
At last, some real unit tests are complete. I'll be pushing to the trunk soon.
This should fix bug 879331: Redefining a key binding breaks menu items
with same binding
I have spent the last hour or so trying to understand if
k.registerCommand might cause problems when overriding a shortcut. I
can't say for sure either way. This question will be much easier to
answer with better key-related classes. In any event, any bug in this
area is much less serious than 879331.
> The new test may have uncovered another key-related buglet.
I don't think there is a problem here, at present. This kind of
hash-related bug should disappear when better key-related classes are
in place.
> Next, I'll ensure that the key bug Terry recently mentioned has been stamped out...
This bug relates to settings, but not 879331.
Edward
Edward K. Ream
On Jan 28, 11:49 am, "Edward K. Ream" <edream...@gmail.com> wrote:
> At last, some real unit tests are complete. I'll be pushing to the trunk soon.
>
> This should fix bug 879331: Redefining a key binding breaks menu items
> with same binding
is still a problem with menu accelerators.
I just ran the following hand test. In ekr.leo I bound print-settings
= ctrl-n. Within ekr.leo, ctrl-n is indeed bound properly to print-
settings. More importantly, selecting the File:New command does work
as expected--redefining ctrl-n doesn't break the menu's operation.
However, the File:New menu item still shows Ctrl+N as the accelerator.
I'll attempt a fix today...
Btw, a simple change now shows *all* bindings for command in each menu
item. I think this is always a good idea, but it can be easily
reverted if people have objections...
Edward
Terry Brown
"Edward K. Ream" <edre...@gmail.com> wrote:
> Btw, a simple change now shows *all* bindings for command in each menu
> item. I think this is always a good idea, but it can be easily
> reverted if people have objections...
Is there a way, which has perhaps existed for years ;-), to see the
binding for a command in the mini-buffer?
Oops, I see auto complete in the mini-buffer fills that role, e.g. to
see binding for fill-paragraph just enter 'Alt-X fill-par<tab>'
Cheers -Terry
Edward K. Ream
>> Next, I'll ensure that the key bug Terry recently mentioned has been stamped out...
>
> This bug relates to settings, but not 879331.
I think I see what is causing problems with settings that disappear
depending on exactly how the .leo files are described.
Some of the configuration code uses c.hash() to associate settings
with commanders. It's bad code, but we are stuck with it But c.hash
probably doesn't work for all descriptions.
Terry, you might try messing with c.hash to see if changing it makes a
difference. In particular, you might trace the result returned in the
various scenarios that have been giving you troubles.
The new key code still doesn't work as I would like. The problem is
that settings are read *twice* for local files, and it's not so easy
to associate values read in the first pass with the values read in the
second pass. In other words, c.config is different in the two passes
because c is different.
These kinds of bizarre considerations lead me to c.hash...
Extreme patience is required for this work. I have added some
important comments to methods like get and getShortcutHelper that
indicate what the various (badly-named) dictionaries and lists come
from and how they are used. So now I understand the code about as well
as it *can* be understood :-)
Looking back on the code, I can see that it arose as the result of 30+
years of procedure-oriented C coding. Theoretically, refactoring into
new classes would help, but that's way too risky to do in one bite.
So I'm stuck with making smallish changes for now...
Edward
Terry Brown
"Edward K. Ream" <edre...@gmail.com> wrote:
> Terry, you might try messing with c.hash to see if changing it makes a
> difference. In particular, you might trace the result returned in the
> various scenarios that have been giving you troubles.
cd /home/tbrown/Package/leo/bzr/leo.repo/trunk
python launchLeo.py ~/.leo/del.leo
@settings in del.leo ignored, c.hash() = "/home/tbrown/.leo/del.leo"
cd /home/tbrown/.leo
python /home/tbrown/Package/leo/bzr/leo.repo/trunk/launchLeo.py del.leo
@settings in del.leo honored, c.hash() = "/mnt/usr1/usr1/home/tbrown/.leo/del.leo"
So, I can see why Leo uses the path to the Leo file to identify its
settings, but at the same time @setting in a particular file should be
honored.
The problem is that os.getcwd() and friends use the POSIX C getcwd()
function (whatever it's called), and POSIX says return the absolute
path.
os.path.abspath(path) === os.path.normpath(join(os.getcwd(), path))
Where to fix it? I'd prefer it use /home/tbrown so settings follow me
around more, at home /home/tbrown may be /mnt/usr1/usr1/home/tbrown,
whereas at work /home/tbrown may be /media/raid2TB/home/tbrown.
So, c.hash() could operate on os.path.abspath(path), simplest, and
probably fixes the ignored @settings problem.
Or c.hash() could be calculated from
os.path.normpath(join(os.getenv('PWD'), path))
At least it explains what's going on.
Cheers -Terry
Edward K. Ream
>> Terry, you might try messing with c.hash to see if changing it makes a
>> difference. In particular, you might trace the result returned in the
>> various scenarios that have been giving you troubles.
>
> cd /home/tbrown/Package/leo/bzr/leo.repo/trunk
> python launchLeo.py ~/.leo/del.leo
>
> @settings in del.leo ignored, c.hash() = "/home/tbrown/.leo/del.leo"
>
> cd /home/tbrown/.leo
> python /home/tbrown/Package/leo/bzr/leo.repo/trunk/launchLeo.py del.leo
>
> @settings in del.leo honored, c.hash() = "/mnt/usr1/usr1/home/tbrown/.leo/del.leo"
Thanks for this, and the rest of your post. Now that you have
confirmed the source of the problem, I'll see if there might not be a
way of associating settings with files that can avoid these
path-related issues entirely.
Edward
Terry Brown
"Edward K. Ream" <edre...@gmail.com> wrote:
> Thanks for this, and the rest of your post. Now that you have
> confirmed the source of the problem, I'll see if there might not be a
> way of associating settings with files that can avoid these
> path-related issues entirely.
That would be good. id(c) might work? Docs. below. Or maybe, to
avoid the (probably miniscule) risk of a c with the same id as a
previously existing c, self.id = id(self)+int(time.time())
c.db uses a hash of the path, but that seems more unavoidable, given
that it's persistent, anyway that's not related to @settings.
Cheers -Terry
id(object)
Return the “identity” of an object. This is an integer (or long
integer) which is guaranteed to be unique and constant for this
object during its lifetime. Two objects with non-overlapping
lifetimes may have the same id() value.
CPython implementation detail: This is the address of the object in
memory.
Edward K. Ream
> At last, some real unit tests are complete. I'll be pushing to the trunk soon.
Work on this project has entered a high-energy phase. There has been
an important collapse in complexity, and other opportunities for
simplification and improvement have appeared.
The ShortcutInfo class has been a spectacular success. Canonicalizing
settings in that class's ctor ensures some important code invariants.
Canonicalizing settings in the ctor appears to have created a buglet
or two. That's actually *good* news: it suggests an important new
invariant: namely that canonicalizing an already-canonicalized
shortcut should be a no-op. That is, for any valid setting S, we
should have:
shortcutFromSetting(S) == shortcutFromSetting(shortcutFromSetting(S))
This constraint probably fails at present for some small number of
settings S. Ensuring the constraint is valid for all S will not only
fix the buglet, but will make Leo's core much more robust. A unit
test enforcing this invariant will be straightforward, and may uncover
some bugs in what is in fact extremely picky code.
Today or tomorrow I *will* push what I have to the trunk. Slight
problems may arise, but that can't be helped: all the present unit
tests work.
After the first push, I'll create a ShortcutsDict class to encapsulate
the "raw" dicts that describe shortcuts. This will eliminate the
wretched _hash hack. Also, the ShortcutsDict and ShortcutInfo classes
will be the natural home to many of the key-related methods that now
clutter leoKeys.py. For example, I deliberately did not specify what
class will ultimately contain shortcutFromSetting. There are some
tricky issues involved, but there is no need to go into details here.
Edward
Edward K. Ream
> Canonicalizing settings in the ctor appears to have created a buglet or two.
The fix was to properly init a new dict.
> it suggests an important new invariant: namely that canonicalizing an already-canonicalized shortcut should be a no-op.
Nice try, Edward. This would be a nice invariant, but it's not going
to happen any time soon. Canonicalized settings were not designed
with this invariant in mind. For example, shortcutFromSetting
translates Shift-a to A, and A to a. Oops.
Yes, it would be possible to change this *convention*, but that would
instantly have ripple effects everywhere. It probably will never
happen. In the meantime, the code must take care to canonicalize
settings in exactly the right places.
With the buglet fixed and the with no hope of making
shortcutFromSetting safer, it looks like time to push the code. I
plan to do this later tonight, after using the code myself for a bit
longer.
Edward
Edward K. Ream
> That would be good. id(c) might work?
worst of all solutions. Imagine the time wasted if somebody reports a
bug do to an id clash.
I'm not sure whether adding time.time() to a hash will make things
better or worse. Let me think about what problem the hash is *really*
attempting to solve. That may suggest a way forward.
Edward
Edward K. Ream
On Jan 29, 5:40 pm, "Edward K. Ream" <edream...@gmail.com> wrote:
> > it suggests an important new invariant: namely that canonicalizing an already-canonicalized shortcut should be a no-op.
>
> Nice try, Edward. This would be a nice invariant, but it's not going
> to happen any time soon. Canonicalized settings were not designed
> with this invariant in mind. For example, shortcutFromSetting
> translates Shift-a to A, and A to a. Oops.
- A global constant will switch between the old and new versions of
the code.
- A unit test will ensure the invariant.
- It will probably become immediately obvious if the new convention
causes problems.
I'll probably do this in the next day or so, while the issues are
fresh in my mind.
Edward
Edward K. Ream
>> > it suggests an important new invariant: namely that canonicalizing an already-canonicalized shortcut should be a no-op.
> On second thought, this should actually be fairly easy to do safely.
This invariant is important because it is the doorway towards another
collapse in complexity. The important thing is that internally almost
everything uses the strokes created by shortcutFromSetting. It
doesn't much matter what convention is used--what matters is that all
the code knows that strokes *are* strokes.
This convention will have several important consequences:
1. I have just discovered that the very strange k.tkbindingFromStroke
is not needed. That is, everything works if k.tkbindingFromStroke is
a do-nothing. That's not too surprising, given that Tk is long gone,
but it's good to know. I'll remove all traces of this beast after
some more testing...
2. Having strokes, and *only* strokes, be the result of the Qt
key-input methods should clarify the code considerably. That's good,
because at present the code is a horror show.
Edward
Edward K. Ream
> it suggests an important new invariant: namely that canonicalizing an already-canonicalized shortcut should be a no-op.
wrapper around a string.
Despite its miniscule size, this could be an important class: it
announces that its contents have been canonicalized once and *only*
once.
With this class in place, the code can now easily distinguish between
"raw" key-related settings strings and canonicalized KeyStroke
objects. This will drastically reduce the importance of the (missing)
identity::
shortcutFromSetting(S) ==
shortcutFromSetting(shortcutFromSetting(S))
The inability to distinguish canonicalized settings strings from raw
settings strings made the code *much* more difficult to understand
that it needed to be. Worse, the code was brittle: subtle
misunderstandings about the contents (history) of strings translated
directly into hard-to-find bugs.
This is yet another example of the importance of types in Python
programs. Not all strings are, in fact, equivalent! In fact,
changing the *contents* of a string can, in effect, change its type.
The new KeyStroke class will make the type transformation explicit, as
it should have been all along.
Edward
Edward K. Ream
> I'm not sure whether adding time.time() to a hash will make things
> better or worse. Let me think about what problem the hash is *really*
> attempting to solve. That may suggest a way forward.
I have spent several pleasant hours researching the situation, mostly
using the clone-find-all command.
As expected, c.hash() is used *only* to create a "weak" link between a
commander C created during a the prepass of a local file and *another*
commander C2 created *later* during the second and final read of the
local file.
Happily, there is no need for a weak link: it is possible to create a
"strong" link to the existing commander (or c.config). The code is
kludgy: g.app.config.updateSettings can set
self.copied_local_options_dict, and the c.config ctor called later can
copy g.app.config.copied_local_options_dict into its own ivar.
Actually, things are a bit more complicated than that, because all the
methods are really g.app.config methods rather than c.config methods,
but the idea is the same.
In short, there is no need for c.hash: it is possible, though not
pretty, to remember the dicts created during the pre-load of a local
file until such time as the permanent c.config object is created.
I'll be committing the code tomorrow, after I clean it up and test it further.
Edward
Edward K. Ream
> As expected, c.hash() is used *only* to create a "weak" link between a commander C created during a the prepass of a local file and *another* commander C2 created *later* during the second and final read of the local file.
> Happily, there is no need for a weak link: it is possible to create a "strong" link to the existing commander (or c.config).
namely that Leo reads each "local" (non-setting) .leo file twice.
First, Leo reads *all* the local .leo files to discover their
settings. Next, Leo rereads the local .leo files "for real". For
example, if I invoke Leo to read a.leo, b.leo and c.leo, Leo will read
the following files, in this order::
leoSettings.leo
myLeoSettings.leo
a.leo (pass 1)
b.leo (pass 1)
c.leo (pass 1)
a.leo (pass 2)
b.leo (pass 2)
c.leo (pass 2)
Leo reads local .leo files twice in order to determine which settings
to use *during the second read*. In particular, Leo reads settings in
order to determine which *plugins* to enable for each file.
The Problem
-----------------
During pass 2 Leo must somehow get links to the commanders (and
c.config objects) created during pass 1.
c.hash() is the way that Leo does this. For each commander c created
in pass 1, g.app.config localOptionsDict.get(c.hash()) contains a
settings dict. g.app.config.get uses this dict to get the proper
dicts to search.
Shortcuts are handled separately, and almost certainly incorrectly,
which is probably why Viktor is having problems. Indeed, at present
Leo remembers only a *single* shortcuts dict. Depending on which
shortcuts are defined where, this could cause the vast majority of
shortcuts to disappear. Sorry about that, Viktor.
Possible solutions
------------------------
1. The quickest solution will be to enhance c.hash() so that it always
delivers a value sufficient to uniquely identify all loaded .leo
files. As Terry suggests, having c.hash() return an absolute path
will probably suffice. As implied above, a similar solution must be
used so that all shortcuts dictionaries are handled properly.
2. Another possible solution would be to read the a.leo, b.leo and
c.leo files only once. This might simplify the code, and speed up
Leo, but it would have the possibly serious drawback of redefining
what options are in effect while a .leo file is loading. This seems
like a risky approach. I strongly suspect the Kent would point out
problems, and such problems might require a reversion of the code.
Conclusions
----------------
First, Leo must, once again, correctly associate shortcuts
dictionaries with each .leo file. The only quick way to do this will
be to use c.hash() to access shortcuts dicts just like for the rest of
Leo's options. BTW, I'll want to ensure that the value of c.hash()
remains constant for each .leo file. I'm not even sure that c.hash()
is always the key into localOptionsDict.
Once Viktor can use the trunk again I'll start looking for other
solutions.
Edward
Viktor Ransmayr
On 31 Jan., 20:43, "Edward K. Ream" <edream...@gmail.com> wrote:
> ...
> Once Viktor can use the trunk again I'll start looking for other
> solutions.
waiting on your advice on what you want me to execute/ test in my
environment ...
With kind regards,
Viktor
Viktor Ransmayr
On 31 Jan., 20:43, "Edward K. Ream" <edream...@gmail.com> wrote:
> ...
> solutions.
thread. - Let
me know what you want me to 'exercise' in my environment ...
With kind regards,
Viktor
Brian Theado
On Sun, Jan 29, 2012 at 1:44 PM, Terry Brown <terry_...@yahoo.com> wrote:
> On Sun, 29 Jan 2012 04:57:25 -0600
> cd /home/tbrown/Package/leo/bzr/leo.repo/trunk
> python launchLeo.py ~/.leo/del.leo
>
> @settings in del.leo ignored, c.hash() = "/home/tbrown/.leo/del.leo"
>
> cd /home/tbrown/.leo
> python /home/tbrown/Package/leo/bzr/leo.repo/trunk/launchLeo.py del.leo
>
> @settings in del.leo honored, c.hash() = "/mnt/usr1/usr1/home/tbrown/.leo/del.leo"
>
> So, I can see why Leo uses the path to the Leo file to identify its
> settings, but at the same time @setting in a particular file should be
> honored.
>
> The problem is that os.getcwd() and friends use the POSIX C getcwd()
> function (whatever it's called), and POSIX says return the absolute
> path.
> os.path.abspath(path) === os.path.normpath(join(os.getcwd(), path))
>
> Where to fix it? I'd prefer it use /home/tbrown so settings follow me
> around more, at home /home/tbrown may be /mnt/usr1/usr1/home/tbrown,
> whereas at work /home/tbrown may be /media/raid2TB/home/tbrown.
>
> So, c.hash() could operate on os.path.abspath(path), simplest, and
> probably fixes the ignored @settings problem.
>
> Or c.hash() could be calculated from
> os.path.normpath(join(os.getenv('PWD'), path))
>
> http://stackoverflow.com/questions/1542803/is-there-a-version-of-os-getcwd-that-doesnt-dereference-symlinks
>
> At least it explains what's going on.
Thanks for sharing these details. I encountered the same issue. I
hadn't realized I was using a path with a symlink in it until your
email. Now I know to use the workaround of specifying a non-symlinked
path on the leo command-line. Before I had found I could just revert
the file and get the settings to take effect. This new workaround
saves me a step of having to revert as soon as I open the file.
Brian
HansBKK
I encountered the same issue. I hadn't realized I was using a path with a symlink in it until your email.