User Facing Screenshot API

[poisson-aerohead]

This commit begins the development of a more appropriate user facing
screenshot API. It extends a prior commit which extended the lower level
content API, which had been a property of the client object but is now
available as a property of the screen object and a method of the root
object.

This commit creates a new screenshot module for the awful module. The
public functions include root(), screen(), client(), snipper(), and
snip(). These take root window, screen, and client window screenshots,
launch an interactive snip tool for cropped screenshots, and take a
cropped screenshot of a geometry passed by argument, respectively. The
init() function is also available for configuration. Using this library
is more appropriate for the average rc.lua.

Since the API is new, this commit does not include any changes to
rc.lua. The developers can modify rc.lua when there is sufficient
confidence in API stability and robustness.

lib/awful/init.lua is modified so that the awful module includes the new
lib/awful/screenshot.lua submodule.

Signed off: Brian Sobulefsky brian.sobulefsky@protonmail.com

[poisson-aerohead]

Hello again everyone. I wanted to post this so the project maintainers could get eyes on it and give feedback. As noted in the commit message, I did not modify rc.lua. Please see a sample binding below. At this time, I did not add test cases because it seems they cause race conditions and I am expecting churn after people examine this. I will add new test cases to tests/test-screenshot.lua before the PR is done.

awful.screenshot.init('/home/USER/Images/Screenshots', 'Screenshot-')

globalkeys = gears.table.join(globalkeys,
        -- Root window screen shot
        awful.key({ modkey }, "Print",
                function()
                    awful.screenshot.root()
                end,
                {description = "Print Screen - Root Window", group = "awesome"}),
        -- Physical screen screen shot
        awful.key({ modkey, "Mod1" }, "Print",
                function()
                    awful.screenshot.screen()
                end,
                {description = "Print Screen - Physical Screen", group = "awesome"}),
        -- Client window screen shot
        awful.key({ modkey, "Control" }, "Print",
                function()
                    awful.screenshot.client()
                end,
                {description = "Print Screen - Client Window", group = "awesome"}),
        -- Snip tool screen shot
        awful.key({ modkey, "Shift" }, "Print",
                function()
                    awful.screenshot.snipper()
                end,
                {description = "Snip Screen", group = "awesome"})
)

Looking forward to hearing from you all.

[codecov]

Codecov Report

Merging #3461 (47de5c9) into master (5077c83) will increase coverage by 0.07%.
The diff coverage is 95.85%.

❗ Current head 47de5c9 differs from pull request most recent head e088fe9. Consider uploading reports for the commit e088fe9 to get more accurate results

@@            Coverage Diff             @@
##           master    #3461      +/-   ##
==========================================
+ Coverage   90.93%   91.00%   +0.07%     
==========================================
  Files         897      900       +3     
  Lines       56682    57549     +867     
==========================================
+ Hits        51542    52372     +830     
- Misses       5140     5177      +37     

Flag	Coverage Δ
gcov	91.00% <95.85%> (+0.07%)	⬆️
luacov	93.71% <95.85%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/awful/keygrabber.lua	80.60% <52.94%> (-2.27%)	⬇️
tests/examples/awful/template.lua	97.92% <94.66%> (-1.49%)	⬇️
lib/awful/screenshot.lua	95.05% <95.05%> (ø)
tests/test-screenshot.lua	97.56% <97.04%> (-1.31%)	⬇️
tests/examples/awful/screenshot/keybindings1.lua	98.38% <98.38%> (ø)
lib/awful/init.lua	91.66% <100.00%> (+0.23%)	⬆️
lib/awful/key.lua	86.48% <100.00%> (ø)
lib/gears/filesystem.lua	85.88% <100.00%> (+1.07%)	⬆️
tests/examples/awful/screenshot/popup.lua	100.00% <100.00%> (ø)
tests/examples/shims/client.lua	100.00% <100.00%> (ø)
... and 6 more

[Elv13]

Hello, thanks for this.

Lets get started with some of the mundane "things to learn when contributing to AwesomeWM". That's pretty much "normal" for when contributing to projects. Your previous contribution was pretty low level, so needed less of that. None of what's below is a criticism, it's just explaining many undocumented conventions and API design guidelines we use for high level APIs. This is the first pass. Once this settles down, there will be some testing and code examples work to do, but that's for another day.

First, there is a tool called luacheck which the CI run. Normally, it prints tons of warnings, but you only get a couple, which is great:

luacheck ../lib/awful/screenshot.lua
Checking ../lib/awful/screenshot.lua 2 warnings

../lib/awful/screenshot.lua:185:5: setting non-standard global variable 'initilialized'
../lib/awful/screenshot.lua:432:9: unused variable 'date_time'

Total: 2 warnings / 0 errors in 1 file

Next, the documentation needs 3 --- at the top of the comment block to be interpreted. If I add this to your changes, I get:

As you can see, there is still a couple trivial fixes. The format for the parameter is @tparam[opt=something] type_name param_name description .. In this case, type_name is missing, which confuses ldoc and it renders the same description for everything (because every parameter ends up being called "The"). You can install ldoc, clear your cmake work directory and run cmake again. It should detect it and start rendering the doc. You can open it in your web browser as a local file.

So that's for the trivial "how to get the CI to pass and the doc to render" things. Now, let's move into the code a little bit.

awful.screenshot.init

Our objects constructors are (generally) directly the module name. In Lua, you do that using meta tables. Here's how:

local gobject = require("gears.object")
local gtable = require("gears.table")

local module = {}

--- A property!.
-- @property my_property
-- @tparam[opt=default] string my_property

function module:get_my_property()
    -- You don't need this accessor of the default is
    -- self._privaye.property_name
    return self._private.my_property
end

function module:set_my_property(value)
    -- This accessor can contain business logic and have side effects
    self._private.my_property = value

    self:emit_signal("property::value", value)
end

--- This is a method.
-- @method my_method
function module:my_method()
    -- do something
end

local function new(_, args)
    args = args or {}                                                                                                                                
    local ret = gobject {
        enable_auto_signals = true,
        enable_properties   = true,
    }

    ret._private = {
        my_property = "default_value"
    }

    -- This adds all the method and properties
    gtable.crush(ret, module, true)

    -- This apply all the constructor arguments.
    gtable.crush(ret, args, false)

    return ret
end

return setmetatable(module, { __call = new })

So, screenshot.init should create a "real" object and be renamed to use the convention demonstrated above.

The API in general.

For the API, right now, you have some static functions. I would like to see this moved to a more declarative/OOP style API. This means using a main objects with some properties and method.

With static function, it's hard to scale the API on the long term. For example, creating some composition like "take a screenshot of object foo, scale it, crop it, save it". With a more object oriented API, this comes pretty naturally. It is also easier to extend with new features, since you don't have to add endless parameters to each static functions. For example, the API could look like:

local ss = awful.screenshot {
    client = client.focus, -- automatically use client.content, by default, use root.content
    rectangle = {
        x = 0,
        y = 0,
        width = 100,
        height = 100
    }
}

ss:save("/tmp/image.png")

-- Because why not!
mywibar.widget.some_layout.add(ss.widget)

It also has the advantages of being a memento. Right now, c.content changes "from under", you render the same surface twice and it looks different. This is pretty unexpected if you store the surface in a variable. awful.screenshot could/should hold a copy of the surface within the object and only explicitly refresh it. That way, storing screenshots as variables now works.

[poisson-aerohead]

Great, thanks for this. I will start to implement the documentation changes and I can probably get what I have into the OO style.

The problem with the tparams seems to be that I did 'tparam[opt] string The...' so I actually did put the type. Is the the 'something' in 'opt=something' the default value?

[poisson-aerohead]

Hey, before I go too crazy, can we get people to give some input on what an "object oriented" library should look like? My initial submission centered around taking the various kinds of screenshots, saving them to files, and returning the path to the newly saved file. The various kinds included root window, physical screen, client window, interactive snip tool, and programmatically defined crop. The directory and base filename of the saved screenshot could be specified, but essentially defaulted to something that would likely work. Otherwise, it could be "initialized" once and called over and over again with no further arguments.

I can change this to an "object oriented" design, but is there anything else people think makes sense?

