Hi there,
[Tim wrote] It took longer to write this patch than I expected. You will find it in the attachment but I do not recommend using it before somebody else has verified it (I am new to C/C++).
It's great to see this much needed feature in the process of making its way into the source tree! :) After cosidering the raised issues, I believe it'd be good to make our next release (2.1), with what we have in the repo (plus bug fixes) and to leave the new stuff for 2.2. That allows time to polish and test. Here go my comments.
[Corvid wrote] - I haven't looked at this in any detail yet -- this is just my paging-through-the-patch response -- but I notice a_Nav_push_forced in there. Patches should be kept separate. The smaller they are, the better for review.
- Lines should not be longer than 79 chars.
Fully agreed.
[Corvid wrote] I lean toward not adding features to this patch yet.
(Continuing to add features before getting anything into the tree caused dillo not to get frames support, and that was an unfortunate incident all around.)
Very good advice. I'd like to have the bare functionality merged first, and to improve on that basis when necessary. BTW, if Corvid agrees, I'd like him to be your adviser on preparing this patch until he sends me something for an inclusion review.
[Tim wrote] I had to re-implement the parsing routine. This is mainly because a new line format was introduced which allows you to configure key bindings in your dillorc. Otherwise we would have needed unnecessarily a second file. This is the only thing that changes for the user. The new format looks like this: "set option value"
[Tim wrote] The downside of this patch is that currently there is an issue with the line parser, which claims to find syntax errors even though the lines look alright for me. This needs to be fixed before this patch can go into mainline.
[Diego wrote] I disagree about changing the dillorc format:
- This change is not backwards compatible. Do not read old dillorc format and it is a big inconvenience for users.
[Diego wrote] - There are compatible ways to add this preferences (key_save = "...", key_find = "CTRL + f", ...) [Tim wrote] Sure, but options were not initially thought of as being used for key bindings. When using a special keyword which is only valid for one preferences type (bindings), it is possible to add further parameters such as groups (useful when having lots of key bindings). The other advantage of this implementation is that we could easily modify it to allow multiple key bindings for one symbol.
[Diego wrote] - If the number of key bindings increase much another config file can be convenient. With another config file some example files can be suplied (vim keys, firefox keys, ...) and end user only need to copy the prefered file. [Corvid wrote] That's a good point.
This is an important issue. After giving it some tought, it looks like we can handle it like icewm and others (i.e. with multiple files for different configuration areas). BTW, this is already the case with style.css (CSS configuration file) and cookiesrc. We'd have: dillorc, style.css, keyboardrc and cookiesrc. In the future we may add others (e.g. addblock, read.css). This allows for backwards compatibility, easy separation of tasks, isolation/flexibility of configuration parsers and simple example configuration files for the user. BTW, dillorc has grown a long file to read, and will most probably end being managed by an external app.
[Corvid wrote] - I'm sure that Jorge isn't going to want exception handling.
Good guess!
[Tim wrote] Why not? Exceptions are more flexible when it comes to longer and more complicated functions. Of course, we could also return an integer status code but sometimes it is necessary to return non-integer values.
We're using return codes and for the non-integer return value case we use a pointer argument.
[Johannes wrote] I'm definately against adding exceptions. If we would start a new project, we could discuss pros and cons of using exceptions - I would probabely still vote against using them in that case... But as all current dillo code and also fltk does not use exceptions it would get a real mess if parts of new code would start to use exceptions for error handling while other code is using return values.
Right. Please modify the patch to use return codes. -- Cheers Jorge.-