February 2009 - Dillo-dev - lists.mailman3.com

[patch] Configurable key bindings

by tim.nieradzik＠gmx.de

> 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

Feb. 24, 2009

patches, bundles, etc

by tim.nieradzik＠gmx.de

It is always difficult for users to track changes between larger patches. I think Johannes did it right with his css-prototype repository. It is not the best thing to directly integrate bigger patches into mainline as they might be yet incomplete or lead to instabilities. Thus, having additional repositories is a good thing. If the features are stable enough and are thought to be useful, they can be simply merged into the main tree. In the Git world, where nearly everything is decentralized, it frequently happens that every core developer has an own repository for experimental changes. Of course this approach might not be always practical for smaller projects but we all can benefit from the fundamental idea of decentralisation. For Dillo, experimental per-feature repositories would be very useful before not well-tested patches go into mainline and risk the application's stability. Hence, I advocate that we make use of external repositories more frequently assuming it makes sense based on the changes' size. --Tim

Feb. 24, 2009

[patch] Configurable key bindings

by corvid＠lavabit.com

Tim wrote: > > 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. So something like enum { KEYCMD_RELOAD, ... }; commands[] = { {"reload", KEYCMD_RELOAD}, ... }; isPressed(&bindings[KEYCMD_RELOAD]) ?

Feb. 24, 2009

[patch] Configurable key bindings

by tim.nieradzik＠gmx.de

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

Feb. 24, 2009

[patch] Configurable key bindings

by tim.nieradzik＠gmx.de

> 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

Feb. 24, 2009

patches, bundles, etc

by dillo-dev＠toykeeper.net

BTW, what's the policy on submitting sizable changes with multiple revisions, such as the configurable keybindings? Is it okay to send a changeset bundle, or do the changes need to be flattened into a single patch? It probably doesn't matter for small changes, but it might be nice to include the history for large changes. -- Scott

Feb. 24, 2009

[patch] Configurable key bindings

by dillo-dev＠toykeeper.net

* Tim Nieradzik <tim.nieradzik(a)gmx.de> wrote: > It took longer to write this patch than I expected. That really was quick! I'm excited to see this. I'm not very familiar with Dillo's code, so I only have superficial comments. Mostly, I see some keys missing... > +struct mapping_s keyNames[] = { > + { "Backspace", fltk::BackSpaceKey }, > + { "Delete", fltk::DeleteKey }, > + { "Down", fltk::DownKey }, > + { "End", fltk::EndKey }, > + { "Esc", fltk::EscapeKey }, > + { "F1", fltk::F1Key }, > + { "F2", fltk::F2Key }, > + { "F3", fltk::F3Key }, > + { "F4", fltk::F4Key }, > + { "F5", fltk::F5Key }, > + { "F6", fltk::F6Key }, > + { "F7", fltk::F7Key }, > + { "F8", fltk::F8Key }, > + { "F9", fltk::F9Key }, F10 through F20? > + { "Home", fltk::HomeKey }, > + { "Insert", fltk::InsertKey }, > + { "Left", fltk::LeftKey }, > + { "PageDown", fltk::PageDownKey }, > + { "PageUp", fltk::PageUpKey }, > + { "Print", fltk::PrintKey }, > + { "Return", fltk::ReturnKey }, > + { "Right", fltk::RightKey }, > + { "Space", fltk::SpaceKey }, > + { "Tab", fltk::TabKey }, > + { "Up", fltk::UpKey } > +}; Keypad keys? > +static struct mapping_s modifierNames[] = { > + { "Shift", fltk::SHIFT }, > + { "Ctrl", fltk::CTRL }, > + { "Alt", fltk::ALT }, > + { "Button1", fltk::BUTTON1 }, > + { "Button2", fltk::BUTTON2 }, > + { "Button3", fltk::BUTTON3 } > +}; What about Meta, Hyper, Super, AltGr, and Modeshift? Or, more generally, Mod1 through Mod5? 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. > + if ((Keys::isPressed("hide-panels")) && (get_panelmode() == UI_TEMPORARILY_SHOW_PANELS)) { > + set_panelmode(UI_HIDDEN); > + ret = 1; > + } else if (Keys::isPressed("bookmarks")) { > + a_UIcmd_book(a_UIcmd_get_bw_by_widget(this)); > + ret = 1; > + } else if (Keys::isPressed("find")) { > + set_findbar_visibility(1); > + ret = 1; > + } else if (Keys::isPressed("goto")) { > ... > + } else if (Keys::isPressed("back")) { > + a_UIcmd_back(a_UIcmd_get_bw_by_widget(this)); > + ret = 1; > + } else if (Keys::isPressed("forward")) { > + a_UIcmd_forw(a_UIcmd_get_bw_by_widget(this)); > + ret = 1; > } 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 It's a minor thing really, but I noticed some inconsistent whitespace, and those config lines make it really easy to spot. :) -- Scott

Feb. 24, 2009

[patch] Configurable key bindings

by corvid＠lavabit.com

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. That's a good point.

Feb. 24, 2009

[patch] Configurable key bindings

by darkspirit5000＠gmail.com

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(a)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(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev >

Feb. 24, 2009

[patch] Configurable key bindings

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

Feb. 24, 2009