EDIT:
I think part of the disconnect is that I am thinking of "screenshot" as "awful.screenshot", which to me is a library. You may be thinking of "screenshot" as an actual screenshot. So when you say that c.content "changes from under you", under this new submission you would use (as my excerpt from rc.lua shows above) awful.screenshot.client(), which is clearly a function call. As a function call, it returns a new screenshot of the currently active client (it actually returns a string containing the path to where the new screenshot was saved). That return value would not change from under you. The others are similar.

It is worth noting that instead of returning just a string with the path, an object containing the surface itself and some manipulation methods can be returned. It does not need to save automatically, either, but instead require a ss:save_to_file("/path/to/file.png") as you say. I figured that this is so common, though, that it would become "boilerplate" code, and so should be in the library itself. This is especially the case with appending a date_time string to the file base name to make it unique (and easily searchable if you know about when you took a particular shot).

[poisson-aerohead]

Hi everyone, sorry to have dropped off on this one. I have been busy with some other things. I corrected the documentation generation syntax. The issue was that the correct format seems to be @tparam type name DescriptionString and I was missing the name. I rebased this PR to the current state of the master, so a month's worth of changes should be behind us (including fixing the screenshot test script!).

Please provide a little direction as to what the OO system should look like. I still think there is a disconnect between thinking of "screenshot" as the library for taking the screenshots (how I am using the term) or as an actual image. The way I am looking at it, we need some interface for the user to request a screenshot be taken and how (the root window, the client, a particular screen, a snip tool, a defined crop of the screen). If the desire is for that action to create a new object (that can be saved, scaled, put in a widget, whatever), then I understand that. However, for the screenshot library itself to return objects is a little weird to me (it can certainly be done that way). For example, beautiful uses static methods like beautiful.init(), and gears holds all sorts of static methods like gears.wallpaper.fit(). I personally think it is natural for there to be a similar screenshot library that can be called upon to perform actions, such as awful.screenshot.root() to take a full screenshot or awful.screenshot.client() to get the active client content, but I am open to a different direction. It is worth remembering that if we don't do that, there still needs to be a way to distinguish between the different types of screenshots. So root/screen/client will need to be an argument passed to screenshot.new(), or something along those lines. I find that to be less natural.

If you are looking for the library itself to be an object, please discuss with me the class layout you are visualizing. Otherwise, if the desire is for the screenshot methods to return objects with properties and methods, such as ss = awful.screenshot.screen(); ss:scale(geom); ss:save("/path/to/file.png"); then I view that as a different task and we should define what sort of object that is going to be.

Input from some of the others on here welcome, @actionless @Aire-One @psychon

[actionless]

i think the next step would be to improve the coverage level to pass the CI check:

12.16% of diff hit (target 90.65%)

[poisson-aerohead]

I'm not sure how that part works.

[actionless]

i think you can start from the existing tests for low-level screenshot api as a starting point

[actionless]

or mb adding examples would be better, but here i myself not sure how it would be better to implement for examples

[sclu1034]

I think the disconnect might come from the fact that, while pure static libraries like gears exist, most of our modules are actually more like classes.
awful.screenshot would, first and foremost, be a table with a few functions. But through metatables, it would also be callable, and awful.screenshot() would return an instance/object of the class.
Many other modules work just like that, and their docs can have both a "static functions" and an "object methods" section to reflect that.

In this case, I agree with Elv that a more OOP-like API would be the better fit long-term. I.e. awful.screenshot would be a class, and most functionality would be handled through methods on objects that awful.screenshot() creates.

But that doesn't mean that it can't also cover the use cases you describe.
Building off of Elv's proposed API:

• To make the use case of "take a screenshot and immediately save it" easier, you could add module-level defaults:

awful.screenshot.set_default_path(os.getenv("HOME") .. "/Pictures/Screenshots")
awful.screenshot.set_name_template("%Y-%m-%d.png") -- see GLib.DateTime.format

local shot = awful.screenshot()
shot:save() -- Build file path from defaults
shot:save("/tmp/just_a_test") -- Override defaults. If `just_a_test` is a directory, use name template to store inside

• You can still create static functions like awful.screenshot.client() as thin wrappers around the generic constructor
• Use the "builder" pattern, where each method on the object returns the altered instance to allow chaining

local shot = awful.screenshot()
local small_shot = shot:scale(0.5)
local fancy_shot = shot
	:draw_rectangle(...)
	:draw_circle(...)
	:text(...)

