Add Programname and Windowname to logs #152

t0mOURCSSn · Jul 28, 2016

@kernc
Hi I implemented a feature to include the program & window name in logkeys' logs.
I would be honored to have my code in your project.
New to git and the whole GitHub forking aspect.
I'll gladly explain in detail the differences.
Attached you will find an example log.

The main changes are:
-added argument "--programinfo"
-the code "on key press (EV_MAKE)" was split into 2 functions, one for newlines and one to encode the scan_codes

Issues/Questions:
Currently using "xprop" to determine the window id and name, maybe there is a better command?

kernc · Aug 2, 2016

Thanks for this, I'd be honored to accept it. It fulfills one of the first enhancement requests, #3.

kernc · Aug 2, 2016

kernc reviewed Aug 2, 2016

View changes

kernc · Aug 2, 2016

src/logkeys.cc

+    //// on processid change update program_info write '[process name] "process title" > '
+    //// on process title change (like firefox tabs) would be better. possibly more ressource intensive?
+    if (args.flags & FLAG_PROGRAMINFO) {
+      window_id = execute(COMMAND_STR_AWID);


This spawns three new processes on each keypress. This indeed feels somewhat intensive. Did you perhaps investigate how cumbersome the use of relevant Xlib function calls would be?

kernc · Aug 2, 2016

kernc reviewed Aug 2, 2016

View changes

kernc · Aug 2, 2016

src/args.cc

@@ -51,6 +52,7 @@ void process_command_line_arguments(int argc, char **argv)
    {"export-keymap", required_argument, &flags, FLAG_EXPORT_KEYMAP},
    {"no-func-keys",  no_argument,       &flags, FLAG_NO_FUNC_KEYS},
    {"no-timestamps", no_argument,       &flags, FLAG_NO_TIMESTAMPS},
+    {"programinfo",   no_argument,       &flags, FLAG_PROGRAMINFO},


You correctly call it process later on. It's a process that is running and has a window. I would prefer this being exposed as --process-info or --window-title to be more verbose about what this logs (window's name and title, in particular).

kernc · Aug 2, 2016

kernc reviewed Aug 2, 2016

View changes

kernc · Aug 2, 2016

src/logkeys.cc

+// active window id, title, name
+#define COMMAND_STR_AWID    "xprop -root 32x '\\t$0' _NET_ACTIVE_WINDOW | cut -f 2"
+#define COMMAND_STR_AWTITLE "xprop -id $(" COMMAND_STR_AWID ") _NET_WM_NAME | cut -d '=' -f 2"
+#define COMMAND_STR_AWPNAME "xprop -id $(" COMMAND_STR_AWID ") WM_CLASS | awk '{print $4}' | sed 's:^.\\(.*\\).$:\\1:'"


Seems like you can use xprop -id 0x59ab7b9 0s '\t$1' WM_CLASS | cut -f2- as bit lighter alternative.

kernc · Aug 2, 2016

kernc reviewed Aug 2, 2016

View changes

kernc · Aug 2, 2016

src/logkeys.cc

@@ -531,7 +611,9 @@ int main(int argc, char **argv)
        }
      }
    }
-
+
+    ////possible conflict if key repeated and program changed???


How would this happen?

this was just a comment i forgot to remove.

kernc · Aug 2, 2016

I think there is a user's manual file somewhere in the distribution. Perhaps mention the new switch there as well?

Overall, looks good to me. @jzohrab, do you, as the previous most active maintainer, have something to add?

kernc · Aug 2, 2016

kernc reviewed Aug 2, 2016

View changes

kernc · Aug 2, 2016

src/logkeys.cc

+
+// active window id, title, name
+#define COMMAND_STR_AWID    "xprop -root 32x '\\t$0' _NET_ACTIVE_WINDOW | cut -f 2"
+#define COMMAND_STR_AWTITLE "xprop -id $(" COMMAND_STR_AWID ") _NET_WM_NAME | cut -d '=' -f 2"


If the window title contains '=', this will include only the portion of the tile up to it. Use cut -d'=' -f2-.

t0mOURCSSn · Aug 3, 2016

