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++). 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" For backward compatibility you can also use an equal sign before the value: "set option=value" so that you only have to put a ``set'' to the beginning of each line in your old dillorc. But more interesting is the support for configurable keys. The corresponding keyword for bindings is called ``bind''. The second parameter of a binding line represents the key combination and the third one its action. Thus, to allow reloading with a single `r' (instead of Ctrl-r), you can simply write: "bind r reload" But you can also do more complex things like setting modifiers (Ctrl, Alt, Left mouse click, etc.). A line mapping the backward action to Ctrl-p looks like this: "bind <Ctrl>p backward" There are some limitations, though: Unfortunately, FLTK does not enable us to combine modifiers in order to set even more key combinations. Because FLTK is C++-only and we need one of its header files, I had to realize this part of the parser in C++, as well. The line parser is written in C because there are too many functions which access an array in which it puts the parsed preferences. We should port everything to C++ as this would help us keeping the code clean. Note that not only alpha-numeric characters but also ``special characters'' such as "Backspace", "Enter", "Tab" and a few others are supported. To get a full list, please refer to src/keys.cc which also contains all modifiers and the default key bindings. I have not figured out yet how to implement scrolling hooks. Hence, it would be nice if someone could implement the symbols "scroll-top", "scroll-bottom", "scroll-left", "scroll-right", "scroll-up" and "scroll-down" as I want to use VIM-like key bindings (h, j, k, l for movement) in Dillo. 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. --Tim

- 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.

Yes, it contains my other three patches partially because they were related.

- Lines should not be longer than 79 chars.

Okay, I configured VIM appropriately.

- I'm sure that Jorge isn't going to want exception handling.

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.

PS Are you sure that fltk doesn't allow modifiers to be combined? Grepping through fltk/test, it looks like they're combining them in menubar.cxx.

It looks like this is possible indeed. However, I used event_state(SHIFT & CTRL) to check whether it recognizes a combined Shift and Ctrl but this did not work for me. Any ideas? --Tim

looking in events.h, I think (event_state() & (SHIFT | CTRL)) would work.

Good idea. It does not work as expected though. But this does: ((event_state() & (SHIFT | CTRL)) == (SHIFT + CTRL)) Now that we know how to check for multiple modifiers, does somebody want to implement it? --Tim

Hi Tim, thanks for working on the keybinding feature! On Mon, Feb 23, 2009 at 08:17:13PM +0000, corvid wrote:

Wow that was quick!

- I personally am open to the idea of changing the dillorc format for existing options, but we'll see what the consensus is.

The proposed format looks like the muttrc format. I would not change dillorc to yet another arbitrary format, but if the muttrc style is commonly used - why not. How does vimrc compare to that btw? Would it be possible to make it backward compatible by interpreting [_a-Z0-9]+\s?= the same as if there was a "set " in front?

- 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.

- I'm sure that Jorge isn't going to want exception handling.

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. Cheers, Johannes

I'll add something about that last point to the website, since it's not mentioned currently.

PS Are you sure that fltk doesn't allow modifiers to be combined? Grepping through fltk/test, it looks like they're combining them in menubar.cxx.

_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

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. - Can delete some meta info stored in dillorc. - There are compatible ways to add this preferences (key_save = "...", key_find = "CTRL + f", ...) - 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. I do not known what other option is best, but i think that this format change is not needed. Diego. 2009/2/24, corvid <corvid@lavabit.com>:

I think I'd feel better if it could be used like prefs, that is, isPressed testing bindings.reload instead of looking for the binding associated with the "reload" string.

_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

- This change is not backwards compatible. Do not read old dillorc format and it is a big inconvenience for users.

No, the new format is not backward compatible but it supports the equal signs which were used in the old format for associations. Dillo is mostly used by experienced Linux users who should not have any problems adding a simple "set " before each line. Backward compatibility should never be used as an argument against enhancements. Otherwise programs would just become larger and larger. They carry various compatibility layers without any real benefits. The more compatibility code an application contains, the more difficult it is to maintain it.

- Can delete some meta info stored in dillorc.

Do you mean that the migration ``tool'' may delete comments? The dillorc is not migrated automatically (even though writing a small scrip would not be too difficult). As the user has to do this manually, it is one's choice whether to keep the comments or not.

- There are compatible ways to add this preferences (key_save = "...", key_find = "CTRL + f", ...)

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.

- 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.

The idea of having several ``preset'' files for key bindings is well. In my opinion, we should give the user the right to decide whether to put everything into one file or to separate the configuration into multiple files. Therefore an "include" command for the dillorc should be implemented. Then we could put something like "include /usr/share/dillo/bindings/default" there, as well as some others. Now, if somebody wants to try different key bindings, you would just have to uncomment this particular line. There is no need for replacing your local bindings just to try others out. --Tim

I think I'd feel better if it could be used like prefs, that is, isPressed testing bindings.reload instead of looking for the binding associated with the "reload" string.

Personally, I also like this concept better as it reduces the risk of misspellings (the compiler would throw an error). Furthermore, the code looks much cleaner and it is likely that it become a bit faster (because seeking the "symbols" array is not be necessary anymore). But on the other hand there needs to be a large mapping table that contains all symbol names twice for each entry (like it is done in src/prefs.c). This was the main reason why I decided against a "bindings" array because I simply wanted to avoid duplicate code. To find balance between these two approaches, I would suggest altering isPressed() requiring only the position of the symbol in the "symbols" array. Then we could create a constant for each binding containing that ID. It works similar to prefs' concept except that we only map integers. Hence, when adding new entries, we have to repeat the symbol name less frequent. The only problem with this approach is that it easily breaks when the items are accidentally re-ordered. --Tim

enum { KEYCMD_RELOAD, ... };

No, I thought of using constants: #define KEYCMD_RELOAD (0) #define KEYCMD_OPEN (1) ...

commands[] = { {"reload", KEYCMD_RELOAD}, ... };

There are no mapping tables in my approach. commands[] is the same as symbols[] (like it is called in src/keys.cc) and contains all actions as well as their default key bindings. commands[] = { { "reload", { fltk::CTRL, 'r' } }, { "open" , { fltk::CTRL, 'o' } }, ... };

isPressed(&bindings[KEYCMD_RELOAD])

The isPressed()-call should be as short as possible. I would prefer "isPressed(KEYCMD_RELOAD);". --Tim

F10 through F20?

You are right. I seem to have forgotten them. FLTK only supports F1 through F12 though. But implementing further function keys should not be that difficult. Does your keyboard have 20 function keys?

Keypad keys?

You mean the arrows while having "Num Lock" disabled?

What about Meta, Hyper, Super, AltGr, and Modeshift? Or, more generally, Mod1 through Mod5?

FLTK does not support all of them. I only added those modifiers I thought to be most important. fltk/events.h contains a list of all supported modifiers. Please tell me which ones you want me to add.

BTW, I know it was like this before too, but something to think about for later is that this section could probably be replaced by a table instead of a bunch of if/else statements.

Great idea but how do we realize it best?

And, if you want to have an easier time keeping the formatting consistent, try this in your vimrc:

" show tabs and trailing spaces set listchars=tab:>-,trail:- set list

Thanks for the hint. I have added it to my vimrc.

It's a minor thing really, but I noticed some inconsistent whitespace, and those config lines make it really easy to spot. :)

Okay, I try to pay attention on it the next time. --Tim

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.-

On Mon, Mar 09, 2009 at 04:54:57PM +0100, Tim Nieradzik wrote:

[...] PS: I would like to port some C files to C++ in order to achieve more consistency. What is your opinion on this?