---

If you combine the first two, your use case above becomes

-- Once at the start
awful.screenshot.set_default_path(os.getenv("HOME") .. "/Pictures/Screenshots")
awful.screenshot.set_name_template("%Y-%m-%d.png")

-- In a hotkey
awful.screenshot.client():scale(0.5):save()

It's a few more characters to write in the hotkey hander, but more expressive, without hidden defaults/constants and allows for a much more powerful and extensible API, as Elv already described.

[poisson-aerohead]

@actionless sorry I misunderstood. I didn't realize that the CI metric was what the tests covered. Can I fix that by just adding tests that use the new methods in the tests/*.lua files?

@sclu1034 I agree with your line of thinking and would point out that you are naturally thinking of the object library itself, awful.screenshot, as something distinct from the returned screenshot, which you are calling `shot'. This is what I noticed too, we really have two (albeit related) object hierarchies here.

[actionless]

@poisson-aerohead yup, either tests or examples - both counted for coverage

[poisson-aerohead]

Hi everyone. Sorry for the long hiatus on this one, but as you know, I did have the awful.screenshot library working as I wanted on my own machine, and trying to rewrite this in the style you requested took a bit of a backseat. Some quick notes. As we discuss on this thread, the awful.screenshot library distinguishes between the library itself and the returned screenshot object. So there is a generic constructor, new(args), callable as ss = awful.screenshot(args) that returns a "screenshot object". There are also some wrapper constructors, callable as ss = awful.screenshot.root(), etc.

The returned object has methods like ss:save(), that can take optional arguments. The module should work out of the box with the following rc.lua:

  globalkeys = gears.table.join(globalkeys,
          -- Root window screen shot
          awful.key({ modkey }, "Print",
                  function()
                      local ss = awful.screenshot.root()
                      ss:save()
                  end,
                  {description = "Print Screen - Root Window", group = "awesome"}),
          -- Physical screen screen shot
          awful.key({ modkey, "Mod1" }, "Print",
                  function()
                      local ss = awful.screenshot.screen()
                      ss:save()
                  end,
                  {description = "Print Screen - Physical Screen", group = "awesome"}),
          -- Client window screen shot
          awful.key({ modkey, "Control" }, "Print",
                  function()
                      local ss = awful.screenshot.client()
                      ss:save()
                  end,
                  {description = "Print Screen - Client Window", group = "awesome"}),
          -- Snip tool screen shot
          awful.key({ modkey, "Shift" }, "Print",
                  function()
                      awful.screenshot.snipper()
                  end,
                  {description = "Snip Screen", group = "awesome"})
  )

You can optionally configure it first with

awful.screenshot.set_defaults({directory = '/path/to/your/screenshots',
                                 prefix = 'MyScreenshotPrefix-'
                              })

if desired. I'm sure there will be some issues with getting the tests to pass, which I am a bit underinformed on. Also, the documentation could use a bit of expansion. but it should at least have a basic description for everything.

[poisson-aerohead]

Hey everyone. I don't know what's going on with the failure. On my machine, I can just call it with no configuration. What's weird is that the default prefix is hard coded to "Screenshot-" so I can't imagine any way you could end up with it being nil.

EDIT:
So it looks like the failure might be occurring not due to the prefix portion but the default directory. This defaults to reading the environment HOME variable. It also needs to have bash verify that the directory exists and is writable. would this fail in Xepher?

[actionless]

idk, i was trying to look into your code but you have single-letter var names so i gave up, but good luck fixing it :)

[poisson-aerohead]

@actionless most of my var names are pretty descriptive. I think there might be a few temporary variables where I use that. I got all the tests to successfully run. I currently am doing this by hard coding a path in my test script like local ss = awful.screenshot.root({directory = "/path/to/Images"}). I would think that this is brittle. I had to solve one problem but now that I did I will figure out why reading the environment for HOME is failing. Again, to be clear, it is not failing in actual deployment, but only for the test suite. I have successfully tested (from my rc.lua) taking screenshots without ever setting the defaults and letting it be determined from the HOME environment var.

EDIT:
Yes, speaking of which, here is a "one letter var" example. This is a little helper function not really a part of the library API. It should be clear what the function does and what the variable does. I can change it to "dir" or even "directory" if the project wants. Incidentally, I suspect this is not working in the test suite, so I am drilling down now.

-- Internal function to get the default screenshot directory
-- 
-- Can be expanded to read X environment variables or configuration files.
local function get_default_dir()
  -- This can be expanded
  local d = os.getenv("HOME")
  
  if d then
    d = string.gsub(d, '/*$', '/') .. 'Images/'
    if os.execute("bash -c \"if [ -d \\\"" .. d .. "\\\" -a -w \\\"" .. d ..
                    "\\\" ] ; then exit 0 ; else exit 1 ; fi ;\"") then
      return d
    end
  end

  return nil
end

[poisson-aerohead]

OK, problem solved, well, at least identified. It seems that $HOME in the Xephyr environment is '/dev/null/'. I guess I can understand why this would be the case. I don't know how to fix it to run on the general machine, so I need some input from you all.

Here is the thing, my initialization code is going to try set the 'directory' property to something writable. I suppose I can try to change the initialization logic, but it seems like a good idea to do it the way I did and not weaken the class module to accommodate the test suite. So, what is a good, robust way for the test script to find a writable directory? It does not actually need to write there (the test suite is currently not saving any files). But the init logic will check that the directory is writable.

[actionless]

i was talking of these specifically

[poisson-aerohead]

OK, I'll make them dir, prfx or something.

[actionless]

but those would be nice also to make more maintainable

if you have troubles typing long names - you can just press Ctrl+xn in vim to auto-complete it

[poisson-aerohead]

Yeah its OK, I don't mind. If you look, most of the names are descriptive. It is usually just temporaries in helper functions where I don't do that, but I'll clean them up for the next push.

[actionless]

frame_color

[poisson-aerohead]

The constructor I intentionally did this with everything to avoid name collision. I thought it would be easier to follow. Maybe the constructor local variables should all be named as '_frame_color' ?

[actionless]

or mb try using russian 'а' and just name them the same? ;)

[actionless]

module.frame_color = ....

[poisson-aerohead]

I have updated these three tentatively (this is how they function if no argument provides a value)

-- Configuration data
local global_screenshot_dir = nil
local global_screenshot_prefix = nil
local global_frame_color = gears.color("#000000")
local initialized = nil

[actionless]

using module properties would be better, but now it looks at least readable 👍

[actionless]

leave short var names for either logically reasonable (like x, y coordinates, etc) or ones conventional for awesome API (c, s, cr, etc)

[actionless]

what the difference between dir and directory??

[poisson-aerohead]

So that one is actually for a reason but I can do a better name. That is the escaped string to be passed through bash. You'll see that if dir is writable, that function returns directory. That is why there are two.

[actionless]

that should be clear from var name - not from your comment

it makes no sense giving basically the same name to 2 variables which have different logical meaning

[actionless]

also please double-check what none of your helper functions is already implemented in gears standard library (particularly gears.filesystem)

[actionless]

and again, prfx = prefix - that's very confusing to understand without unwinding every code branch

[poisson-aerohead]

That is just avoiding a collision between the argument name and the local variable which will be stored as a property if it is valid.

[actionless]

try to name the var more clear according to their logical roles in the code, not just randomly duplicate the same name and "avoid a collision"

[actionless]

It seems that $HOME in the Xephyr environment is '/dev/null/'

tests/run.sh
16:export HOME=/dev/null
199:        XDG_CONFIG_HOME="$build_dir"

[actionless]

read XDG spec about images' dir

[poisson-aerohead]

I found this on Archlinux on the user directories. How do you want this handled for the screenshot module? It looks like it would be in '~/.config/user-dirs.dirs'. I say in the comment we can expand it to include parsing such a file. Is there already a gears routine to get this?
EDIT: This too.

[actionless]

see our gears.filesystem please for similar functions (or mb even better add them there, not here)

[actionless]

why do you trying to find XDG spec on arch linux website? here is not only explained everything, but even given example (very similar to our gears, again):

https://www.freedesktop.org/wiki/Software/xdg-user-dirs/

[actionless]

i believe the most practical way would be to parse that file and replace $HOME to currently detected home dir

this is still not very robust - but would avoid making code asynchronous for external call of bash

I implemented the changes