Page MenuHomePhabricator

efl.ui.textbox: add and use keyboard bindings
ClosedPublic

Authored by ali.alzyod on Jan 29 2020, 8:02 AM.

Details

Summary

As other widgets, efl.ui.textbox will use keyboard bindings instead of listen to keyboard events

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15762
Build 10737: arc lint + arc unit
ali.alzyod created this revision.Jan 29 2020, 8:02 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Jan 29 2020, 8:03 AM
bu5hm4n requested changes to this revision.Jan 30 2020, 8:25 AM

This looks overall good and correct.
One piece is missing: in elm_config.c you need to add code that copies over the key bindings for "Efl.Ui.Textbox". I think code like this:

static void
_elm_key_bindings_copy_missing_bindings_textbox(Elm_Config *cfg, Elm_Config *syscfg)
{
   Elm_Config_Bindings_Widget *wd;
   Eina_List *n, *nnext;

   EINA_LIST_FOREACH_SAFE(syscfg->bindings, n, nnext, wd)
     {
         if (!eina_streq(wd->name, "Efl.Ui.Textbox"))
           {
              syscfg->bindings = eina_list_remove_list(syscfg->bindings, n);
              cfg->bindings = eina_list_append(cfg->bindings, wd);
           }
     }
}

Should do the right thing. It should be called in line 2474 in elm_config.c.

(We need to do that in order to update users config to support textbox).

This revision now requires changes to proceed.Jan 30 2020, 8:25 AM

This looks overall good and correct.
One piece is missing: in elm_config.c you need to add code that copies over the key bindings for "Efl.Ui.Textbox". I think code like this:

static void
_elm_key_bindings_copy_missing_bindings_textbox(Elm_Config *cfg, Elm_Config *syscfg)
{
   Elm_Config_Bindings_Widget *wd;
   Eina_List *n, *nnext;

   EINA_LIST_FOREACH_SAFE(syscfg->bindings, n, nnext, wd)
     {
         if (!eina_streq(wd->name, "Efl.Ui.Textbox"))
           {
              syscfg->bindings = eina_list_remove_list(syscfg->bindings, n);
              cfg->bindings = eina_list_append(cfg->bindings, wd);
           }
     }
}

Should do the right thing. It should be called in line 2474 in elm_config.c.

(We need to do that in order to update users config to support textbox).

Thank you for the information, hmmm but it is strange that it works fine in my machine without this update.

bu5hm4n added a comment.EditedJan 31 2020, 5:58 AM

Okay, i rechecked here, there is a bug in the update code. The bug is fixed with the revisions i have added as children to this revision, when you run it now, the config is not automatically updated.

diff --git a/src/lib/elementary/elm_config.c b/src/lib/elementary/elm_config.c
index 63eda20a3f..f6ff33cf0a 100644
--- a/src/lib/elementary/elm_config.c
+++ b/src/lib/elementary/elm_config.c
@@ -2474,6 +2474,12 @@ _config_update(void)
    tcfg = _config_system_load();
    IFCFGEND

+   IFCFG(0x0017)
+   _elm_key_bindings_copy_missing_bindings_of_widget(_elm_config, tcfg, "Efl.Ui.Textbox");
+   /* after this function call, the tcfg is partly invalidated, reload! */
+   _config_free(tcfg);
+   tcfg = _config_system_load();
+   IFCFGEND
    /**
     * Fix user config for current ELM_CONFIG_EPOCH here.
     **/

This is all this patch needs after you apply the revisions before.

ali.alzyod added a comment.EditedJan 31 2020, 10:52 PM

@bu5hm4n Thank you and I will update this patch.
But the change seems strange to me because,
in the previous version, it will automatically update all bindings regardless of widget name(_elm_key_bindings_copy_missing_bindings(_elm_config, tcfg);), but now, we need to mention widget name (updated bindings in file + hardcoded the names update in elm_config.c for each widget)

Is not updating last IFCFG from (0x0016) to be (0x0017), without any other modification is enough? (for example, someone installing EFL for the first time in his system, the binding adding textbox will be executed twice)

As I said before, there is a bug currently in the update code. _elm_key_bindings_copy_missing_bindings should not be executed. The revision D11261 is fixing that. Revision D11262 is adding a helper, so you only have to add the code snipped i have pasted above to this revision.

As I said before, there is a bug currently in the update code. _elm_key_bindings_copy_missing_bindings should not be executed. The revision D11261 is fixing that. Revision D11262 is adding a helper, so you only have to add the code snipped i have pasted above to this revision.

I see the steps you are suggesting, but I am confused with this update, why do we need to do such update (It seems strange to me).
As I see the issue, if we made any update to config files, them we need to increase the CFG number, regardless of the change to read the whole file again.
but now we need to hard code the changes again in elm_config.c file which is strange.

Suppose someone update Efl.Ui.Scroller keybindings, then what will happen ?
in elm_config.c you will add new check for "Efl.Ui.Scroller" again or change the condition before _elm_key_bindings_copy_missing_bindings and this call will update "Efl.Ui.Textbox" anyway, then because we hardcoded "Efl.Ui.Textbox" it will be checked again.

So can you help me understand why we need all this ?

I am not sure what exactly is the problem, so here is the whole story about elm_config:

  • Elm has a config, this is stored somewhere in the home dir of the user. The user can edit this config to his needs.
  • Every time a new efl version comes with a new config, we need to update the user config, to also have the new default value, if we don't do that, we are leaving the user with a config that is not inline with the default in fields he has not altered.
  • Due to some fuckups in the past, a "copy over *all* the keybindings" move was required. (more details can be found here https://phab.enlightenment.org/D9723#180539). However, as @zmike states in that ticket, we should never do that per default, and he is very right with that. So _elm_key_bindings_copy_missing_bindings should not be called on a arbitrary config update, we should only copy the keybindings that are new.

For how the update code works, please check the macros IFCFG and IFCFGEND, this code is not called *all* the time, it is only called when the config is updated from some version lower than that, if you are updating from 16 to 17, then for example *only* the code from my code snipped is executed.

ali.alzyod added a comment.EditedFeb 1 2020, 4:40 AM

Due to some fuckups in the past, a "copy over *all* the keybindings" move was required. (more details can be found here https://phab.enlightenment.org/D9723#180539). However, as @zmike states in that ticket, we should never do that per default, and he is very right with that. So _elm_key_bindings_copy_missing_bindings should not be called on a arbitrary config update, we should only copy the keybindings that are new.

To be honest I do not see why _elm_key_bindings_copy_missing_bindings should not be called on a arbitrary config update, why not, it will only be executed only once when user run first efl application with the new version (right?!), and I have same comments in mind as the one you added in that comment (https://phab.enlightenment.org/D9723#180539),
I see it is strange to hardcode the changes in elm_config.c (unless we do not want to overwrite custom changes the user made but even for this we can apply changes in backward order where user changes is the last applied changed)

Calling _elm_key_bindings_copy_missing_bindings at any arbitrary config update *will* overwrite the configs a user has made, if he ever kicked out the keybindings for a specific widget, they would magically come back. This is *not* what we want. What we want is: updating the config with what we have added upstream, and at least give the user the possibility to see what this new thing is, *without* overwriting the configs he has taken.

Your suggestion to apply the user changes in backward order does not really change the situation, first we don't know the order that he has made the changes, so we only have one diff that we would apply. However, to know which of these changes are due to user-changes and which are due to config updates, we would still need to know from which config to which config we are updating, which leaves you with exactly the same situation as you have right now (You have to specify *in code* which are the default values that are new, and not just changed by the user)

ali.alzyod added a comment.EditedFeb 2 2020, 12:51 AM

@bu5hm4n I see, when the user deletes keybindings for the whole widget (or specific keys), it is not easy to detect (unless we know exactly what each binding in file version contains). (or we should always have the user modified file, and original file at the same install)

Anyway with even with this current approach, there are still other issues like if user made some specific changes in Widget key bindings, in the current approach we rewrite everything not just only the change (we will rewrite user custom changes in these keybindings).

@bu5hm4n I see, when the user deletes keybindings for the whole widget (or specific keys), it is not easy to detect (unless we know exactly what each binding in file version contains). (or we should always have the user modified file, and original file at the same install)

We have both.

Anyway with even with this current approach, there are still other issues like if user made some specific changes in Widget key bindings, in the current approach we rewrite everything not just only the change (we will rewrite user custom changes in these keybindings).

This is not true. we only overwrite Efl.Ui.Textbox which has not been there before.

@bu5hm4n I see, when the user deletes keybindings for the whole widget (or specific keys), it is not easy to detect (unless we know exactly what each binding in file version contains). (or we should always have the user modified file, and original file at the same install)

We have both.

then detecting user changes can be easily detected

Anyway with even with this current approach, there are still other issues like if user made some specific changes in Widget key bindings, in the current approach we rewrite everything not just only the change (we will rewrite user custom changes in these keybindings).

This is not true. we only overwrite Efl.Ui.Textbox which has not been there before.

Of-course we only overwrite textbox because there were not textbox key bindings, but If new update coming to textbox in future, them we will copy the whole new textbox keybindings and we do not care about user deleted keys

@bu5hm4n I see, when the user deletes keybindings for the whole widget (or specific keys), it is not easy to detect (unless we know exactly what each binding in file version contains). (or we should always have the user modified file, and original file at the same install)

We have both.

then detecting user changes can be easily detected

You still need encode the knowledge, which version jump resulted in which change, which still means that you need to change the base config file + the update method.

Anyway with even with this current approach, there are still other issues like if user made some specific changes in Widget key bindings, in the current approach we rewrite everything not just only the change (we will rewrite user custom changes in these keybindings).

This is not true. we only overwrite Efl.Ui.Textbox which has not been there before.

Of-course we only overwrite textbox because there were not textbox key bindings, but If new update coming to textbox in future, them we will copy the whole new textbox keybindings and we do not care about user deleted keys

This is not true. We only update The Textbox keybindings form the version jump 16 to 17.Never again after that.

ali.alzyod added a comment.EditedFeb 2 2020, 3:20 AM

@bu5hm4n I see, when the user deletes keybindings for the whole widget (or specific keys), it is not easy to detect (unless we know exactly what each binding in file version contains). (or we should always have the user modified file, and original file at the same install)

We have both.

then detecting user changes can be easily detected

You still need encode the knowledge, which version jump resulted in which change, which still means that you need to change the base config file + the update method.

I think the only knowledge you need from user is what he has changed, other things is handled directly by the toolkit installation (replace old keybindings file with new one).

Anyway with even with this current approach, there are still other issues like if user made some specific changes in Widget key bindings, in the current approach we rewrite everything not just only the change (we will rewrite user custom changes in these keybindings).

This is not true. we only overwrite Efl.Ui.Textbox which has not been there before.

Of-course we only overwrite textbox because there were not textbox key bindings, but If new update coming to textbox in future, them we will copy the whole new textbox keybindings and we do not care about user deleted keys

This is not true. We only update The Textbox keybindings form the version jump 16 to 17.Never again after that.

Again I am talking about future not this patch, For example, suppose we want to add "Page_Up" key action to all widgets, what is the solution? we will end-up overwrite all users changes right?

Or in simple words:
If the user installing EFL for the first time with config V17
Check for V16 will add Textbox(because it is already in the config file), and Check for V17 will add Textbox again.

This revision seems right.

Or in simple words:
If the user installing EFL for the first time with config V17
Check for V16 will add Textbox(because it is already in the config file), and Check for V17 will add Textbox again.

No. Please read the macros. If the user has version V17, the code in IFCFG(0x0017) is not getting executed, nor the code in IFCFG(0x0016).

bu5hm4n accepted this revision.Feb 4 2020, 12:07 AM
This revision is now accepted and ready to land.Feb 4 2020, 12:07 AM
Closed by commit rEFLa92f8c210bb6: efl.ui.textbox: add and use keyboard bindings (authored by ali.alzyod, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyFeb 4 2020, 12:15 AM
This revision was automatically updated to reflect the committed changes.