Please don't. C++ is an unfortunate compromise we had to make to be able to use FLTK2. It has increased complexity, reduced portability and we'd like to confine it to a minimum (in size and features).

Submitting a patch for each changed file to the list might be a bit inconvenient.

That's not the idea. One changeset (which can touch several files) per email is the standard.

Therefore, I am considering creating a repository on FreeHg.org which only deals with refactoring code.

Separate repos. are suggested for major changes (as the CSS branch). -- Cheers Jorge.-

Hi Tim, On Mon, Mar 09, 2009 at 04:54:57PM +0100, Tim Nieradzik wrote:

Here is an updated version of the patch. It aims to be fully backward compatible. You do not need to change anything in your dillorc (put the key bindings into ~/.dillo/keysrc).

Good.

Changes: - Ported src/dir\.[ch] to C++ (now called src/paths\.(cc|hh)). - Split src/prefs.c (parser is now in src/prefsparser.c). - Changed method names in src/dillo.cc to use camel case. - Added meta-modifier and function keys F10-F12. - Fixed some inconsistent white spaces. - Shortened some license headers. - Key bindings are to be defined in keysrc (previously dillorc). - Minor code cleanups such as using "TODO:" instead of "\todo". - Removed a_Prefs_freeall because it was not used anywhere.

After giving a couple of read-passes to the patch, and considering that we still have some time before April (when I'll have time to focus on the release), we can merge this long patch in small independent chunks. Please separate them into different patch files. For instance: Simple ones: - Changed method names in src/dillo.cc to use camel case. - Fixed some inconsistent white spaces. - Shortened some license headers. - Minor code cleanups such as using "TODO:" instead of "\todo". - Removed a_Prefs_freeall because it was not used anywhere. Refactors: - Split src/prefs.c (parser is now in src/prefsparser.c). Core of it: - Key bindings are to be defined in keysrc (previously dillorc). - Added meta-modifier and function keys F10-F12.

PS: I would like to port some C files to C++ in order to achieve more consistency. What is your opinion on this?

As explained before. Not unless we're forced or there's a compelling reason. For instance I'd decline this: - Ported src/dir\.[ch] to C++ (now called src/paths\.(cc|hh)). (unless there's a strong reason I've not seen). -- Cheers Jorge.-

On Wed, Mar 25, 2009 at 04:20:19PM +0000, corvid wrote:

Jorge wrote:

...

- Removed a_Prefs_freeall because it was not used anywhere.

Should we rip out all of the *_freeall()s?

Ooops! It slipped. The idea of those functions is to free memory there. They don't harm when empty. Let's keep them until something better springs up. -- Cheers Jorge.-

On Wed, Mar 25, 2009 at 04:46:33PM +0000, corvid wrote:

Jorge wrote:

...

On Wed, Mar 25, 2009 at 04:20:19PM +0000, corvid wrote:

...

Jorge wrote:

...

- Removed a_Prefs_freeall because it was not used anywhere.

Should we rip out all of the *_freeall()s?

Ooops! It slipped.

The idea of those functions is to free memory there. They don't harm when empty. Let's keep them until something better springs up.

Should we add calls to them like in dillo1 and get them working, then?

Yes, that'd be good. Note that those functions are called at exit time. They were added long ago to help check for memory leaks. BTW, it'd be good to have some memory usage data for dillo-2.1. Something that can be put in a web page: e.g. a table with some explanations, on how dillo-2.1 compares with dillo-0.8.6 and with a big browser. IIRC sometime ago we made tests with a 13MB page, and the notion of dillo2 having near half the memory footprint of dillo1 came from there. We need to show some numbers and how we got them. Note: I remmeber having an awful memory footprint on 64bit Ubuntu. It didn't happen with 32bit. What's the current status, I don't know. -- Cheers Jorge.-

Hi, On Fri, Mar 27, 2009 at 02:05:23PM -0600, Scott Scriven wrote:

* Jorge Arellano Cid <jcid@dillo.org> wrote:

...

BTW, it'd be good to have some memory usage data for dillo-2.1.

A while ago, I noticed that dillo was using a lot of memory on a specific page, and I didn't remember it doing that before. So I tested a few versions and found that dillo2 uses more memory initially, but less memory for large tables. Specifically:

Memory use (in KiB) (on ubuntu 7.10 x86_64) dillo-0.8.6/src/dillo: 3692 file:///nothing 4900 http://dillo.org/ 45204 http://idlerpg.net/db.php dillo-2.0/src/dillo: 4700 file:///nothing 6564 http://dillo.org/ 36408 http://idlerpg.net/db.php hg/dillo/src/dillo: 4812 file:///nothing 6684 http://dillo.org/ 36756 http://idlerpg.net/db.php

...

Note: I remmeber having an awful memory footprint on 64bit Ubuntu. It didn't happen with 32bit. What's the current status, I don't know.

I have both 64-bit and 32-bit ubuntu systems here, if you need measurements. For example, I tried the above on my 32-bit box:

Memory use (in KiB) (on ubuntu 8.04 i686) dillo-0.8.6/src/dillo: 4916 file:///nothing 6504 http://dillo.org/ 36824 http://idlerpg.net/db.php dillo-2.0/src/dillo: 3972 file:///nothing 5732 http://dillo.org/ 25144 http://idlerpg.net/db.php hg/dillo/src/dillo: 4036 file:///nothing 5760 http://dillo.org/ 25296 http://idlerpg.net/db.php

Yes, 64bit uses more memory. I've collected the following numbers: (Ubuntu 8.10, 32bit) dillo-2.1: VSZ RSS COMMAND 8148 3872 ./dillo (empty) 26936 19196 ./dillo DragonFly.html (1.1MB HTML) 178684 174620 ./dillo MySQL-6.0.html (13MB HTML) 119788 113096 ./dillo MySQL-6.0.html (no IMG) f=7.2 dillo-0.8.6 VSZ RSS COMMAND 16592 3960 ./d086 83784 35748 ./d086 DragonFly.html (1.1MB HTML) 357320 293796 ./d086 MySQL-6.0.html (13MB HTML) 297224 234344 ./d086 MySQL-6.0.html (no IMG) f=16.3 firefox-3.0.8 VSZ RSS COMMAND 100884 37572 ./firefox 123184 59688 ./firefox DragonFly.html (1.1MB HTML) 312652 241016 ./firefox MySQL-6.0.html (13MB HTML) 328352 237560 ./firefox MySQL-6.0.html (no IMG) f=15 The 13MB page can be found at: http://dillo.org/test/mysql-6.0.doc.tar.bz2 The 1.1MB page can be found at: http://dillo.org/test/DragonFly.html.bz2 This numbers are the basis to claim that dillo2 uses half the memory footprint of dillo1. The "f" factor is the RSS for the MySQL manual (with no images), minus the empty RSS, minus 13MB, all divided by 13MB. The memory-cached page is substracted to get a clean HTML to Widgets factor. This is, if you have a 130KB html page, f*130KB will be used for rendering. It'd be good to have some wall-clock numbers too. Then the data can be tabulated and shown in the web site. Suggestions are highly welcomed. The VSZ and RSS numbers come from ps. I don't know of a better way to measure memory usage. The general idea is to have reproduceable numbers that show how dillo differs in memory and speed with other browsers. -- Cheers Jorge.-

After giving a couple of read-passes to the patch, and considering that we still have some time before April (when I'll have time to focus on the release), we can merge this long patch in small independent chunks. Please separate them into different patch files. [...]

I am definitely in favour of your idea but if I recall correctly many changes depend at each other. Therefore it could become difficult splitting it into separate patches. If I have a spare minute the next few days, I will try to do it anyway but from my point of view the easiest way would be to merge the patch as-is (or at least part of it). For the next patches, I know how to do it better.

[...] For instance I'd decline this:

- Ported src/dir\.[ch] to C++ (now called src/paths\.(cc|hh)).

(unless there's a strong reason I've not seen).

It was ported since OO method calls in C++ are superior to C functions when it comes to readability (a_Dir_init() vs Paths::init(), a_Dir_get_owd() vs Paths::getOldWorkingDir()). The reason why I renamed the file was that the functions returned a path instead of only the directory. I also moved some file name constants to paths.hh because it makes sense organizing such path-related things within one file and to use an unified prefix (in this case PREFS_RC_). Anyway, I fully understand your view on this topic as my decision to use C++ is mostly preference- and aesthetics-driven. --Tim

This patch cleans up src/prefs.c and introduces the PrefsParser (src/prefsparser.cc). Furthermore, src/dir.c is now C++-based as in C this declaration would have been problematic: bool Paths::getPrefs(const char* rcFile, FILE* &fp) --Tim

What's the main point of dParser_parse_rc_line()?

The main difference to the current implementation is that equal signs are interpreted as separator. Besides, it requires that ``line'', ``name'' and ``value'' are initialised with a specific length (in dpid/dpid.c it is set to 1024). The major advantage is that corrupt files (with very long lines) would not congest the RAM since we abort after 1024 characters. This is very unlikely though.

The comment says it extracts type, name and value, but it doesn't extract type.

Sorry, I had forgotten to update the description. In a previous version of my patch there has been a ``type'' attribute. As proposed, I removed it later on. Since then this function does basically the same job as dParser_get_rc_pair(). --Tim

The patch has a strange comment WRT equal signs:

+ // parse the value + i = 0; + while (*p) { + // skip all spaces and equal signs + if (!isspace(*p)) { + value[i++] = *p; + } + + p++; + } + + value[i] = '\0';

So either the implementation or the comment is wrong...

The comment is wrong, I am sorry. If you look a few lines above, you will see that we have already checked whether the current character is an equal sign. In a previous implementation I used to have an additional check there but removed it later as we do not want to skip any equal signs within values. Equal signs are very common in search URLs so skipping them could have led to many problems. I guess I just forgot to adjust the comment accordingly after having changed this part.

If the required job is basically the same, I can keep the current function (maybe returning the new termination codes) and continue integrating the patchset.

Yes, that is probably best. --Tim

This patch adds support for configurable key bindings. --Tim

Jorge wrote:

A new, rewritten version of the patch was committed.

Hurray! PS Don't we need something like the following:

Jorge wrote:

The patch is not 100% complete. For instance there's a need to provide a default keysrc file in a similar way that cookiesrc or dillorc (with useful comments).

Started looking into making a default keysrc. Some of the bindings in keys.cc are not the same as those in dillo2-help.html. Is that intentional? What is the purpose of KEYS_HIDE_PANELS when we have KEYS_FULLSCREEN?

Rogut?s wrote:

And one more suggestion: wouldn't it be more intuitive to have "command = keybinding" instead of "keybinding = command"?

*agrees*

Johannes Hofmann (2009-05-07 21:17):

On Thu, May 07, 2009 at 06:02:08PM +0000, corvid wrote:

...

Rogut?s wrote:

...

And one more suggestion: wouldn't it be more intuitive to have "command = keybinding" instead of "keybinding = command"?

*agrees*

Well in muttrc you have e.g:

bind pager <Down> next-line

So it's rather "keybinding = command". Perhaps the '=' is a bit confusing.

Looks like you're right. And shell's bindkey expects a key first (as well as vim's :map). Intuitive is the notion that the shorter part should be supplied first. Currently, dillo's commands look shorter/cleaner than keybindings. And it is the other way around with mutt's and vim's complex (more evolved?) commands... -- -- Rogut?s Sparnuotos

This patch allows reloading by just pressing enter in the navigation bar. --Tim