Feature 14 #36

ileGITimo · 2018-11-23T17:39:48Z

using feature-14 branch this adds multi screen capability and old unclutter onescreen and not flags (albeit doing case insensitive match against window's name or class name or class resource

This commit introduces compatibility with the original unclutter project by allowing both all of its command-line parameters as well as the single dash syntax for parameters. fixes Airblader#14

This commit introduced proper support for specifying the X display to use. relates to Airblader#14

ileGITimo · 2018-11-23T17:48:27Z

Forgot to add that the old unclutter works with window managers that create a virtual root (e.g. tvtwm, swm) but the xfixes version doesn't., My PR doesn't address that, which may be needed for full compatibility and the two people that still use tvtwm:-)

Airblader

Hey, many thanks for the PR. I have a few comments, mostly just small things. I'd also wanna give this a go locally, but in the meantime we can address these already.

Airblader · 2018-11-24T11:01:51Z

man/unclutter-xfixes.man

+See *--exclude-root*.
+
+*--onescreen*::
+This argument is ignored.


This needs to be updated

Updated in new commit

Airblader · 2018-11-24T11:02:11Z

man/unclutter-xfixes.man

+This argument is ignored.
+
+*--root*::
+This argument is ignored as this is the default behavior in unclutter-xfixes.


This needs to be updated, though we should keep the information that it's the default.

Updated in new commit.

Airblader · 2018-11-24T11:02:22Z

man/unclutter-xfixes.man

+This argument is ignored.
+
+*--not*::
+This argument is ignored.


This needs to be updated (and properly described)

Updated in new commit.

Airblader · 2018-11-24T11:02:49Z

include/globals.h

@@ -9,3 +9,10 @@ extern Display *display;
 extern int xi_ext_opcode;

 extern Config config;
+
+extern Matches *matches;


Doesn't this belong into the config?

Moved to config

Airblader · 2018-11-24T11:03:22Z

src/unclutter.c

+    if (roots == NULL)
+        bail("Failed to allocate root windows.");
+    for(screen = 0; screen < num_screens; screen++)
+      roots[screen] = XRootWindow(display, screen);


The formatting here seems inconsistent

Not sure where. Tweaked it.

I meant that this is only intended by two spaces rather than four.

Airblader · 2018-11-24T11:17:15Z

src/event.c

+    return false;
+}
+
+static Window last_avoided = None;


I think we should move this into the global.h where all the global stuff is stored. At the very least it should be at the top of the file.

Moved to the top of the file. This is not a global. I would actually move it as a static inside the is_on_ignored_list function, the only place where it is used. Same for last_cursor_pos, which could be moved inside x_check_cb, unwrapped as last_cursor_pos_x and last_cursor_pos_y, which would get rid of the need for the coordinates_t type.

Airblader · 2018-11-24T11:17:36Z

src/event.c

+            last_avoided = win;
+            return true;
+        }
+    } while (XQueryPointer(display, win_in, &win_dummy,


At least the XQueryPointer call should be on a single line for readability.

src/event.c

Airblader · 2018-11-24T11:18:56Z

src/unclutter.c

+                } else if (OPT_NAME_IS("debug")) {
+                    config.debug = true;
+                    break;
+                } else if (OPT_NAME_IS("keystroke") ||


I think these can all go on a single line

Airblader · 2018-11-24T11:19:34Z

src/unclutter.c

+            bail("Failed to allocate space for matches");
+        for (c = 0; optind < argc ; c++, optind++) {
+            char *name = argv[optind];
+            matches[c].name =  name;


There seems to be an extra space between = and name

ileGITimo · 2018-11-24T16:24:08Z

Pushed almost all requested changes to my feature-14 branch, should be reflected in the pull request.
Sorry, not github literate about all this comment stuff.
Let me know if you still like more changes.

Airblader

Just the one last formatting thing (I've clarified it) and then could you please also squash the commits together? Thank you!

Airblader · 2018-11-24T22:23:39Z

man/unclutter-xfixes.man

+
+*--not*::
+This argument will result in all arguments that aren't options or option arguments, to be collected
+into a list that specifies windows where the cursor shall not be removed.  These will be the windows


nit: There's an extra space here before "These"

Airblader · 2018-11-24T22:24:08Z

man/unclutter-xfixes.man

+This argument will result in all arguments that aren't options or option arguments, to be collected
+into a list that specifies windows where the cursor shall not be removed.  These will be the windows
+where an element of the list matches, in a case insensitive comparison, the starting characters of
+either the WM_NAME, or the name or class of the WM_CLASS, properties of the window.  (Note that this


nit: Another extra space

Airblader · 2018-11-24T22:36:34Z

src/unclutter.c

+    if (roots == NULL)
+        bail("Failed to allocate root windows.");
+    for(screen = 0; screen < num_screens; screen++)
+      roots[screen] = XRootWindow(display, screen);


I meant that this is only intended by two spaces rather than four.

src/event.c

ileGITimo · 2018-11-25T02:16:18Z

Removed the extra spaces in the man page.
Reformatted one function with 4 space indentation.
(Hopefully) squashed all commits into one.

Implements old unclutter root, onescreen and not options.

ileGITimo · 2018-11-27T15:45:35Z

found some more indentation problems in cursor.c, forced push/squashed a new commit

Airblader · 2018-11-27T20:15:11Z

Thanks! I'm a little short on time right now but just wanted to let you know that it's on my list and I'll get to it!

ileGITimo · 2018-12-09T18:08:07Z

Thanks! Now I need to decide whether I'll keep building this from your github repo or try to push this into Fedora, which means volunteering to be a maintainer ...

Airblader · 2018-12-09T18:11:25Z

Volunteer to be a maintainer! ;-)

Airblader added 2 commits October 19, 2016 18:32

Establish compatibility with old unclutter.

953ae43

This commit introduces compatibility with the original unclutter project by allowing both all of its command-line parameters as well as the single dash syntax for parameters. fixes Airblader#14

Implemented -display argument.

fa8df8a

This commit introduced proper support for specifying the X display to use. relates to Airblader#14

Airblader requested changes Nov 24, 2018

View reviewed changes

ileGITimo force-pushed the feature-14 branch from 536bbd8 to 57fea4d Compare November 25, 2018 02:12

Works for displays with more than one screen

8ef5a7c

Implements old unclutter root, onescreen and not options.

ileGITimo force-pushed the feature-14 branch from 6956ff7 to 8ef5a7c Compare November 27, 2018 15:43

Airblader approved these changes Dec 9, 2018

View reviewed changes

Airblader merged commit 73f1d87 into Airblader:master Dec 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature 14 #36

Feature 14 #36

ileGITimo commented Nov 23, 2018

ileGITimo commented Nov 23, 2018

Airblader left a comment

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

Airblader Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo Nov 24, 2018

ileGITimo commented Nov 24, 2018

Airblader left a comment

Airblader Nov 24, 2018

Airblader Nov 24, 2018

Airblader Nov 24, 2018

ileGITimo commented Nov 25, 2018

ileGITimo commented Nov 27, 2018

Airblader commented Nov 27, 2018

ileGITimo commented Dec 9, 2018

Airblader commented Dec 9, 2018

Feature 14 #36

Feature 14 #36

Conversation

ileGITimo commented Nov 23, 2018

ileGITimo commented Nov 23, 2018

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ileGITimo commented Nov 24, 2018

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ileGITimo commented Nov 25, 2018

ileGITimo commented Nov 27, 2018

Airblader commented Nov 27, 2018

ileGITimo commented Dec 9, 2018

Airblader commented Dec 9, 2018