Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client.keys: callable table? #3194

Open
blueyed opened this issue Oct 5, 2020 · 8 comments
Open

client.keys: callable table? #3194

blueyed opened this issue Oct 5, 2020 · 8 comments
Assignees
Milestone

Comments

@blueyed
Copy link
Member

blueyed commented Oct 5, 2020

I've noticed that client.keys was changed from a function to a table in Git master (for 4.4), and wondered if this a) was intentional really (likely), but b) more importantly if it could be kept working like before by having a "callable table"?

/cc @Elv13 (just guessing, have not investigated / git-blamed etc)

@blueyed blueyed added this to the v4.4 milestone Oct 5, 2020
@psychon
Copy link
Member

psychon commented Oct 5, 2020

Seems like 296ad18, although I am not quite sure what it actually creates. It seems like the call itself just creates awful.client.get_keys and .set_keys and the already existing magic then handles assignment.

@blueyed Could you describe your actual problem? I am not quite sure, but I would guess that you saying that c:client() used to work and now no longer does?

@blueyed
Copy link
Member Author

blueyed commented Oct 5, 2020

I would guess that you saying that c:client()c:keys() used to work and now no longer does?

Correct. Quite easy to address/fix in my config, but would be better if it would just keep working I guess.

@Elv13
Copy link
Member

Elv13 commented Oct 6, 2020

Correct. Quite easy to address/fix in my config, but would be better if it would just keep working I guess.

Can you describe what broke? I guess my unit tests were not good enough, but I put quite a lot of effort into re-implementing the old behavior.

What was done

  • The capi.key and capi.button were deprecated.
  • The c:buttons() style properties are all being deprecated and replaced one by one in favor of c.buttons.
  • awful.key/awful.button were not real objects, they were tuples of capi.key/capi.button

Why it was done

1: Mixing old and new property style is inconsistent

One of the common problem users have is due to API inconsistencies. For example, we have the client c:geometry()["width"] while we also have the screen s.geometry.width. In the long run, I want to replace all the former with the later. I have a giant PR series bit rotting to finish the job, but it has issues (probably like the one you had). With the "declarative style" modules proliferating across the API, having those legacy APIs is more and more problematic since it prevents me from implementing some meta-syntax-sugar until they all die. Plus, the newer ldoc magic has a lot of nice features for normal properties (type, signals, permissions, etc) which cannot be used for methods.

2: Modularity

One of the project I had ongoing for a while is the push toward modularity. This is implemented in 2 main ways. First of all, following your early work on this, the request:: style signals are increasingly used to have clear ways to plug logic during specific events and startup "milestones". This creates a second problem. Some APIs, like awful.rules.rules, root.keys/root.buttons/awful.layout.layouts/client:buttons()/client:keys() were "write only". Now all APIs are being ported one by one to have :append_foo() and :remove_foo() methods/functions with proper signals and proper request::init_foo point of entry for modules. Module which need to add keybindings to clients would previously have had to parse the awful.rules.rules table and figure out how to inject themselves in. If they wanted to add a global one, they had to use an arbitrary number of gears.timer.delayed_call()+another call to check if it survived because it would get overridden by other modules and/or rc.lua code.

Not only is it hard to enable/disable/add/remove specific keybindings, but the API was also lossy. Because awful.key was implemented as a tuple, it was "squashed" by gears.table.join into an array of capi.key. This means it was impossible to "just" improve the awful.key and turn it into an object.

In an attempt to preserve backward compatibility, a __call was added on the client.buttons/client.keys properties to "act like before" when you call it while using proper objects when used in the "modern API style". One tradeoff I had to make is that it support "mixing" both styles using way too much ugly code. Since I would have been unable to use the improved objects if the gears.table.join kept destroying them, I had to come up with some fuglyness to undo the destruction and get the object back. My guess is that this is where you had issues.

3: Modernizing the input APIs

With the awful.keygrabber modernization I did in 2017 and the awful.spawn work we did in Berlin in 2015, we were in this weird spot where we had 3 pseudo identical syntax to describe keybinding, but they were only compatible on the surface. By moving to real objects, this code can slowly be unified. It also made possible to implement higher level bindings like "all F keys" or "all numpad keys". When mixed with awful.keygrabber, it also gives us i3 like modal keybindings.

4: Introspection

One of the big roadblock @actionless has when he improves the hotkey popup is that the way we "track" what key do what was a total mess. There was some hardcoded "record this stuff here cuz it's gonna get destroyed when this function returns" style of nonsense. With awful.keyboard/mouse, the idea is that there should be proper APIs to query what-do-what. Maybe one day this could allow mouse-gestures or unity style HUD to be added.

@actionless

This comment has been minimized.

@Elv13

This comment has been minimized.

@blueyed
Copy link
Member Author

blueyed commented Mar 17, 2021

@Elv13

Correct. Quite easy to address/fix in my config, but would be better if it would just keep working I guess.

Can you describe what broke?

This snippet is broken now, since c:keys() isn't a method (getter/setter) now anymore: https://gist.github.com/blueyed/386a0657bedc4beaeaf9
Fix/change: https://gist.github.com/blueyed/386a0657bedc4beaeaf9/revisions#diff-64ffc17ae1cabcb9ed08b0ac51955cfdb7c95bafe993090b9935755f422478c1 - does it look ok, or would it need to copy tables?

Note that there is still root.keys as a method, so this is adding some inconsistency there now until it also becomes a property.

@Elv13
Copy link
Member

Elv13 commented Mar 21, 2021

@blueyed Umm, I am still not sure what broke, c:keys() is still there. It still works. If you have some time, please help find what broke. I added quite a lot of tests and they all pass. I am not saying you didn't hit a corner case, obviously you did, but I don't know what and don't have the bandwidth to reproduce your whole setup.

The only know thing that change is type(c.keys) == "function". c.keys returns a metatable with a __call.

Note that there is still root.keys as a method,

It is both a property and method, just like c:keys().

There was more commits that I havn't got merged that also did this to :struts(), :geometry() and every other accessor method across capi. The goal is to bring consistency about how properties work.

The reason why the key and button one is merged was because it was necessary in order to merge capi.key and awful.key classes into a single object as mentioned above.

@actionless
Copy link
Member

@blueyed is there any piece of previously published API doc which declares the behavior which got changed but you still expecting it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants