awesomeWM / awesome-www

Website of AwesomeWM
https://awesomewm.org/
56 stars 34 forks source link

Added screen rotation recipe #123

Closed raksooo closed 6 years ago

raksooo commented 6 years ago

https://github.com/raksooo/screenrotation

psychon commented 6 years ago

This addition is basically fine with me.

Some quick comments on your code:

Your functions are not local. In fact, all of them are just set in the global table. You might want to do something about that, but that requires reordering things a bit (a local function has to be declared before all of the calls to it).

You can replace your getScreen function with

function getScreen()
    return (next(screen.primary.outputs))
end

Your splitString function looks like it does about the same thing as gears.string.split.

Replace your init() function with:

tag.connect_signal("property::selected", function(t)
    local r = t.rotation or "normal"
    [The same code as previously in here, but using t instead of tag]
end)

That way this code also works with tags that are created later (in fact, you don't need this in an init function at all anymore and can do this just in "global context").

Can that code in getDevices() be somehow replaced with filtering the output of xinput? Surely it can somehow list devices...? That would be less Linux specific and would feel less "weird" to me (the current version works around X11 instead of with it).

I'd replace your spawn in rotateInput with the following:

    awful.spawn({
        "xinput",
        "set-prop",
        v,
        "Coordinate Transformation Matrix",
        rotationMatrices[rotationString]
    }, false)

The extra , false argument disables startup notification and providing the arguments as a table means you do not have to escape things. (Same comment to the spawn in rotation.rotate)

raksooo commented 6 years ago

Thanks for the feedback and I'm sorry I haven't got back to you until now. I've updated it according to your feedback except for sending a table into awful.spawn in rotateInput. I ran into the problem that the last argument is actually multiple integers which should not be enclosed in quotes.

The getDevices function now uses xinput to show device names but still has to find the relevant ones since xinput does not have a search function.

psychon commented 6 years ago

Oh, sorry that I forgot about this pull request after leaving my comment. I kind of expected someone else to also take a look at this, give another :+1: and then merge this.