Page MenuHomePhabricator

efl_ui_item: add keybindings for selecting changes
ClosedPublic

Authored by bu5hm4n on Aug 23 2019, 3:28 AM.

Details

Summary

This started as a small commit, when pressing enter -> set the item as
selected.
However, it was a bit more complex, it seems that there was never config
update code to copy bindings into the user profile. Which lead to the
fact that you are missing a lot of keyboard related features if you
havnt wiped your config in the last 1-2 years. For me keybindings for
Efl.Ui.Scroller Tab_Bar Image_Zoomable (Item) have been missing and were
never inserted. WHich is a problem for a user just constantly updating.

For now i created a function that copies over the bindings from the
system config, and they are merged into the user config. Intentional
leaving our of keybinding structs for a user-config will result in them
beeing merged again on the next config update. If you want to get rid of
key bindings as a user you can just keep the empty struct, which is the
signal for "i know what i am doing, i do not want to have them". The
problem that the system config is partly invalidated (due to moving the
key bindings struct to the user config), is fixed due to the fact that
the config is reloaded after that.

This function should be called everytime someone updates the config in
regards of the keybindings.
Depends on D9722

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.Aug 23 2019, 3:28 AM
bu5hm4n requested review of this revision.Aug 23 2019, 3:28 AM
zmike requested changes to this revision.Aug 23 2019, 11:16 AM

Historically in EFL projects we've just added keybindings based on version changes, e.g., version X adds keybind Y, so add Y to the config when doing the X upgrade. This change would add back bindings which have been manually deleted by users. Instead, the bindings update function should just copy the system config (and then not destroy the system config unnecessarily) for the config version being upgraded for.

src/lib/elementary/elm_config.c
2451

This is bad; we should not be reloading the config from disk again here.

This revision now requires changes to proceed.Aug 23 2019, 11:16 AM
bu5hm4n requested review of this revision.Aug 23 2019, 11:39 AM

This is IMO a terrible guideline, a user does not even know where the configs are, so i guess he also does not know how to add configs manually from config 1 to another config. We completly lack this migration code for all the newly added widgets, which leaves (at least for me, and someone who sent me his config) the new widgets like Efl.Ui.Scroller [and others]. The user could delete the list content, but keep the struct, which would us also give a chance code wise to check if a config is broken, or the user disabled things.

Additionally, commits like 850498e977d4fb216a832e72f843fd10412ac7ad went and only renamed the class + the config and accessed the config with the new key, no migration code at all was added for this (this is not the only thing where that was done, slider, radio, check, calender [...]) got the same problem, so without a commit like this above we are more or less left with a situation where someone has to dig through the history and find out when this was done. Then we would have to filter out which changes to the config have been made afterwards, and which config bumps have been there, and react accordingly. Given the fact of the complexity of this, vs. the amount of users that potentially disabled theire key bindings by hand, i am very tempted of just overwriting them for now if they completly removed it.

If you want to see the full scale problem of this: Take a config from stable 2 years ago, and you will see that we fail to even press a button with Enter.

src/lib/elementary/elm_config.c
2451

We are stealing the keybindings from the system config, copying such a complex struct is really just a pain, this code here is executed once per upgrade, i guess its not too bad.

Additionally, this is not really needed, as noone touches tcfg after that, i just want to reload that here, as someone writing upgrade code for a later version bump will propebly forget about this.

zmike accepted this revision.Aug 23 2019, 12:03 PM

I suppose I'm okay with a one-time config upgrade for this case. But for future cases then it should work as I described.

This revision is now accepted and ready to land.Aug 23 2019, 12:03 PM
Closed by commit rEFLc467dc6e813c: efl_ui_item: add keybindings for selecting changes (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyAug 26 2019, 5:44 AM
This revision was automatically updated to reflect the committed changes.