Hi
you raise valid points. I will look into them as soon as i have the time, that should be before the end of the week.

t0mOURCSSn · Aug 6, 2016

t0mOURCSSn reviewed Aug 6, 2016

View changes

t0mOURCSSn · Aug 6, 2016

src/logkeys.cc

+      window_id = execute(COMMAND_STR_AWID);
+
+      if (window_id.compare(old_window_id) != 0) {
+        cur_process_name = execute(COMMAND_STR_AWPNAME);


COMMAND_STR_AWPNAME contains COMMAND_STR_AWID to determine the window id.
Since here we already have the window id, it would be better to change COMMAND_STR_AWPNAME and COMMAND_STR_AWTITLE to use the already determined window_id, maybe by splitting them up?
this way we would use xprop to get the window id only once, instead of 3 times.

t0mOURCSSn · Aug 6, 2016

t0mOURCSSn reviewed Aug 6, 2016

View changes

t0mOURCSSn · Aug 6, 2016

src/logkeys.cc

    if (args.flags & FLAG_WINDOW_TITLE) {
      window_id = execute(COMMAND_STR_AWID);

+      //// really ugly!


really ugly this part!

kernc · Aug 6, 2016

Just to be safe, don't go into replacing xprop calls with Xlib calls. I now think it's better if logkeys doesn't depend on X.

t0mOURCSSn · Aug 6, 2016

I made the changes you suggested in the comments.

Apart from the 'really ugly' commented part, I think this might be the final non-X11 version. (unless we find other errors/improvements that is)
I am working on an X11 version with events.
You might want to consider adding a "milestone"/tag either before this current pull request or before the X11 version. That way we could allow for backwards-compatibility or let people download the "OG"-version.

For the X11 version:
I tried to implement X11 with XNextEvent to recieve active-window-changed events with XSelectInput(Display, Window, PropertyChangeMask) and get WM_NAME and WM_CLASS with the help of XGetWindowProperty().

While I was able to make the code snippet work in a separate while loop in main(), I was unable to make it work in the default while (read(input_fd, &event, sizeof(struct input_event)) > 0) loop.
Both XNextEvent and your while loop are listening for events.
Possible solutions I found:

Find an alternative to XNextEvent. There are ways to cicle for events in X11 but I can't seem to get further with X11 without expending unreasonable efforts.
Asynchronously multithread the X11 get-window-name function and the main while loop. The function would just bruteforce a newline with the necessary information and the main loop remains untouched. It's currently working well but I still have to figure out some minor things.

I just saw your update. How do we proceed? My code is almost finished, should we just keep it in a separate fork?

t0mOURCSSn · Aug 8, 2016

FYI: I did a quick google search and it seems saving clipboard content can only be done with the help of X11.

kernc · Aug 14, 2016

The problem with Xlib is that, unless it was dynamically loaded if present (dlopen & co.), it would have to be installed in order to compile / run logkeys. Which would make it somewhat less useful on headless servers where X is not used / running / desired / installed. If anything, X calls should be ifdef-ed with a compile-time flag.

Similarly to xprop, one can get clipboard contents with xclip (with X running, of course). But don't ram too much into this PR.

kernc · Aug 14, 2016

kernc reviewed Aug 14, 2016

View changes

kernc · Aug 14, 2016

src/logkeys.cc

@@ -48,10 +48,17 @@
 #define COMMAND_STR_DUMPKEYS ( EXE_DUMPKEYS " -n | " EXE_GREP " '^\\([[:space:]]shift[[:space:]]\\)*\\([[:space:]]altgr[[:space:]]\\)*keycode'" )
 #define COMMAND_STR_GET_PID  ( (std::string(EXE_PS " ax | " EXE_GREP " '") + program_invocation_name + "' | " EXE_GREP " -v grep").c_str() )

+#define COMMAND_STR_DEVICE    EXE_GREP " -E 'Handlers|EV' /proc/bus/input/devices | " EXE_GREP " -B1 120013 | " EXE_GREP " -Eo event[0-9]+"


-B1 120013 is not equal to what was here before?

kernc · Aug 14, 2016

kernc reviewed Aug 14, 2016

View changes

kernc · Aug 14, 2016

src/logkeys.cc

+      //// really ugly!
+      if (window_id.compare(old_window_id) != 0) {
+        process_name = sprintf("xprop -id $(%s) 0s '\\t$1' WM_CLASS | cut -f2-", window_id.c_str());
+        window_title = sprintf("xprop -id $(%s) _NET_WM_NAME | cut -d'=' -f2-", window_id.c_str());


Perhaps #define these commands with the rest of shell commands defined above?

kernc · Aug 14, 2016

kernc reviewed Aug 14, 2016

View changes

kernc · Aug 14, 2016

src/usage.cc

@@ -27,6 +27,7 @@ void usage()
 "      --export-keymap=FILE  export configured keymap to FILE and exit\n"
 "      --no-func-keys        log only character keys\n"
 "      --no-timestamps       don't prepend timestamps to log file lines\n"
+"      --window-title        add active program window name and window title to log file\n"


Please also add an entry into the manual file, man/logkeys.8.

t0mOURCSSn · Aug 19, 2016

Is there still interest in this feature?
I have no idea on exactly how to use ifdef in order to find out if x is installed.

t0mOURCSSn added some commits Aug 6, 2016

changed --programinfo flag to --window-title

Loading status checks…

a037bc7

Update usage.cc

Loading status checks…

85c8f39

changed programinfo flag to window-title

Loading status checks…

cc9510a

Changed the command strings for slight performance improvement

Loading status checks…

b2e44dc

moved window-info update code into EV_MAKE if clause, since we are on…
…ly interested in "visible" key presses. changed printf into snprintf. updated manual file to include --window-title. #defined commands that were left in code previously.

6268cc7

kernc/logkeys

Join GitHub today

Add Programname and Windowname to logs #152

Conversation

t0mOURCSSn commented Jul 28, 2016 • edited

t0mOURCSSn added some commits Jul 25, 2016

This comment has been minimized.

kernc commented Aug 2, 2016

kernc reviewed Aug 2, 2016 View changes

This comment has been minimized.

kernc reviewed Aug 2, 2016 View changes

This comment has been minimized.

kernc reviewed Aug 2, 2016 View changes

This comment has been minimized.

kernc reviewed Aug 2, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kernc commented Aug 2, 2016

kernc reviewed Aug 2, 2016 View changes

This comment has been minimized.

kernc Aug 2, 2016 • edited

This comment has been minimized.

t0mOURCSSn commented Aug 3, 2016

t0mOURCSSn added some commits Aug 6, 2016

t0mOURCSSn reviewed Aug 6, 2016 View changes

This comment has been minimized.

t0mOURCSSn reviewed Aug 6, 2016 View changes

This comment has been minimized.

This comment has been minimized.

kernc commented Aug 6, 2016

This comment has been minimized.

t0mOURCSSn commented Aug 6, 2016 • edited

This comment has been minimized.

t0mOURCSSn commented Aug 8, 2016

This comment has been minimized.

kernc commented Aug 14, 2016

kernc reviewed Aug 14, 2016 View changes

This comment has been minimized.

kernc reviewed Aug 14, 2016 View changes

This comment has been minimized.

kernc reviewed Aug 14, 2016 View changes

This comment has been minimized.

kernc Aug 14, 2016 • edited

This comment has been minimized.

t0mOURCSSn commented Aug 19, 2016

t0mOURCSSn commented Jul 28, 2016 •

edited

kernc reviewed Aug 2, 2016

View changes

kernc reviewed Aug 2, 2016

View changes

kernc reviewed Aug 2, 2016

View changes

kernc reviewed Aug 2, 2016

View changes

kernc reviewed Aug 2, 2016

View changes

kernc Aug 2, 2016 •

edited

t0mOURCSSn reviewed Aug 6, 2016

View changes

t0mOURCSSn reviewed Aug 6, 2016

View changes

t0mOURCSSn commented Aug 6, 2016 •

edited

kernc reviewed Aug 14, 2016

View changes

kernc reviewed Aug 14, 2016

View changes

kernc reviewed Aug 14, 2016

View changes

kernc Aug 14, 2016 •

edited